Skip to content

Add zstd support for llvm 16#238266

Closed
SaltyKitkat wants to merge 2415 commits intoNixOS:stagingfrom
SaltyKitkat:llvm_zstd
Closed

Add zstd support for llvm 16#238266
SaltyKitkat wants to merge 2415 commits intoNixOS:stagingfrom
SaltyKitkat:llvm_zstd

Conversation

@SaltyKitkat
Copy link
Contributor

@SaltyKitkat SaltyKitkat commented Jun 17, 2023 •
edited
Loading

Description of changes

LLVM add zstd support since 16

And maybe we should 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.

@ofborg ofborg bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Jun 17, 2023
@RaitoBezarius
Copy link
Member

RaitoBezarius commented Jun 17, 2023

Can you detail more what is meant by zstd support and what does it bring concretely?

@rrbutani
Copy link
Contributor

rrbutani commented Jun 17, 2023

Can you detail more what is meant by zstd support and what does it bring concretely?

@RaitoBezarius This enables features like clang -gz=zstd (compressing debug sections with zstd) and corresponding support in lld and in the LLVM binutils.

rrbutani
rrbutani suggested changes Jun 17, 2023
View reviewed changes
Copy link
Contributor

@rrbutani rrbutani left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for opening this PR! zstd support in LLVM is definitely something we overlooked.

A few comments:

  • zstd support in clang and lld did land in LLVM 16 but zstd was added to the LLVM support library in LLVM 15: llvm/llvm-project@e939bf6; can you amend this PR to backport this change to llvmPackages_15 as well?
    • edit: will do in a follow up PR
  • it'd be nice to have some tests for this (i.e. a passthru test on clang that attempts to pass -gz=zstd if enableZstd is true on the underlying llvm) but I don't think it's a blocker for this PR
  • LLVM also has a LLVM_USE_STATIC_ZSTD option (added LLVM 16) that does not appear to be autodetected; we may want to pass this explicitly as part of the cmake flags when stdenv.hostPlatform.isStatic is true (though I am not sure if this is actually required...)

pkgs/development/compilers/llvm/16/lldb/default.nix Outdated
Copy link
Contributor

@rrbutani rrbutani Jun 17, 2023

Choose a reason for hiding this comment

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

I don't see anything in lldb (as of LLVM 16) that uses zstd (and I believe LLVM projects should inherit zstd from propagatedBuildInputs from llvm anyways).

Is there some functionality in lldb that requires zstd as a dep here?

Copy link
Contributor Author

@SaltyKitkat SaltyKitkat Jun 18, 2023

Choose a reason for hiding this comment

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

Is there some functionality in lldb that requires zstd as a dep here?

Actually I don't know. I just search where zlib appears, and add zstd. LOL

pkgs/development/compilers/llvm/16/lldb/default.nix Outdated
Copy link
Contributor

@rrbutani rrbutani Jun 17, 2023

Choose a reason for hiding this comment

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

Ditto; is this required?

(as an aside I think zlib here is redundant but it's fine; can address that in another PR)

Copy link
Contributor Author

@SaltyKitkat SaltyKitkat Jun 18, 2023

Choose a reason for hiding this comment

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

I remove both zlib and zstd in this file
and it seems that lldb can still be compiled with zlib and zstd support?
cmake found zlib and zstd anyway.

pkgs/development/compilers/llvm/16/llvm/default.nix Outdated
Copy link
Contributor

@rrbutani rrbutani Jun 17, 2023

Choose a reason for hiding this comment

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

Can we gate this on an enableZstd input to the package (defaulted to `true)?

Copy link
Contributor

@rrbutani rrbutani Jun 17, 2023

Choose a reason for hiding this comment

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

It'd also be nice if we could then propagate it as a passthru (i.e. passthru = { inherit enableZstd; }) so we can gate tests and such in downstream deps (i.e. clang, lld) on it.

Copy link
Contributor Author

@SaltyKitkat SaltyKitkat Jun 19, 2023

Choose a reason for hiding this comment

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

Can we gate this on an enableZstd input to the package (defaulted to `true)?

I'm a little confused here.

  • Why we need a feature gate here? It seems that no zstd support brings no benefits from my perspective?

  • I know nothing about tests in nixpkgs. Maybe I need some help about the tests.

Copy link
Member

@RaitoBezarius RaitoBezarius Jun 19, 2023

Choose a reason for hiding this comment

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

Feature gate is useful to control closure sizes and sometimes when something make a special variant incompatible, e.g. static builds, musl builds, cross compilation, etc.

Copy link
Member

@RaitoBezarius RaitoBezarius Jun 19, 2023

Choose a reason for hiding this comment

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

If we passthru it, we can recognize whether any LLVM, Clang, LLD at hand is able to do zstd or not and we can decide whether our test harness will attempt to decompress stuff using zstd. Implementing the passthru is the only thing needed here at the moment AFAIK.

@ofborg ofborg bot requested a review from rrbutani June 18, 2023 03:33
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. and removed 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Jun 18, 2023
@SaltyKitkat
Copy link
Contributor Author

SaltyKitkat commented Jun 18, 2023

@rrbutani oops… backporting this to llvm15 seems cause lots of rebuild

should we open a separate pr for that?

@rrbutani
Copy link
Contributor

rrbutani commented Jun 18, 2023

backporting this to llvm15 seems cause lots of rebuild

should we open a separate pr for that?

Sure! If/when you open that PR, be sure to target staging.

@rrbutani
Copy link
Contributor

rrbutani commented Jun 19, 2023

Now that #236420 has reached master I think we'll need to retarget this PR to staging; there are too many LLVM 16 rdeps (via Rust 1.70).

@SaltyKitkat would you mind adding back in the LLVM 15 changes and retargeting this PR? (instructions for retargeting to staging are here)

@SaltyKitkat SaltyKitkat changed the base branch from master to staging June 19, 2023 06:22
@github-actions github-actions bot added 6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: golang Go is a high-level general purpose programming language that is statically typed and compiled. 6.topic: haskell General-purpose, statically typed, purely functional programming language 6.topic: kernel The Linux kernel 6.topic: nim Nim programing language 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: ocaml OCaml is a general-purpose, high-level, multi-paradigm programming language. 6.topic: pantheon The Pantheon desktop environment 6.topic: policy discussion Discuss policies to work in and around Nixpkgs 6.topic: printing Drivers, CUPS & Co. 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: qt/kde Object-oriented framework for GUI creation 6.topic: ruby A dynamic, open source programming language with a focus on simplicity and productivity. 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 6.topic: stdenv Standard environment 6.topic: steam Steam game store/launcher (store.steampowered.com) 6.topic: systemd Software suite that provides an array of system components for Linux operating systems. 6.topic: TeX Issues regarding texlive and TeX in general 6.topic: vim Advanced text editor 6.topic: vscode A free and versatile code editor that supports almost every major programming language. 6.topic: xfce The Xfce Desktop Environment 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 19, 2023
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jun 19, 2023
@SaltyKitkat
Copy link
Contributor Author

SaltyKitkat commented Jun 19, 2023

Oops…

@NixOS NixOS locked as too heated and limited conversation to collaborators Jun 19, 2023
@rrbutani rrbutani added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label May 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dtzWill dtzWill Awaiting requested review from dtzWill

@alyssais alyssais Awaiting requested review from alyssais

@lovek323 lovek323 Awaiting requested review from lovek323

@RaitoBezarius RaitoBezarius Awaiting requested review from RaitoBezarius

@sternenseemann sternenseemann Awaiting requested review from sternenseemann

@primeos primeos Awaiting requested review from primeos

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@rrbutani rrbutani Awaiting requested review from rrbutani

Assignees

No one assigned

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: cinnamon Desktop environment 6.topic: emacs Text editor 6.topic: erlang General-purpose, concurrent, functional high-level programming language 6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: golang Go is a high-level general purpose programming language that is statically typed and compiled. 6.topic: haskell General-purpose, statically typed, purely functional programming language 6.topic: kernel The Linux kernel 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 6.topic: nim Nim programing language 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: ocaml OCaml is a general-purpose, high-level, multi-paradigm programming language. 6.topic: pantheon The Pantheon desktop environment 6.topic: policy discussion Discuss policies to work in and around Nixpkgs 6.topic: printing Drivers, CUPS & Co. 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: qt/kde Object-oriented framework for GUI creation 6.topic: ruby A dynamic, open source programming language with a focus on simplicity and productivity. 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 6.topic: stdenv Standard environment 6.topic: steam Steam game store/launcher (store.steampowered.com) 6.topic: systemd Software suite that provides an array of system components for Linux operating systems. 6.topic: TeX Issues regarding texlive and TeX in general 6.topic: vim Advanced text editor 6.topic: vscode A free and versatile code editor that supports almost every major programming language. 6.topic: xfce The Xfce Desktop Environment 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.