Skip to content

bottle --merge: check for all cellars#10130

Merged
iMichka merged 1 commit intoHomebrew:masterfrom
iMichka:prefixes
Dec 24, 2020
Merged

bottle --merge: check for all cellars#10130
iMichka merged 1 commit intoHomebrew:masterfrom
iMichka:prefixes

Conversation

@iMichka
Copy link
Copy Markdown
Member

@iMichka iMichka commented Dec 24, 2020

  • 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 typecheck 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?

@iMichka iMichka added the critical Critical change which should be shipped as soon as possible. label Dec 24, 2020
@BrewTestBot
Copy link
Copy Markdown
Contributor

BrewTestBot commented Dec 24, 2020

Review period ended.

@iMichka
Copy link
Copy Markdown
Member Author

iMichka commented Dec 24, 2020

This fixes (on linux):

Error: --keep-old was passed but there are changes in:
cellar: old: "/home/linuxbrew/.linuxbrew/Cellar", new: :"/home/linuxbrew/.linuxbrew/Cellar"

@iMichka iMichka force-pushed the prefixes branch 3 times, most recently from e27fe3f to 0ae581f Compare December 24, 2020 11:11
@iMichka iMichka changed the title bottle --merge: check for all prefixes and repositories bottle --merge: check for all cellars Dec 24, 2020
@iMichka iMichka enabled auto-merge December 24, 2020 11:46
@iMichka iMichka merged commit 7b24507 into Homebrew:master Dec 24, 2020
@iMichka iMichka deleted the prefixes branch December 24, 2020 11:56
Comment on lines 554 to -555
next if key == :prefix && old_value == Homebrew::DEFAULT_PREFIX
next if key == :cellar && old_value == Homebrew::DEFAULT_REPOSITORY
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.

Would be good to have the same thing for these too:

HOMEBREW_DEFAULT_PREFIX = "/usr/local"
HOMEBREW_DEFAULT_REPOSITORY = "#{HOMEBREW_DEFAULT_PREFIX}/Homebrew"
HOMEBREW_MACOS_ARM_DEFAULT_PREFIX = "/opt/homebrew"
HOMEBREW_MACOS_ARM_DEFAULT_REPOSITORY = HOMEBREW_MACOS_ARM_DEFAULT_PREFIX
HOMEBREW_LINUX_DEFAULT_PREFIX = "/home/linuxbrew/.linuxbrew"
HOMEBREW_LINUX_DEFAULT_REPOSITORY = "#{HOMEBREW_LINUX_DEFAULT_PREFIX}/Homebrew"

Copy link
Copy Markdown
Contributor

@SeekingMeaning SeekingMeaning Dec 24, 2020

Choose a reason for hiding this comment

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

Should we use these instead?

DEFAULT_PREFIX ||= HOMEBREW_DEFAULT_PREFIX
DEFAULT_REPOSITORY ||= HOMEBREW_DEFAULT_REPOSITORY
DEFAULT_CELLAR = "#{DEFAULT_PREFIX}/Cellar"

Their initial values seem to be set here:

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.

We should check them for every platform so we never set it even if we're running brew bottle on a different platform to the platform it's bottling for (e.g. when merging).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, right, I didn't think of that

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 24, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants