Skip to content
This repository was archived by the owner on Jan 16, 2025. It is now read-only.

Fix #13012 properly (recursion stack overfl.) #13037

Closed
wants to merge 1 commit into from

Conversation

samueljohn
Copy link
Contributor

Fix #13012 properly and don't set the SDK if CLT

Undoing parts of the hot fix 78b9e85.

The only thing missing was to check for system "/usr/bin/xcrun -find make 1>/dev/null 2>&1"
and then it's safe to call locate.

This commit restores the original functionality but without the risk for recursion
and improves the logic of MacOS.locate. See below.

To important changes in this commit:

  • For Xcode and CLT: don't add the SDK and leave things as before.
    So if MacOS.clt_installed?, then no SDKROOT and -L and -I
    directories are set in ENV.macosxsdk.
  • Improved the logic for MacOS.locate for Xcode-only situations
    by assuring that the xcode-select path is correct. This is done
    by checking that bin/make exists and is executable. Otherwise it
    was possible to set xcode-select to an empty dir.
    This check is done in MacOS.sdk_path too.
    We are now able to use Xcode wherever it is and can work even, if
    xcode-select is set to invalid values. (Remember some users don't
    have sudo access and that is needed to fix xcode-select).

Some minor whitespace fixes.
Minor backtick fix in doctor.rb's printout.

@@ -423,7 +428,7 @@ def xcode_prefix
@xcode_prefix ||= begin
path = `/usr/bin/xcode-select -print-path 2>/dev/null`.chomp
path = Pathname.new path
if $?.success? and path.directory? and path.absolute?
if $?.success? and path.directory? and path.absolute? and !xctools_fucked?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

... we cannot trust xcode-select, so check additionally if xctools_fucked.

@samueljohn
Copy link
Contributor Author

I incorporated @MikeMcQuaid suggestions from the first version of this patch.
The functionality is as initially intended but without the possibility of a recursion.

ping @mistydemeo.

@samueljohn
Copy link
Contributor Author

commit message updated. @MikeMcQuaid, alright?

@MikeMcQuaid
Copy link
Member

That's cool, thanks.

@@ -299,28 +299,28 @@ def locate tool

if File.executable? "/usr/bin/#{tool}"
path = Pathname.new "/usr/bin/#{tool}"
elsif not MacOS.xctools_fucked? and system "/usr/bin/xcrun -find #{tool} 1>/dev/null 2>&1"
elsif not MacOS.xctools_fucked? and system "/usr/bin/xcrun -find #{tool} &>/dev/null"
Copy link
Contributor

Choose a reason for hiding this comment

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

&> is a bashism, and while bash is the standard shell on OS X for now, it may not always be, so we should try to use portable constructs.

Copy link
Member

Choose a reason for hiding this comment

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

If they change the default from Bash it will be to ZSH which also supports this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps, but there's really no reason to avoid the portable idiom here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, @jacknagel what is the suggested portable idiom?
(we should think of the guys trying to port homebrew to linux and such)

Copy link
Contributor

Choose a reason for hiding this comment

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

1>/dev/null 2>&1 (as it was before) is fine.

@@ -210,7 +210,7 @@ def check_for_broken_symlinks
end
end
unless broken_symlinks.empty? then <<-EOS.undent
Broken symlinks were found. Remove them with `brew prune':
Broken symlinks were found. Remove them with `brew prune`:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is just a small thing but to be consistent with other places and with the github markdown style.

@samueljohn
Copy link
Contributor Author

@mistydemeo @jacknagel @MikeMcQuaid and @adamv I urge you to please, please let this into homebrew ASAP. Feel free to change minor issues yourself. We have some reports about postgresql, graphviz, valgrind and some other fail if the SDK from Xcode is in the "-L" path. This commit fixes that, too.

Undoing parts of the hot fix 78b9e85.

The only thing missing was to check for `system "/usr/bin/xcrun -find make 1>/dev/null 2>&1"`
and then it's safe to call locate.

This commit restores the original functionality but without the risk for recursion
and improves the logic of `MacOS.locate`. See below.

To important changes in this commit:

- For Xcode _and_ CLT: don't add the SDK and leave things as before.
So if `MacOS.clt_installed?`, then no `SDKROOT` and `-L` and `-I`
directories are set in `ENV.macosxsdk`.

- Improved the logic for `MacOS.locate` for Xcode-only situations
by assuring that the xcode-select path is correct. This is done
by checking that `bin/make` exists and is executable. Otherwise it
was possible to set xcode-select to an empty dir.
This check is done in `MacOS.sdk_path` too.
We are now able to use Xcode wherever it is and can work even, if
xcode-select is set to invalid values. (Remember some users don't
have sudo access and that is needed to fix xcode-select).

Some minor whitespace fixes.
Minor backtick fix in doctor.rb's printout.
@adamv
Copy link
Contributor

adamv commented Jun 27, 2012

I'm going to pull this. Shit is broken.

@mistydemeo
Copy link
Contributor

@adamv Go ahead, it looks like @samueljohn just updated this to fix the remaining issue.

@adamv
Copy link
Contributor

adamv commented Jun 27, 2012

Pulled.

@adamv adamv closed this Jun 27, 2012
# Since we are pretty unrelenting in finding Xcode no matter where
# it hides, we can now throw in the towel.
opoo "You really should consult the `brew doctor`!"
""
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't cause a problem now because we only use dev_tools_path as a string, but returning a different type of object in this case could theoretically cause problems in the future.

If we're not returning a path at all, maybe this should be a showstopper that raises an exception, rather than returning an empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I wanted brew list and other still to function even if no compiler/Xcode or whatever is found since raising an exception may be too harsh.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could add a DevToolsMissingError exception and just ignore it in the appropriate places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds like a clean solution. I am not the best person to
implement that, however. I am still new to homebrew after all.

Dipl.-Inform. Samuel John


PhD student,
Neuroinformatics Group, Faculty
of Technology, D33594 Bielefeld

+49 (0)1577 573 0 572
[email protected]
[email protected]
jabber: [email protected]


Am 28.06.2012 um 07:21 schrieb Jack Nagel
[email protected]:

elsif File.exist? "#{xcode_prefix}/usr/bin/make"
  # cc stopped existing with Xcode 4.3, there are c89 and c99 options though
  Pathname.new "#{xcode_prefix}/usr/bin"
else
  •  # yes this seems dumb, but we can't throw because the existance of
    
  •  # dev tools is not mandatory for installing formula. Eventually we
    
  •  # should make formula specify if they need dev tools or not.
    
  •  Pathname.new "/usr/bin"
    
  •  # Since we are pretty unrelenting in finding Xcode no matter where
    
  •  # it hides, we can now throw in the towel.
    
  •  opoo "You really should consult the `brew doctor`!"
    
  •  ""
    

We could add an DevToolsMissingError exception and just ignore it in the appropriate places.


Reply to this email directly or view it on GitHub:
https://github.com/mxcl/homebrew/pull/13037/files#r1065757

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll look into it.

We could actually call the appropriate doctor check directly and print that as part of the exception message.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the best bet here would be to return nil, and then do this where necessary:

raise DeveloperToolsNotFoundError if MacOS.dev_tools_path.nil?

and have DeveloperToolsNotFoundError call the right doctor check and print the output. I'll work up a patch today or tomorrow.

@adamv adamv mentioned this pull request Jun 27, 2012
@Homebrew Homebrew locked and limited conversation to collaborators Feb 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stack overflow error
5 participants