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

mac/xcode: compare with existing Version class. #3422

Merged
merged 7 commits into from Nov 5, 2017

Conversation

Projects
None yet
5 participants
@MikeMcQuaid
Member

MikeMcQuaid commented Nov 5, 2017

Additionally, return null versions when it makes sense to do so. This means that comparisons on the Xcode/CLT version do not need to be guarded on whether Xcode/CLT is installed.

Also, while we're here do the same thing for mac/xquartz, move MACOS_*VERSION to compat and simplify and fix up code accordingly for these changes.

Requested by @ilovezfs and @DomT4 in Homebrew/homebrew-core#20059

MikeMcQuaid added some commits Nov 5, 2017

mac/xcode: compare with existing Version class.
Additionally, return null versions when it makes sense to do so. This
means that comparisons on the Xcode/CLT version do not need to be
guarded on whether Xcode/CLT is installed.
version: support to_i.
This is needed for existing MacOS::Xcode.version calls that relied on
this returning a string. It mirrors similar behaviour for to_f.

@MikeMcQuaid MikeMcQuaid merged commit ffe523a into Homebrew:master Nov 5, 2017

1 of 3 checks passed

codecov/patch 56% of diff hit (target 69.22%)
Details
codecov/project 69.22% (-0.01%) compared to 5bf0584
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MikeMcQuaid MikeMcQuaid deleted the MikeMcQuaid:null-versions branch Nov 5, 2017

@agargiulo

This comment has been minimized.

Show comment
Hide comment
@agargiulo

agargiulo Nov 5, 2017

With this PR included, Brew breaks down into infinite recursion for me.
I think it's related to using a variable version and a method also named version

The following goes on for 100,000+ lines

/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:193:in `installed?'
/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:115:in `detect_version'
/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:105:in `version'
/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:275:in `detect_version'
/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:265:in `version'
/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:193:in `installed?'
/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:115:in `detect_version'
/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:105:in `version'
/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:275:in `detect_version'
/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:265:in `version'
/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:193:in `installed?'
/usr/local/Homebrew/Library/Homebrew/extend/os/mac/development_tools.rb:22:in `installed?'
/usr/local/Homebrew/Library/Homebrew/cmd/install.rb:108:in `install'
/usr/local/Homebrew/Library/Homebrew/brew.rb:100:in `<main>'

if I do this, then the stack issues vanish:

export HOMEBREW_NO_AUTO_UPDATE=1
pushd $(brew --repo)
git checkout HEAD^ # ( Commit bef4213f )
popd
brew install whatever

Brew Config:

[12:05:54] % brew config
HOMEBREW_VERSION: 1.3.6-181-gbef4213
ORIGIN: https://github.com/Homebrew/brew
HEAD: bef4213ffc2d52ca38a83c24178c3c7f86273eb9
Last commit: 4 hours ago
Core tap ORIGIN: https://github.com/Homebrew/homebrew-core
Core tap HEAD: 0c79d86080b560e7d29ac71840d86705bd602453
Core tap last commit: 76 minutes ago
HOMEBREW_PREFIX: /usr/local
HOMEBREW_AUTO_UPDATE_SECS: 300
HOMEBREW_DEV_CMD_RUN: 1
HOMEBREW_GITHUB_API_TOKEN: set
HOMEBREW_GIT_CONFIG_FILE: /usr/local/Homebrew/.git/config
HOMEBREW_NO_AUTO_UPDATE: 1
CPU: quad-core 64-bit kabylake
Homebrew Ruby: 2.3.3 => /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.3/bin/ruby
Clang: 9.0 build 900
Git: 2.15.0 => /usr/local/bin/git
Curl: 7.54.0 => /usr/bin/curl
Perl: /usr/local/bin/perl => /usr/local/Cellar/perl/5.26.1/bin/perl
Python: /usr/local/opt/python/libexec/bin/python => /usr/local/Cellar/python/2.7.14/Frameworks/Python.framework/Versions/2.7/bin/python2.7
Ruby: /Users/agargiulo/.rbenv/shims/ruby => /Users/agargiulo/.rbenv/versions/2.4.2/bin/ruby
Java: 1.8.0_144
macOS: 10.13-x86_64
Xcode: N/A
CLT: 9.0.1.0.1.1506734476
X11: N/A

agargiulo commented Nov 5, 2017

With this PR included, Brew breaks down into infinite recursion for me.
I think it's related to using a variable version and a method also named version

The following goes on for 100,000+ lines

/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:193:in `installed?'
/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:115:in `detect_version'
/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:105:in `version'
/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:275:in `detect_version'
/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:265:in `version'
/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:193:in `installed?'
/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:115:in `detect_version'
/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:105:in `version'
/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:275:in `detect_version'
/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:265:in `version'
/usr/local/Homebrew/Library/Homebrew/os/mac/xcode.rb:193:in `installed?'
/usr/local/Homebrew/Library/Homebrew/extend/os/mac/development_tools.rb:22:in `installed?'
/usr/local/Homebrew/Library/Homebrew/cmd/install.rb:108:in `install'
/usr/local/Homebrew/Library/Homebrew/brew.rb:100:in `<main>'

if I do this, then the stack issues vanish:

export HOMEBREW_NO_AUTO_UPDATE=1
pushd $(brew --repo)
git checkout HEAD^ # ( Commit bef4213f )
popd
brew install whatever

Brew Config:

[12:05:54] % brew config
HOMEBREW_VERSION: 1.3.6-181-gbef4213
ORIGIN: https://github.com/Homebrew/brew
HEAD: bef4213ffc2d52ca38a83c24178c3c7f86273eb9
Last commit: 4 hours ago
Core tap ORIGIN: https://github.com/Homebrew/homebrew-core
Core tap HEAD: 0c79d86080b560e7d29ac71840d86705bd602453
Core tap last commit: 76 minutes ago
HOMEBREW_PREFIX: /usr/local
HOMEBREW_AUTO_UPDATE_SECS: 300
HOMEBREW_DEV_CMD_RUN: 1
HOMEBREW_GITHUB_API_TOKEN: set
HOMEBREW_GIT_CONFIG_FILE: /usr/local/Homebrew/.git/config
HOMEBREW_NO_AUTO_UPDATE: 1
CPU: quad-core 64-bit kabylake
Homebrew Ruby: 2.3.3 => /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.3/bin/ruby
Clang: 9.0 build 900
Git: 2.15.0 => /usr/local/bin/git
Curl: 7.54.0 => /usr/bin/curl
Perl: /usr/local/bin/perl => /usr/local/Cellar/perl/5.26.1/bin/perl
Python: /usr/local/opt/python/libexec/bin/python => /usr/local/Cellar/python/2.7.14/Frameworks/Python.framework/Versions/2.7/bin/python2.7
Ruby: /Users/agargiulo/.rbenv/shims/ruby => /Users/agargiulo/.rbenv/versions/2.4.2/bin/ruby
Java: 1.8.0_144
macOS: 10.13-x86_64
Xcode: N/A
CLT: 9.0.1.0.1.1506734476
X11: N/A
@dpoggi

This comment has been minimized.

Show comment
Hide comment
@dpoggi

dpoggi Nov 5, 2017

I'm in the middle of opening an issue for it. Beat me to the punch 😄

dpoggi commented Nov 5, 2017

I'm in the middle of opening an issue for it. Beat me to the punch 😄

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Nov 5, 2017

Contributor

do you have a concrete example of how to trigger it

Contributor

ilovezfs commented Nov 5, 2017

do you have a concrete example of how to trigger it

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Nov 5, 2017

Member

I'm looking into this as we speak.

Member

MikeMcQuaid commented Nov 5, 2017

I'm looking into this as we speak.

@dpoggi

This comment has been minimized.

Show comment
Hide comment
@dpoggi

dpoggi Nov 5, 2017

It happens for people with just the command-line tools and no Xcode.

https://github.com/Homebrew/brew/blob/master/Library/Homebrew/extend/os/mac/development_tools.rb#L22

The call to MacOS::Xcode.installed? finishes successfully (returns false), the call to MacOS::CLT.installed? is going to infinite stack depth.

dpoggi commented Nov 5, 2017

It happens for people with just the command-line tools and no Xcode.

https://github.com/Homebrew/brew/blob/master/Library/Homebrew/extend/os/mac/development_tools.rb#L22

The call to MacOS::Xcode.installed? finishes successfully (returns false), the call to MacOS::CLT.installed? is going to infinite stack depth.

@agargiulo

This comment has been minimized.

Show comment
Hide comment
@agargiulo

agargiulo Nov 5, 2017

Nearly any brew command I ran while checked out to commit ffe523a
brew info doesn't care but brew install brew config brew doctor brew upgrade all errored with the giant trace

agargiulo commented Nov 5, 2017

Nearly any brew command I ran while checked out to commit ffe523a
brew info doesn't care but brew install brew config brew doctor brew upgrade all errored with the giant trace

@agargiulo

This comment has been minimized.

Show comment
Hide comment
@agargiulo

agargiulo Nov 5, 2017

Ah yeah that sounds like me. CLT no xcode

agargiulo commented Nov 5, 2017

Ah yeah that sounds like me. CLT no xcode

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Nov 5, 2017

Contributor

Yup that does the trick. Thanks.

Contributor

ilovezfs commented Nov 5, 2017

Yup that does the trick. Thanks.

MikeMcQuaid added a commit that referenced this pull request Nov 5, 2017

xcode: check Xcode installed before using version.
This avoids infinite recursion described in:
#3422 (comment)
@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Nov 5, 2017

Member

@agargiulo @dpoggi Please brew update and try again, thanks.

Member

MikeMcQuaid commented Nov 5, 2017

@agargiulo @dpoggi Please brew update and try again, thanks.

@agargiulo

This comment has been minimized.

Show comment
Hide comment
@agargiulo

agargiulo Nov 5, 2017

All good now, thank you for the quick replies 😁

agargiulo commented Nov 5, 2017

All good now, thank you for the quick replies 😁

@dpoggi

This comment has been minimized.

Show comment
Hide comment
@dpoggi

dpoggi Nov 5, 2017

All smiles here 👍

Thanks @MikeMcQuaid!

dpoggi commented Nov 5, 2017

All smiles here 👍

Thanks @MikeMcQuaid!

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Nov 5, 2017

Member

Thanks for the quick response, sorry for the pain!

Member

MikeMcQuaid commented Nov 5, 2017

Thanks for the quick response, sorry for the pain!

@DomT4

This comment has been minimized.

Show comment
Hide comment
@DomT4

DomT4 Nov 7, 2017

Contributor

A belated thanks for this @MikeMcQuaid ❤️. It's a significant enough change that to be honest when I got the ping I thought you'd opened an issue for discussion or such, but nope, you'd fixed it 😸.

Contributor

DomT4 commented Nov 7, 2017

A belated thanks for this @MikeMcQuaid ❤️. It's a significant enough change that to be honest when I got the ping I thought you'd opened an issue for discussion or such, but nope, you'd fixed it 😸.

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Nov 10, 2017

Member

@DomT4 My pleasure!

Member

MikeMcQuaid commented Nov 10, 2017

@DomT4 My pleasure!

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.