-
-
Notifications
You must be signed in to change notification settings - Fork 9.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: allow linux binaries in casks #19121
base: master
Are you sure you want to change the base?
Conversation
ddf1c8e
to
852e228
Compare
Okay, as far as I can tell from https://sorbet.org/docs/class-of this should work since I'm also fine removing the types, but that seems counterproductive. |
In order to generate the linux variations in the API, I believe you'll need to update the brew/Library/Homebrew/cask/cask.rb Line 464 in 55f399c
should become something like this (copied from the formula equivalent): brew/Library/Homebrew/formula.rb Line 2663 in 55f399c
This may cause problems until |
I'm planning to finish this up without API support, make some casks compatible and then do the API and the removal of the MacOSVersion logic. |
852e228
to
37e6183
Compare
That sounds fine. |
37e6183
to
2e9165a
Compare
2e9165a
to
29e28d1
Compare
One more thing to keep in mind before we make this widely available, and I'd like @MikeMcQuaid to weigh in on that one: What do we want to do with existing casks? Mark all of binary-only casks as |
I think we should add |
What does binary-only or "allow Linux binaries" mean in the context of this PR, I'm a bit confused. What casks are expected to work or not work after this is merged? |
After this PR, casks that only have binary or zap will be installable on Linux*. Obviously most of the casks that meet those criteria would be binaries that only work on macOS. So we need some way to prevent that.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good thanks! Couple of final bits.
@@ -311,17 +311,18 @@ def version(arg = nil) | |||
# | |||
# ```ruby | |||
# sha256 arm: "7bdb497080ffafdfd8cc94d8c62b004af1be9599e865e5555e456e2681e150ca", | |||
# intel: "b3c1c2442480a0219b9e05cf91d03385858c20f04b764ec08a3fa83d1b27e7b2" | |||
# intel: "b3c1c2442480a0219b9e05cf91d03385858c20f04b764ec08a3fa83d1b27e7b2" | |||
# linux: "1a2aee7f1ddc999993d4d7d42a150c5e602bc17281678050b8ed79a0500cc90f" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this imply linux x86_64? Given linux arm will be coming in the near future: what do you invisage doing there? linux_intel
or something may make more sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered this, but it feels weird:
sha256 macos:
arm: "abc"
intel: "def"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feel like having some Linux-specific makes more sense until the majority of casks support Linux
# ``` | ||
# | ||
# @api public | ||
def os(macos: nil, linux: nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to use a linux: nil
state here to infer "this cask does not support Linux", at least in certain cases, rather than needing thousands of depends_on :macos
CC @p-linnane for thoughts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that if you use an on_linux
block this assumption breaks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SMillerDev could be if neither of those are used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll need @Rylan12 to help me with that one. Will probably be somewhere during FOSDEM.
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?That was actually easier than expected.
TODO:
--zap
support for LinuxTODO in the future:
OnSystem::MacOSOnly