Search for llvm-config on OSX using brew #3247
Conversation
|
I think your indentation is off. Are you short-circuiting the |
|
@jc00ke Yes, thanks, fixed. |
| elsif @darwin | ||
| if `command -v brew`.chomp != "" | ||
| if `brew list --versions llvm`.chomp != "" | ||
| config = `brew list llvm | grep '/llvm-config$'`.chomp |
There was a problem hiding this comment.
@joelparkerhenderson In theory, config may still equal "" (if brew list llvm doesn't output anything that matches the regex). So why not remove the two ifs above, call this regardless, and then handle the ""? Or better yet, check $?.success? instead.
There was a problem hiding this comment.
Good, I'll do your suggestions.
|
@joelparkerhenderson You should also squash the initial commit + the indentation fix into a single commit so as to not to pollute the Git history. |
|
Personally I oppose adding a patch like this as it's a slippery slope down a path where we'll end up with dozens of specific version/command checks, scripts to check for package managers, etc, etc. What happens if somebody is using Macports/Fink/ which in turn has its own ways of naming things? Do we also add checks for all that? What if this then varies between package manager and/or OS versions? I feel this sort of logic really belongs in Ruby version managers, not in Rubinius itself. Users can already specify an alternative |
|
As an extra note: |
|
I'm also not a fan. I think the detection should happen in ps: @yorickpeterse I've been wanting to refactor |
On Mac OSX, this command fails: `ruby-install rbx`. The reason is that the OSX LLVM does not show up as expected. Many OSX developers install an additional LLVM by using the `brew` package manager: `brew install llvm`. The `brew` manager does not replace the existing system LLVM because that would cause problems with other OSX software, for example XCode. Any approach that uses `brew link` is potentially dangerous on OSX. Therefore, this patch adds a search for llvm-config: if the system is Darwin, and the system says that `brew` is installed, and `brew` says the formula for LLVM is installed, then the code searches for the `llvm-config` file. I expect this code will solve the `ruby-install rbx` issue for the majority of OSX brew users. If this code is accepted, then subsequent improvements could add logging, advice, and corner cases such as multiple brew LLVM installations.
| end | ||
| else | ||
| @log.write "not found" | ||
| end |
There was a problem hiding this comment.
@joelparkerhenderson lol, you did the polar opposite of what I suggested, I meant:
elsif @darwin
out = `brew list llvm | grep '/llvm-config$'` # regardless of whether `brew` or the llvm formula are present
config = out.chomp if $?.success?
else
# ...There was a problem hiding this comment.
Ha! Your way is definitely terser - I like it. When I read more of the configure code I saw a bunch of logging that shows whether pieces are found or not, so I then tried to use the existing code's approach. Seems potentially useful. Any implementation is fine with me - I just want it to work for my team.
|
@jc00ke adding OS specific workarounds to ruby-install are expressly forbidden. I don't want Rubinius to become dependent on ruby-install. We can workaround this homebrew issue via |
Yes. The existing configure code has these. IMHO it's a cost/benefit tradeoff. Complexity can be borne once in code, or borne by every person who hits the issue and then has to figure out how to solve it.
The troubleshooting cost is high and is O(N) whereas the patch cost is low and O(1). |
|
Generally, an application doesn't take care of installing dependencies. Indeed, we don't install any other dependencies. At the same time, I'm forced to vendor zlib to be able to run psych on Heroku to install gems. So, sometimes we have to make exceptions. Since LLVM is so important, we probably need to make an exception. I'd prefer a patch that identifies the platform, discovers the package manager, and checks for the package. This will be fairly easy to extend in the future. Right now, ruby-install does a pretty nice job of that. We can probably crib some code. There's no point in refactoring configure unless someone wants to write it in shell. I'm removing the Ruby dependency to build so that we can make it easier to package Rubinius. The best way to avoid all these problems is to get Rubinius into the major package systems. The easiest way for anyone to test out Rubinius will be to install the package. They can easily uninstall if they don't like it. Given a typical package system (even on brew), Rubinius would conflict with MRI but that temporary conflict is resolved by uninstalling. We (collectively) have been pushing the Ruby switchers so much, but the typical user is not going to be using multiple Rubies. If anyone wants to help move Rubinius adoption ahead significantly, help us package it for the major distros. |
|
@joelparkerhenderson thanks for working on the patch. I applied @Roman2K's because it was simple and provides an easy way to extend the conditional for other platforms if needed. |
On Mac OSX, this command fails:
ruby-install rbx.The reason is that the OSX LLVM does not show up as expected.
Many OSX developers install an additional LLVM by using the
brewpackage manager:brew install llvm.The
brewmanager does not replace the existing system LLVM because that would cause problems with other OSX software, for example XCode. Any approach that usesbrew linkis potentially dangerous on OSX.Therefore, this patch adds a search for llvm-config: if the system is Darwin, and the system says that
brewis installed, andbrewsays the formula for LLVM is installed, then the code searches for thellvm-configfile.I expect this code will solve the
ruby-install rbxissue for the majority of OSX brew users.If this code is accepted, then subsequent improvements could add logging, advice, and corner cases such as multiple brew LLVM installations.