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

os/mac/extend/ENV/super: handle nil sdk. #14784

Merged
merged 1 commit into from Feb 23, 2023

Conversation

MikeMcQuaid
Copy link
Member

I'm aware this is not meant to happen but: sometimes it does and the lack of handling produces a subpar error.

@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Feb 23, 2023
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

@Bo98
Copy link
Member

Bo98 commented Feb 23, 2023

It should be impossible for that diagnostic to pass if the SDK is nil. Where are you seeing this happen so I can reproduce?

The reason I'm concerned is if the diagnostic is missing this, then this kicks the issue down the road to be, for example, an unclear build error.

@Bo98
Copy link
Member

Bo98 commented Feb 23, 2023

Relevant check:

def check_if_supported_sdk_available
return unless DevelopmentTools.installed?
return unless MacOS.sdk_root_needed?
return if MacOS.sdk
locator = MacOS.sdk_locator
source = if locator.source == :clt
return if MacOS::CLT.below_minimum_version? # Handled by other diagnostics.
update_instructions = MacOS::CLT.update_instructions
"Command Line Tools (CLT)"
else
return if MacOS::Xcode.below_minimum_version? # Handled by other diagnostics.
update_instructions = MacOS::Xcode.update_instructions
"Xcode"
end
<<~EOS
Your #{source} does not support macOS #{MacOS.version}.
It is either outdated or was modified.
Please update your #{source} or delete it if no updates are available.
#{update_instructions}
EOS
end

@Bo98
Copy link
Member

Bo98 commented Feb 23, 2023

If you want do this anyway then can we do the nil check after the diagnostic check?

@MikeMcQuaid
Copy link
Member Author

@Bo98 clean brew doctor, still hitting this.

mikebook # brew reinstall testbottest --debug
/opt/homebrew/Library/Homebrew/brew.rb (Formulary::FormulaLoader): loading /opt/homebrew/Library/Taps/homebrew/homebrew-test-bot/Formula/testbottest.rb
==> Fetching homebrew/test-bot/testbottest
==> Downloading file:///opt/homebrew/Library/Taps/homebrew/homebrew-test-bot/Formula/tarballs/testbott
/usr/bin/env /opt/homebrew/Library/Homebrew/shims/shared/curl --disable --cookie /dev/null --globoff --show-error --user-agent Homebrew/4.0.3-99-g68859ae\ \(Macintosh\;\ arm64\ Mac\ OS\ X\ 13.2.1\)\ curl/7.86.0 --header Accept-Language:\ en --retry 3 --location --silent --head --request GET file:///opt/homebrew/Library/Taps/homebrew/homebrew-test-bot/Formula/tarballs/testbottest-0.1.tbz
Already downloaded: /Users/mike/Library/Caches/Homebrew/downloads/5e69b0b3b4db6c7bbe51550a583ff70a3744334b5f3e18b72100f8b414c07729--testbottest-0.1.tbz
==> Reinstalling homebrew/test-bot/testbottest 
/usr/bin/env /opt/homebrew/Library/Homebrew/shims/shared/git --version
Error: An exception occurred within a child process:
  NoMethodError: undefined method `path' for nil:NilClass
/opt/homebrew/Library/Homebrew/extend/os/mac/extend/ENV/super.rb:95:in `setup_build_environment'
/opt/homebrew/Library/Homebrew/build.rb:83:in `install'
/opt/homebrew/Library/Homebrew/build.rb:229:in `<main>'
mikebook # brew config               (master) [/opt/homebrew/Library/Taps/homebrew/homebrew-test-bot]
HOMEBREW_VERSION: 4.0.3-99-g68859ae
ORIGIN: https://github.com/Homebrew/brew
HEAD: 68859ae256ac8a97fd9b76421b949dde05afd542
Last commit: 2 hours ago
Core tap origin: https://github.com/Homebrew/homebrew-core
Core tap HEAD: 47ade94bb5b2a55a174365cb1f2658461d66801e
Core tap last commit: 26 hours ago
Core tap branch: master
Core tap JSON: 22 Feb 15:24 UTC
HOMEBREW_PREFIX: /opt/homebrew
HOMEBREW_AUTOREMOVE: set
HOMEBREW_BAT_THEME: ansi
HOMEBREW_BOOTSNAP: set
HOMEBREW_CASK_OPTS: []
HOMEBREW_DEVELOPER: set
HOMEBREW_EDITOR: code
HOMEBREW_GITHUB_API_TOKEN: set
HOMEBREW_MAKE_JOBS: 10
HOMEBREW_NO_ENV_HINTS: set
Homebrew Ruby: 2.6.10 => /System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/bin/ruby
CPU: 10-core 64-bit arm_firestorm_icestorm
Clang: 14.0.0 build 1400
Git: 2.39.2 => /opt/homebrew/bin/git
Curl: 7.86.0 => /usr/bin/curl
macOS: 13.2.1-arm64
CLT: 14.2.0.0.1.1668646533
Xcode: N/A
Rosetta 2: false
mikebook # brew doctor
Please note that these warnings are just used to help the Homebrew maintainers
with debugging if you file an issue. If everything you use Homebrew for is
working fine: please don't worry or file an issue; just ignore this. Thanks!

Warning: Some taps are not on the default git origin branch and may not receive
updates. If this is a surprise to you, check out the default branch with:
  git -C $(brew --repo homebrew/formula-analytics) checkout master
mikebook # git -C $(brew --repo homebrew/formula-analytics) checkout master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
mikebook # brew doctor
Your system is ready to brew.

I'm aware this is not meant to happen but: sometimes it does and the
lack of handling produces a subpar error.
@MikeMcQuaid
Copy link
Member Author

If you want do this anyway then can we do the nil check after the diagnostic check?

@Bo98 Yup, good call. Done that now, it'll skip setting HOMEBREW_SDKROOT if it's nil.

Agreed that fixing the diagnostic would be ideal. I'd still like to land this because it's not great to assume something be never nil that is sometimes nil.

@MikeMcQuaid
Copy link
Member Author

@Bo98 Yeh, the issue is with an depends_on :xcode => :optional where MacOS.sdk_for_formula will return a nil sdk without failing because of Xcode. There's various ways it could be handled but it feels like either:

  • MacOS.sdk_for_formula should be adapted so it never returns nil
  • MacOS.sdk_for_formula's value should only be used when it's not nil

@Bo98
Copy link
Member

Bo98 commented Feb 23, 2023

depends_on :xcode => :optional

Very unusual requirement that. Will take a look, thanks!

@MikeMcQuaid MikeMcQuaid merged commit 7d7e494 into Homebrew:master Feb 23, 2023
@MikeMcQuaid MikeMcQuaid deleted the super_sdk_nil branch February 23, 2023 16:10
@github-actions github-actions bot added the outdated PR was locked due to age label Mar 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 26, 2023
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.

None yet

3 participants