Skip to content

Add arch cask DSL - #13657

Merged
Rylan12 merged 3 commits into
Homebrew:masterfrom
Rylan12:add-arch-and-variable-cask-dsl
Aug 9, 2022
Merged

Add arch cask DSL#13657
Rylan12 merged 3 commits into
Homebrew:masterfrom
Rylan12:add-arch-and-variable-cask-dsl

Conversation

@Rylan12

@Rylan12 Rylan12 commented Aug 5, 2022

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor

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
@MikeMcQuaid

Copy link
Copy Markdown
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

Rylan12 commented Aug 8, 2022

Copy link
Copy Markdown
Member Author

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
Copy Markdown
Member

accepting the less intuitive system for 16 cases.

less intuitive system IMO 馃憤馃徎

@BrewTestBot

Copy link
Copy Markdown
Contributor

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

Rylan12 commented Aug 9, 2022

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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

Rylan12 commented Aug 9, 2022

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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

Rylan12 commented Aug 9, 2022

Copy link
Copy Markdown
Member Author

馃憤 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 thread Library/Homebrew/cask/dsl.rb Outdated
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

3 participants