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.

Sign up for GitHub

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

Check flavour to prevent mixed architectures #9126

Closed
wants to merge 1 commit into from
Closed

Check flavour to prevent mixed architectures #9126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Library/Homebrew/brew.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ if [[ -n "$HOMEBREW_MACOS" ]]
then
HOMEBREW_DEFAULT_CACHE="${HOME}/Library/Caches/Homebrew"
HOMEBREW_DEFAULT_LOGS="${HOME}/Library/Logs/Homebrew"
HOMEBREW_DEFAULT_PREFIX="/usr/local"
HOMEBREW_DEFAULT_TEMP="/private/tmp"
HOMEBREW_MACOS_ARM_DEFAULT_PREFIX="/opt/homebrew"
Comment on lines +26 to +28
Copy link
Member

Choose a reason for hiding this comment

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

If these stay around given other comments after this:

  • they need to be used in global.rb so that their definitions aren't duplicated
  • a definition should be present for Linux

else
CACHE_HOME="${XDG_CACHE_HOME:-${HOME}/.cache}"
HOMEBREW_DEFAULT_CACHE="${CACHE_HOME}/Homebrew"
Expand Down Expand Up @@ -429,6 +431,8 @@ export HOMEBREW_MACOS_VERSION_NUMERIC
export HOMEBREW_USER_AGENT
export HOMEBREW_USER_AGENT_CURL
export HOMEBREW_BOTTLE_DEFAULT_DOMAIN
export HOMEBREW_DEFAULT_PREFIX
export HOMEBREW_MACOS_ARM_DEFAULT_PREFIX

if [[ -n "$HOMEBREW_MACOS" && -x "/usr/bin/xcode-select" ]]
then
Expand Down
12 changes: 12 additions & 0 deletions Library/Homebrew/cmd/update.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,18 @@ git_init_if_necessary() {
trap - EXIT
fi

if [[ -n "${HOMEBREW_MACOS_VERSION}" ]] &&
[[ -z "$(git config --get homebrew.flavour 2>/dev/null)" ]]
then
if [[ "${HOMEBREW_PREFIX}" = "${HOMEBREW_MACOS_ARM_DEFAULT_PREFIX}" ]]; then
git config --replace-all homebrew.flavour "arm64" 2>/dev/null
elif [[ "${HOMEBREW_PREFIX}" = "${HOMEBREW_DEFAULT_PREFIX}" ]]; then
git config --replace-all homebrew.flavour "x86_64" 2>/dev/null
else
git config --replace-all homebrew.flavour "$(uname -m)" 2>/dev/null
fi
fi
Comment on lines +48 to +58
Copy link
Member

Choose a reason for hiding this comment

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

  • feels like this should be in brew.sh (or even install.rb) rather than update.sh
  • would be good to set a variable inside the if/elif to DRY up the repeated git config --replace-all homebrew.flavour
  • feels like this should be inferred based on previously/newly installed formulae rather than the current architecture on existing installs (to avoid a single Rosetta/non-Rosetta run setting these incorrectly)


[[ -d "$HOMEBREW_LIBRARY/Taps/homebrew/homebrew-core" ]] || return
safe_cd "$HOMEBREW_LIBRARY/Taps/homebrew/homebrew-core"
if [[ ! -d ".git" ]]
Expand Down
49 changes: 37 additions & 12 deletions Library/Homebrew/install.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ module Install
module_function

def perform_preinstall_checks(all_fatal: false, cc: nil)
check_prefix
check_flavour_matches_architecture
check_cpu
attempt_directory_creation
check_cc_argv(cc)
Expand All @@ -29,19 +29,44 @@ def perform_build_from_source_checks(all_fatal: false)
Diagnostic.checks(:build_from_source_checks, fatal: all_fatal)
end

def check_prefix
if Hardware::CPU.intel? && HOMEBREW_PREFIX.to_s == HOMEBREW_MACOS_ARM_DEFAULT_PREFIX
odie "Cannot install in Homebrew on Intel processor in ARM default prefix (#{HOMEBREW_PREFIX})!"
elsif Hardware::CPU.arm? && HOMEBREW_PREFIX.to_s == HOMEBREW_DEFAULT_PREFIX
odie <<~EOS
Cannot install in Homebrew on ARM processor in Intel default prefix (#{HOMEBREW_PREFIX})!
Please create a new installation in #{HOMEBREW_MACOS_ARM_DEFAULT_PREFIX} using one of the
"Alternative Installs" from:
#{Formatter.url("https://docs.brew.sh/Installation")}
You can migrate your previously installed formula list with:
brew bundle dump
def check_flavour_matches_architecture
homebrew_flavour = HOMEBREW_REPOSITORY.cd do
Utils.popen_read("git", "config", "--get", "homebrew.flavour").chomp.presence
Copy link
Member

Choose a reason for hiding this comment

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

Could consider passing this as a global variable, too, so it only needs read once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did that in my first draft, which:

[introduced] a read-only HOMEBREW_FLAVOUR environment variable that tells us under which architecture a given Homebrew installation can be run.

I removed it after your comment that said:

I'd like to avoid user configuration here until we feel it's needed from user issues/requests/confusion.

That environment variable wasn’t user configuration. It was read-only, would be set by Homebrew, ignoring whatever its initial value is. Its purpose was to pass it as a global variable so it only needs to be read once. I removed it anyway according to your request.

So you’d prefer me to add that variable back, right?

Copy link
Member

Choose a reason for hiding this comment

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

That environment variable wasn’t user configuration. It was read-only, would be set by Homebrew, ignoring whatever its initial value is. Its purpose was to pass it as a global variable so it only needs to be read once. I removed it anyway according to your request.

I didn't request you remove it, I requested we avoid it being user configurable as it wasn't clear from the proposal whether it would be or not.

So you’d prefer me to add that variable back, right?

Yes, I think it would be worth considering if it aligns with the fix for #9126 (comment)

#9126 (comment) is another global variable that should be passed from Bash into Ruby to avoid duplication if it's needed there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MikeMcQuaid Ok, thanks. Will look for the old commit, restore it and apply your suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

end

if !homebrew_flavour ||
(Hardware::CPU.intel? && homebrew_flavour == "x86_64") ||
(Hardware::CPU.arm? && homebrew_flavour == "arm64")
return
end

error_title = "This Homebrew installation in #{HOMEBREW_PREFIX} only works on #{homebrew_flavour} processors."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
error_title = "This Homebrew installation in #{HOMEBREW_PREFIX} only works on #{homebrew_flavour} processors."
error_title = "This Homebrew installation in #{HOMEBREW_PREFIX} only works on #{homebrew_flavour} processors!"

odie error_title unless Hardware::CPU.arm?

ohai "Checking whether Rosetta is installed"
rosetta_installed = quiet_system(
"/usr/bin/swift",
"#{HOMEBREW_REPOSITORY}/Library/Homebrew/utils/rosetta_installed.swift",
)
rosetta_instructions = if rosetta_installed
<<~EOS
To use it, you can:
- run your Terminal from Rosetta 2 or
- run 'arch -x86_64 brew' instead of 'brew'.
Comment on lines +53 to +55
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
To use it, you can:
- run your Terminal from Rosetta 2 or
- run 'arch -x86_64 brew' instead of 'brew'.
To use it, you can choose between:
- running your Terminal from Rosetta 2
- running 'arch -x86_64 brew' instead of 'brew'.

the inline or felt weird

EOS
else
"To use it, you need to install Rosetta 2 first."
end

odie <<~EOS
#{error_title}
#{rosetta_instructions}
Or create a new installation in #{HOMEBREW_MACOS_ARM_DEFAULT_PREFIX} using one of the
"Alternative Installs" from:
#{Formatter.url("https://docs.brew.sh/Installation")}
You can migrate your previously installed formula list with:
brew bundle dump
EOS
end

def check_cpu
Expand Down
4 changes: 4 additions & 0 deletions Library/Homebrew/utils/rosetta_installed.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
#!/usr/bin/swift

import CoreFoundation
exit(CFBundleIsArchitectureLoadable(CPU_TYPE_X86_64) ? 0 : 1)