Fix missing Homebrew prefix resolution. #230
Conversation
utils/make-pkgconfig.swift
Outdated
| @@ -46,24 +46,21 @@ extension String: Error { | |||
| } | |||
|
|
|||
| func makeFile() throws { | |||
| let pkgConfigPath = "/usr/local/lib/pkgconfig" | |||
| guard let brew = which("brew") else { throw "Missing callable `brew` instance." } | |||
There was a problem hiding this comment.
We cannot assume the LLVM installation was done via brew because that breaks Linux users who didn't use brew to install (or Mac users who didn't use brew for that matter). This is why the CI is failing.
There was a problem hiding this comment.
I see.
I'm going to add platform branching. (#available)
For the Linux, I don't know well about it. Just keep /usr/local/ for Linux with hoping some Linux expert to come up to fix it.
For the macOS, I think /usr/local can cover only x86 Homebrew at default location. (maybe some other package managers?) How about calling brew --prefix first and falling back to /usr/local?
There was a problem hiding this comment.
I'm not sure how useful #available will be since brew is now available for Linux I think, so in theory someone could install it that way.
As for trying brew first then falling back to /usr/local, that's probably fine. @segiddins @dduan what do you think? Could still be a problem for those who have brew installed but installed it via some other means. Not sure how much we care about that.
There was a problem hiding this comment.
I agree to that #available is not necessary.
utils/make-pkgconfig.swift
Outdated
| guard let brewPrefix = run(brew, args: ["--prefix"]) else { return nil } | ||
| return which(brewPrefix + "/opt/llvm/bin/llvm-config") | ||
| } | ||
| let brewLLVMConfig = which(brewPrefix + "/opt/llvm/bin/llvm-config") |
There was a problem hiding this comment.
Let's keep this in a closure since it only needs to be executed if the other 2 which calls on line 63 returned nil.
|
@matthewseaman I pushed new commit. Thanks. |
There was a problem hiding this comment.
Looks good to me, though @segiddins or @dduan should take a look before merging to make sure this is fine.
|
Okay |
|
@eonil Thanks for this! |
Make package making script to work with arbitrary (non-default) Homebrew locations.