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

bottle merge: add support for linux cellar #9310

Merged
merged 1 commit into from
Nov 26, 2020
Merged

Conversation

iMichka
Copy link
Member

@iMichka iMichka commented Nov 26, 2020

The first attempt to build and pull a formula (hello) for linux in homebrew-core
resulted in a wrong cellar line being added to the formula's bottle block.

How to test/debug this, using the 4 bottles that where built for hello:
brew bottle --merge --debug hello--2.10_1.mojave.bottle.json hello--2.10_1.x86_64_linux.bottle.json hello--2.10_1.big_sur.bottle.json hello--2.10_1.catalina.bottle.json

This command would add create the following bottle block:

bottle do
cellar "/home/linuxbrew/.linuxbrew/Cellar"
sha256 "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur
sha256 "5334dd344986e46b2aa4f0471cac7b0914bd7de7cb890a34415771788d03f2ac" => :catalina
sha256 "22948764d8f8d7be4870ff92dae64d986eb63a9150b219c20fff87d1a6aa93d6" => :mojave
sha256 "702dc7f78444d2f4f1c19324be654bcbb8b99dd0e9ce26c3e2fbc3b6464a189f" => :x86_64_linux
end

After the change in this PR, the result is the following:

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

The brew bottle --merge code will pick the most common cellar line between the 4 bottles, by order of priority:

  • non-relocatable (fixed cellar path)
  • cellar :any
  • cellar :any_skip_relocation

In the case of the hello bottle, the 3 mac bottles are "cellar :any_skip_relocation", and the linux bottle
is non-relocatable. So the linux bottle wins and the code correctly determines that the 4 bottles should
be non-relocatable.

In that case, the /home/linuxbrew/.linuxbrew/Cellar path is defined as cellar, and by convention
we do not write that out to the formula file, hence the cellar path check that needs to be modified
in this PR.

This PR also fixes the same situation for mac ARM cellar paths

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?
  • Have you successfully run brew man locally and committed any changes?

@BrewTestBot
Copy link
Member

Review period will end on 2020-11-27 at 13:41:08 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Nov 26, 2020
Library/Homebrew/global.rb Outdated Show resolved Hide resolved
Library/Homebrew/dev-cmd/bottle.rb Outdated Show resolved Hide resolved
@iMichka
Copy link
Member Author

iMichka commented Nov 26, 2020

Thanks @MikeMcQuaid , I made the asked changes. I was wondering why there was a hard-coded "/usr/local/Cellar" in there but it was probably a legacy piece of code that never got updated :)

The first attempt to build and pull a formula (hello) for linux in homebrew-core
resulted in a wrong cellar line being added to the formula's bottle block.

How to test/debug this, using the 4 bottles that where built for hello:
brew bottle --merge --debug hello--2.10_1.mojave.bottle.json hello--2.10_1.x86_64_linux.bottle.json hello--2.10_1.big_sur.bottle.json hello--2.10_1.catalina.bottle.json

This command would add create the following bottle block:

  bottle do
    cellar "/home/linuxbrew/.linuxbrew/Cellar"
    sha256 "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur
    sha256 "5334dd344986e46b2aa4f0471cac7b0914bd7de7cb890a34415771788d03f2ac" => :catalina
    sha256 "22948764d8f8d7be4870ff92dae64d986eb63a9150b219c20fff87d1a6aa93d6" => :mojave
    sha256 "702dc7f78444d2f4f1c19324be654bcbb8b99dd0e9ce26c3e2fbc3b6464a189f" => :x86_64_linux
  end

After the change in this PR, the result is the following:

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

The brew bottle --merge code will pick the most common cellar line between the 4 bottles, by order of priority:
- non-relocatable (fixed cellar path)
- cellar :any
- cellar :any_skip_relocation

In the case of the hello bottle, the 3 mac bottles are "cellar :any_skip_relocation", and the linux bottle
is non-relocatable. So the linux bottle wins and the code correctly determines that the 4 bottles should
be non-relocatable.

In that case, the /home/linuxbrew/.linuxbrew/Cellar path is defined as cellar, and by convention
we do not write that out to the formula file, hence the cellar path check that needs to be modified
in this PR.

This PR also fixes the same situation for mac ARM cellar paths
@iMichka iMichka added the critical Critical change which should be shipped as soon as possible. label Nov 26, 2020
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Nov 26, 2020
@iMichka iMichka merged commit d42e8c7 into Homebrew:master Nov 26, 2020
@iMichka iMichka deleted the cellar branch November 26, 2020 22:31
@iMichka
Copy link
Member Author

iMichka commented Nov 26, 2020

Follow-up issue: #9315

@iMichka iMichka removed the critical Critical change which should be shipped as soon as possible. label Nov 26, 2020
@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Nov 26, 2020
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 27, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 27, 2020
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 27, 2020
@BrewTestBot
Copy link
Member

Review period ended.

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.

5 participants