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

treewide: drop LLVM11 #283591

Merged
2 commits merged into from Mar 19, 2024
Merged

treewide: drop LLVM11 #283591

2 commits merged into from Mar 19, 2024

Conversation

ghost
Copy link

@ghost ghost commented Jan 24, 2024

Description of changes

unpin llvm11 from freshbootstraptools and drop LLVM11. i have a PR in review to update the bootstrap-tools for darwin which should be merged soonish #295557

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ghost ghost requested review from K900, toonn and reckenrode January 24, 2024 20:09
@ghost ghost requested a review from RaitoBezarius as a code owner January 24, 2024 20:09
@ghost
Copy link
Author

ghost commented Jan 24, 2024

as per @reckenrode

Bootstrap tools should probably be the minimally supported version of LLVM to prevent accidentally depending on the bootstrap version of LLVM matching the version in the stdenv.

so that will need to be changed from llvmPackages_16.

@reckenrode
Copy link
Contributor

To elaborate on that a bit, I want to avoid assumptions about the bootstrap compiler lest it cause trouble for future LLVM version updates for Darwin. In theory, it only matters if the bootstrap tools are ever updated, and the breakage would have to happen between now and when the switch is made to LLVM 18., so the risk is low. It may not be worth changing the version if it creates more work. (Assuming the SDK issue with libc++ can be addressed.)

@elliottslaughter
Copy link
Contributor

Please be aware that if you remove LLVM 11, you will need to disable Terra on Linux ARM due to this outstanding issue: terralang/terra#597

On every system other than Linux ARM, Terra can go up to LLVM 17 if needed, but the Linux ARM bug keeps that platform on LLVM 11.

@ghost
Copy link
Author

ghost commented Jan 24, 2024

Please be aware that if you remove LLVM 11, you will need to disable Terra on Linux ARM due to this outstanding issue: terralang/terra#597

On every system other than Linux ARM, Terra can go up to LLVM 17 if needed, but the Linux ARM bug keeps that platform on LLVM 11.

the build fails when trying to compile with an llvm above 13. eg, with llvm16
CMake Error at src/CMakeLists.txt:229 (target_link_libraries):
  Target "TerraLibraryShared" links to:
FFI::ffi

but the target was not found. Possible reasons include:

* There is a typo in the target name.
* A find_package call is missing for an IMPORTED target.
* An ALIAS target is missing.

CMake Error at src/CMakeLists.txt:345 (target_link_libraries):
Target "TerraExecutable" links to:

FFI::ffi

but the target was not found. Possible reasons include:

* There is a typo in the target name.
* A find_package call is missing for an IMPORTED target.
* An ALIAS target is missing.

@elliottslaughter
Copy link
Contributor

@a-n-n-a-l-e-e let me know if I should move this discussion to a different thread; until then I'll just reply here.

That looks like a missing dependency. In general, Terra only relies on a minimal LLVM build. (For our builds, I usually turn off all the optional build flags.) It's fine if you built LLVM with optional dependencies but then that means you'll have to add those dependencies to Terra.

I'm not an expert on what this FFI is but I've noticed that in many distros, the set of default LLVM dependencies has been creeping up over time (even though LLVM actually does not require them to be enabled). I would consult the LLVM build itself to determine what this FFI is and then add it to Terra, and see if that fixes it.

I see I forgot to release the current support for LLVM 17 in Terra, which I'll probably do shortly. Until then, version 1.1.1 supports up to LLVM 16.

@ghost
Copy link
Author

ghost commented Jan 25, 2024

thanks @elliottslaughter. PR updated with llvm16. just needed to add libffi and update a path.

@ghost ghost mentioned this pull request Jan 26, 2024
@ghost ghost marked this pull request as draft February 29, 2024 19:10
@ghost ghost changed the base branch from master to staging-next March 19, 2024 14:54
@ghost ghost marked this pull request as ready for review March 19, 2024 14:57
@ofborg ofborg bot added the 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin label Mar 19, 2024
@ghost ghost merged commit 5e500c3 into NixOS:staging-next Mar 19, 2024
28 of 29 checks passed
@ghost ghost deleted the llvm11-drop branch March 19, 2024 17:27
@rrbutani rrbutani added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label May 27, 2024
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 6.topic: stdenv Standard environment 8.has: clean-up 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants