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

Allow platform-specific cellars in bottle blocks #9315

Closed
iMichka opened this issue Nov 26, 2020 · 9 comments · Fixed by #10547
Closed

Allow platform-specific cellars in bottle blocks #9315

iMichka opened this issue Nov 26, 2020 · 9 comments · Fixed by #10547
Assignees
Labels
in progress Maintainers are working on this linux outdated PR was locked due to age

Comments

@iMichka
Copy link
Member

iMichka commented Nov 26, 2020

Feature suggestion

A detailed description of the proposed feature

Bottle blocks currently look like this:

bottle do
  cellar :any_skip_relocation
  sha256 "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur
  sha256 "5334dd344986e46b2aa4f0471cac7b0914bd7de7cb890a34415771788d03f2ac" => :catalina
  sha256 "22948764d8f8d7be4870ff92dae64d986eb63a9150b219c20fff87d1a6aa93d6" => :mojave
  sha256 "702dc7f78444d2f4f1c19324be654bcbb8b99dd0e9ce26c3e2fbc3b6464a189f" => :x86_64_linux
end

The cellar line is often different on Linux, and might be different between Mac OS versions too.

The proposal is to have per-platform cellar lines, for example:

bottle do
  sha256 "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur, cellar :any_skip_relocation
  sha256 "5334dd344986e46b2aa4f0471cac7b0914bd7de7cb890a34415771788d03f2ac" => :catalina, cellar :any_skip_relocation
  sha256 "22948764d8f8d7be4870ff92dae64d986eb63a9150b219c20fff87d1a6aa93d6" => :mojave, cellar :any_skip_relocation
  sha256 "702dc7f78444d2f4f1c19324be654bcbb8b99dd0e9ce26c3e2fbc3b6464a189f" => :x86_64_linux, cellar :any
end

Making this work is mostly rewriting the merge function in bottle.rb.
Backward compatibility is a concern of course and need to be maintained with the old syntax.

The motivation for the feature

This is needed for the linuxbrew-core to homebrew-core migration.

How the feature would be relevant to at least 90% of Homebrew users

Having the right cellars for the right platform makes sure things do not go up in flames when we merge both repositories, so it's probably a good thing.

What alternatives to the feature have been considered

  • Use the Mac cellar line for linux: this might lead to some weird breakage
@iMichka iMichka self-assigned this Nov 26, 2020
@iMichka iMichka added in progress Maintainers are working on this linux labels Nov 26, 2020
@MikeMcQuaid
Copy link
Member

Making this work is mostly rewriting the merge function in bottle.rb.

I mildly disagree. I see one step before that and (at least) two after:

  1. adding DSL support for the new cellar: argument which takes priority over the block cellar (and both can be specified)

  2. make the merge logic in bottle.rb start writing the new cellar and never the old one

  3. add a RuboCop to fix up all existing bottle do blocks automatically with brew style --fix

  4. odeprecated the old cellar DSL and eventually odisabled and eventually remove it (across three minor releases)

  5. and 2. should be achieved with the minimum possible number of changes to bottle.rb. It maybe warrants a larger refactoring but that should happen after tests have been added (i.e. tests are added and merged before the refactoring is started and test changes aren't needed during the refactoring) and not as part of 1. or 2.

iMichka added a commit to iMichka/brew that referenced this issue Dec 7, 2020
- Extract the json reading from the json merging: it makes the code hard to understand.
This helps for the separation of concerns: the jsons are read in one method;
and merged in a second one
- Added a first test to check the merge function

No change in behaviour was done, this change is just there to increase code coverage
and to prepare for Homebrew#9315
iMichka added a commit to iMichka/brew that referenced this issue Dec 22, 2020
iMichka added a commit to iMichka/brew that referenced this issue Dec 23, 2020
@iMichka
Copy link
Member Author

iMichka commented Jan 19, 2021

After a bunch of refactoring and cleanup PR's, test additions around the bottle command, and finally the addition of the reading of the new syntax, I'll start working on step 2. Step 2 consists in writing the new cellars to the respective sha256 lines.

@iMichka
Copy link
Member Author

iMichka commented Jan 28, 2021

We are done here, see #10449.

Thanks all for the help!

@iMichka iMichka closed this as completed Jan 28, 2021
@iMichka iMichka reopened this Jan 28, 2021
@iMichka
Copy link
Member Author

iMichka commented Jan 28, 2021

Woups, re-opening we need to discuss step 3 and 4:

add a RuboCop to fix up all existing bottle do blocks automatically with brew style --fix

odeprecated the old cellar DSL and eventually odisabled and eventually remove it (across three minor releases)

Should we just leave this as-is and let the bottling magic fix the things over the years for us? I think that the old syntax will be gone with the next macOS version, so roughly in one year. This leaves also time to external taps to rebuild stuff naturally without noticing.

@MikeMcQuaid
Copy link
Member

add a RuboCop to fix up all existing bottle do blocks automatically with brew style --fix

I think it would be ideal to have this as it'll let us deprecate, disable and remove the old behaviour more quickly. Doesn't mean you have to do it, though! One of our other rubocop experts could pick this up CC @SeekingMeaning @Rylan12 @carlocab @GauthamGoli in case any of you are interested.

@sjackman
Copy link
Member

Brewsci/bio would be a great third-party tap beta test site for such a tool! I'd happily volunteer to try it out.

@iMichka
Copy link
Member Author

iMichka commented Feb 6, 2021

The last remaining step is to:

odeprecated the old cellar DSL

Does someone want to pick that up?

@Rylan12
Copy link
Member

Rylan12 commented Feb 6, 2021

I opened a PR to prepare those deprecations: #10547

@iMichka
Copy link
Member Author

iMichka commented Feb 8, 2021

Cool. I'll close this then, and the deprecations will follow the normal process. Thanks all!

@iMichka iMichka closed this as completed Feb 8, 2021
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Mar 11, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Mar 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
in progress Maintainers are working on this linux outdated PR was locked due to age
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants