Skip to content
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

Add arch cask DSL #13657

Merged
merged 3 commits into from Aug 9, 2022
Merged

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Aug 5, 2022

As discussed in Homebrew/homebrew-cask#129182, this PR adds two new DSL items to casks. The first is an arch method which takes two items and, when called without arguments, will return whichever item corresponds to the current arch. Here's an example:

cask "foo" do
  version "1.0.0"
  sha256 "..."
  arch arm: "darwin-arm64", intel: "darwin"

  url "https://example.com/foo/#{version}/#{arch}/foo.dmg"
end

On arm the url will be https://example.com/foo/1.0.0/darwin-arm64/foo.dmg and on Intel it will be https://example.com/foo/1.0.0/darwin/foo.dmg.

There are several casks that currently have multiple variables that change based on Hardware::CPU.type, but instead of adding an on_arch_conditional method, I suggest we simply pass arrays to arch like this:

cask "foo" do
  version "1.0.0"
  sha256 "..."
  arch arm: ["darwin-arm64", "arm"], intel: ["darwin", "intel"]

  url "https://example.com/foo/#{version}/#{arch.first}/foo-#{arch.last}.dmg"
end

Additionally, you can currently leave off either the arm or intel parameter to default to nil (which converts to "" when interpolated). If you pass an array to arch and leave off the other arch parameter, it will default to an array of the same size filled with nil. For example:

arch intel: "intel"

arch.to_s # returns "" on arm and "intel" on intel
arch arm: ["darwin-arm64", "arm"]

arch.first.to_s # returns "darwin-arm64" on arm and "" on intel
arch.last.to_s  # returns "arm" on arm and "" on intel

@BrewTestBot
Copy link
Member

Review period will end on 2022-08-08 at 20:09:31 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 5, 2022
@Rylan12 Rylan12 changed the title Add arch and on_arch_conditional cask DSLs Add arch cask DSL Aug 5, 2022
@Rylan12 Rylan12 force-pushed the add-arch-and-variable-cask-dsl branch from ed1c23f to dd72f1a Compare August 5, 2022 21:15
@MikeMcQuaid
Copy link
Member

  arch arm: "darwin-arm64", intel: "darwin"

Love this.

arch arm: ["darwin-arm64", "arm"]
arch.first.to_s # returns "darwin-arm64" on arm and "" on intel
arch.last.to_s  # returns "arm" on arm and "" on intel

I think this is unnecessarily confusing and unintuitive. Can you explain why this is needed and why arch needs different values at different times? IMO we should just use arch for one of these and set another variable for the latter unless this is a super common pattern.

If we really, really want to have some sort of "multi-variable arch" I'd suggest something like this:

arch arm: { default: "arm", livecheck: "darwin-arm64" }
arch.to_s  # returns "arm" on arm and "" on intel
arch.default.to_s  # returns "arm" on arm and "" on intel
arch.livecheck.to_s # returns "darwin-arm64" on arm and "" on intel

@Rylan12
Copy link
Member Author

Rylan12 commented Aug 8, 2022

unless this is a super common pattern.

It looks like there are currently 16 cases of this. So not super common. Either way, it's a small number of casks that are impacted, so I think it's a decision between adding the on_arch_conditional DSL for only 16 cases or accepting the less intuitive system for 16 cases.

@MikeMcQuaid
Copy link
Member

accepting the less intuitive system for 16 cases.

less intuitive system IMO 👍🏻

@BrewTestBot
Copy link
Member

Review period ended.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 8, 2022
@Rylan12
Copy link
Member Author

Rylan12 commented Aug 9, 2022

I think this is unnecessarily confusing and unintuitive

I realized I'm not 100% sure what you were talking about here. It sounds like we're on the same page about not adding on_arch_conditional in favor of passing an array to arch for the 16 cases where it's needed.

Were you talking about defaulting to an array when one parameter is blank and the other is an array? If so, I don't feel strongly about that and am happy to simply default to nil and require an array be passed explicitly when needed. This currently only happens 2 times in homebrew/cask so it's probably clearer to just require ["", ""] in those two cases.

@MikeMcQuaid
Copy link
Member

I realized I'm not 100% sure what you were talking about here. It sounds like we're on the same page about not adding on_arch_conditional in favor of passing an array to arch for the 16 cases where it's needed.

Sorry, I mean specifically arch arm: ["darwin-arm64", "arm"] and having to call arch.first.to_s.

I don't think we should ever be passing in an array here. This should be two separate variables, one being arch and the other being something else OR passing a Hash and then having named methods (OpenStruct style).

@Rylan12
Copy link
Member Author

Rylan12 commented Aug 9, 2022

Gotcha. That is what I thought you meant. I think I'm okay with using a hash instead of relying on #first and #last if you think that's better. Does "accepting the less intuitive system for 16 cases" still apply or should I implement the hash alternative?

Also, in case it wasn't clear: the #to_s at the end won't be used most of the time since arch is usually used in string interpolation. I just added it to the example to make it clear that we were getting the string.

@MikeMcQuaid
Copy link
Member

Does "accepting the less intuitive system for 16 cases" still apply or should I implement the hash alternative?

I defer to you there. My opinion is probably "less intuitive system" for those 👍🏻

@Rylan12
Copy link
Member Author

Rylan12 commented Aug 9, 2022

👍 Okay. It's also easier to implement the array method, so I'll stick with that for now. I think this PR is ready now

Comment on lines 253 to 254
# If arm and intel are arrays and one isn't specified, default to an array that is the same size as the other
empty_value = Array.new(arm&.count || intel&.count) if arm.is_a?(Array) || intel.is_a?(Array)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my apologies for poor communication: I'm proposing that this only accepts values and not arrays and that they array form we move into two separate declarations e.g.

arch arm: "arm", intel: "intel"
livecheck_arch = something_else arm: "larm", intel: "lintel"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is what I thought you meant by

My opinion is probably "less intuitive system" for those 👍🏻

What is the "less intuitive system" in your mind?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example I've had here is what I considered the "less intuitive" system although it's probably "more intuitive" 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hahaha okay, gotcha! I'm okay with either one since it's so few cases. Let me revert this back to include the original DSL proposal and see if that's what you're looking for.

@Rylan12 Rylan12 merged commit 044fefd into Homebrew:master Aug 9, 2022
@Rylan12 Rylan12 deleted the add-arch-and-variable-cask-dsl branch August 9, 2022 19:49
@github-actions github-actions bot added the outdated PR was locked due to age label Sep 9, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants