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

llvm 15, 16: add zstd support #238866

Closed
wants to merge 5,086 commits into from
Closed

Conversation

SaltyKitkat
Copy link
Contributor

Description of changes

Since llvm 16 support using zstd as its compression method, we should also support that.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.05 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.

@rrbutani
Copy link
Contributor

(for anyone following along, this is another attempt at #238266)

Copy link
Contributor

@rrbutani rrbutani left a comment

Choose a reason for hiding this comment

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

Thanks for continuing to work on this; a few more things:

  • can you change your commit to start with llvm_16 instead of llvm 16? ofborg picks up the former and it's convention
  • as mentioned in #238266 LLVM 16+ has a LLVM_USE_STATIC_ZSTD option; given the direction bzip2, zstd: Add enableStatic option #237563 is headed (i.e. potentially having static/dynamic library outputs present with isStatic = false/isStatic = true respectively IIUC) I think it'd be good to add something like this to cmakeFlags:
    ++ lib.optional enableZstd "-DLLVM_USE_STATIC_ZSTD=${toString stdenv.hostPlatform.isStatic}"
  • as mentioned, now that we're targeting staging anyways we can apply these changes (minus the LLVM_USE_STATIC_ZSTD bit) to llvmPackages_15 in this PR

pkgs/development/compilers/llvm/16/llvm/default.nix Outdated Show resolved Hide resolved
@SaltyKitkat SaltyKitkat force-pushed the llvm_zstd branch 3 times, most recently from 5330724 to d4fc290 Compare June 21, 2023 06:20
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 21, 2023
@SaltyKitkat
Copy link
Contributor Author

Is it necessary to mention this change in release note or something like that?

@ofborg ofborg bot requested a review from rrbutani June 21, 2023 08:21
@SaltyKitkat
Copy link
Contributor Author

Thanks for continuing to work on this; a few more things:

  • can you change your commit to start with llvm_16 instead of llvm 16? ofborg picks up the former and it's convention

  • as mentioned in #238266 LLVM 16+ has a LLVM_USE_STATIC_ZSTD option; given the direction bzip2, zstd: Add enableStatic option #237563 is headed (i.e. potentially having static/dynamic library outputs present with isStatic = false/isStatic = true respectively IIUC) I think it'd be good to add something like this to cmakeFlags:
    nix ++ lib.optional enableZstd "-DLLVM_USE_STATIC_ZSTD=${toString stdenv.hostPlatform.isStatic}"

  • as mentioned, now that we're targeting staging anyways we can apply these changes (minus the LLVM_USE_STATIC_ZSTD bit) to llvmPackages_15 in this PR

Besides, I set CmakeFlags LLVM_PREFER_STATIC_ZSTD in llvm 15 due to this

@SaltyKitkat SaltyKitkat marked this pull request as ready for review June 21, 2023 08:42
@SaltyKitkat SaltyKitkat changed the title llvm 16: add zstd support llvm 15, 16: add zstd support Jun 21, 2023
@rrbutani
Copy link
Contributor

Is it necessary to mention this change in release note or something like that?

I think that's not necessary; this isn't a breaking change and enableZstd is enabled by default (users won't have to do anything to get the feature).


Besides, I set CmakeFlags LLVM_PREFER_STATIC_ZSTD in llvm 15 due to this

Ah, I traced LLVM_PREFER_STATIC_ZSTD to llvm/llvm-project@c0b4f24 and assumed it didn't make it into LLVM 15 (it was introduced after 15.x branched) but it turns out that this and the commit changing it to LLVM_USE_STATIC_ZSTD were backported to LLVM 15.

So we can just mirror the change you made to llvmPackages_16 exactly.

Copy link
Contributor

@rrbutani rrbutani left a comment

Choose a reason for hiding this comment

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

Looks good! Just one last set of nits.

nixos/modules/services/printing/cupsd_tmpfiles.conf Outdated Show resolved Hide resolved
pkgs/development/compilers/llvm/15/llvm/default.nix Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the 8.has: module (update) This PR changes an existing module in `nixos/` label Jun 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related
Projects
None yet
Development

Successfully merging this pull request may close these issues.