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

nvc: go back to using default llvm as dependency #71444

Closed
wants to merge 1 commit into from

Conversation

mitchblank
Copy link
Contributor

The restriction on building against llvm@6 (later updated to llvm@7) was introduced by @jnozsc (#54741) However the issue that was referenced was fixed before 1.5.0 was released (nickg/nvc@f48c651) so when homebrew updated to 1.5.0 (#58612) this restriction should probably have been relaxed more than it was.

According to the project's README it is tested with llvm {7,8,9} so at a minimum we should probably bump its dependency to llvm@9. However, this wouldn't be enough to fix the Big Sur bottling issue.

Nothing in the project's build instructions suggests that llvm versions newer than 9.x are to be avoided these days so just switching to our default llvm (currently 11.1) instead. Seems to build and test fine.

cc @nickg, @chenrui333

The restriction on building against llvm@6 (later updated to llvm@7)
was introduced by @jnozsc (Homebrew#54741)  However the issue that was
referenced was fixed before 1.5.0 was released...
nickg/nvc@f48c651
...so when homebrew updated to 1.5.0 this restriction should probably
be dropped.

According to the project's README it is tested with llvm {7,8,9} so
at a minimum we should probably bump its dependency to llvm@9.  However,
this wouldn't be enough to fix the Big Sur bottling issue.

Nothing in the project's build instructions suggests that llvm versions
newer than 9.x are to be avoided these days so just switching to our
default llvm (currently 11.1) instead.  Seems to build and test fine.
@nickg
Copy link

nickg commented Feb 19, 2021

Nothing in the project's build instructions suggests that llvm versions newer than 9.x are to be avoided these days so just switching to our default llvm (currently 11.1) instead. Seems to build and test fine.

I've tested with 11 and it works fine. Just haven't updated the README.

@SMillerDev
Copy link
Member

Does it not work with the macOS provided LLVM?

@mitchblank
Copy link
Contributor Author

Oh, I didn't realize that we had any formula that used the system llvm (looks like only git-delta and vlmcsd are doing this right now, at least as an explicitly declared uses_from_macos dependency) I can give it a try.

@SMillerDev
Copy link
Member

Most of them don't declare it explicitly unless it can't be gcc. (Which would be the default on Linux)

@Bo98
Copy link
Member

Bo98 commented Feb 20, 2021

If it requires stuff like libLLVM then system LLVM won't work. The use of llvm-config suggests to me that may be the case (system LLVM does not have llvm-config).

For things that only need libclang, system LLVM can work - but usually requires extra steps in the build system to find it (since there's no clang-config).

@nickg
Copy link

nickg commented Feb 20, 2021

If it requires stuff like libLLVM then system LLVM won't work. The use of llvm-config suggests to me that may be the case (system LLVM does not have llvm-config).

It uses libLLVM for code generation so won't work without that.

@mitchblank
Copy link
Contributor Author

Yeah, it doesn't seem like llvm API is exposed in any of the SDKs at all, e.g. find /Applications/Xcode.app -name ExecutionEngine.h returns zero results.

@SMillerDev -- could you explain what you want more clearly. Just so we're on the same page, the llvm dependency is not due to the project preferring clang over gcc; this is a project that builds on top of the LLVM compiler framework itself. It doesn't seem that's possible unless we take the llvm formula dependency.

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Thanks, @mitchblank.

@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Mar 24, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Mar 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants