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

Fixes #10551 Search for LLVM 17 during initial setup #10552

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

KilianB
Copy link
Contributor

@KilianB KilianB commented Apr 26, 2024

What does this PR do?

  • Update build script to not failed if LLVM - 17 isn't found.
  • Docs are updated to point for contributors

Fixes #10551

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • [x ] Code changes

How did you verify your code works?

After changes build succeedes locally. Beforehand it did not.

@TiBianMod
Copy link

@paperdave made this changes before 2-3 days on this PR #10492

@KilianB
Copy link
Contributor Author

KilianB commented Apr 26, 2024

In that case the build.sh was missed. :-)

- CC=${CC:-$(which clang-17 || which clang || which cc)}
+ CC=${CC:-$(which clang-16 || which clang || which cc)}
- CXX=${CXX:-$(which clang++-17 || which clang++ || which c++)}
+ CXX=${CXX:-$(which clang++-16 || which clang++ || which c++)}

@paperclover
Copy link
Member

ah, i missed setup.sh

image

we should use the version of LLVM set in the variable lol

Copy link
Member

@paperclover paperclover left a comment

Choose a reason for hiding this comment

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

can you switch it all to LLVM 16. we recently did an upgrade but had issues so we downgraded. the setup.sh is the one that was missed during the downgrade

@KilianB KilianB force-pushed the fix_build_setup_llvm_17 branch from e505e85 to 7583aa1 Compare April 26, 2024 21:56
@KilianB
Copy link
Contributor Author

KilianB commented Apr 26, 2024

@paperdave done. There were some additional changes in env.sh that might have been missed as well. In the commit Jarred also added CMAKE flags.

I am not well versed of the impact of those, should all these changes in the file be reverted?

https://github.com/oven-sh/bun/pull/10161/files#diff-5a4957bc7903de70fcfb50dc6aa8851f9d16d84566568736af52e9e2df8c8628

@paperclover
Copy link
Member

no, i think those flags fixed a linux crash

@paperclover paperclover merged commit a6d39e1 into oven-sh:main Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bun setup fails due to LLVM mismatch. LLVM 16 is required. Detected cc as /usr/bin/cc
3 participants