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

llvmPackages.*: Add devExtraCmakeFlags parameter #342040

Merged
merged 1 commit into from
Sep 22, 2024

Conversation

pwaller
Copy link
Contributor

@pwaller pwaller commented Sep 15, 2024

Description of changes

cmake flags have a 'last flag wins' logic, so by appending to the end of
the flags it is possible to override any cmake flag.

It also ignores (and warns) if a flag is unused, so passing flags across
all packages should be safe if you want to target one package pass flags which aren't supported by all packages.

In combination with #320261, this PR allows consistently overriding all
packages within LLVM with additional cmake arguments. Consistency here
means for example 'if you override LLVM, then all dependencies on it
also see the overridden LLVM in their input'. Consistency is hard to
achieve with the other obvious way of implementing this as a user: if
you use overrideAttrs then you have to write a big mess of override code
in order to override all dependents, and this can be very difficult in a
cross-compilation scenario using crossSystem and useLLVM, for example.

With this PR it is possible to write an overlay which overlays
llvmPackages with super.llvmPackages.override { devExtraCmakeFlags = [ ... ]; },
and then the toolchain used with useLLVM in effect should respect
these flags.

This is useful in development for experimenting with the effect of
various flags, hence the chosen name devCmakeFlags.

This won't work out of the box without #341855 applied, which fixes
override passthrough.

See-Also: #320261, #341855

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.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label Sep 15, 2024
cmake flags have a 'last flag wins' logic, so by appending to the end of
the flags it is possible to override any cmake flag.

It also ignores (and warns) if a flag is unused, so passing flags across
all packages should be safe if you want to target one package.

In combination with NixOS#320261, this PR allows consistently overriding all
packages within LLVM with additional cmake arguments. Consistency here
means for example 'if you override LLVM, then all dependencies on it are
also see the overridden LLVM in their input'. Consistency is hard to
achieve with the other obvious way of implementing this as a user: if
you use overrideAttrs then you have to write a big mess of override code
in order to override all dependents, and this can be very difficult in a
cross-compilation scenario using crossSystem and useLLVM, for example.

With this PR it is possible to write an overlay which overlays
`llvmPackages` with `llvmPackage.override { devExtraCmakeFlags = [ ... ]; }`,
and then the toolchain used with useLLVM in effect should respect
these flags.

This is useful in development for experimenting with the effect of
various flags, hence the chosen name `devCmakeFlags`.

This won't work out of the box without NixOS#341855 applied, which fixes
override passthrough.

See-Also: NixOS#320261, NixOS#341855
Signed-off-by: Peter Waller <[email protected]>
@pwaller pwaller marked this pull request as ready for review September 15, 2024 11:08
@pwaller
Copy link
Contributor Author

pwaller commented Sep 15, 2024

/cc potentially interested @RossComputerGuy @Ericson2314 @reckenrode @paparodeo @alyssais

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Sep 15, 2024
@alyssais
Copy link
Member

Could you share an example use case? What flags did you want to set?

@pwaller
Copy link
Contributor Author

pwaller commented Sep 15, 2024

Could you share an example use case? What flags did you want to set?

Setting LLVM_BUILD_INSTRUMENTED=IR, LLVM_PROFDATA, LLVM_USE_LTO in various cases in order to get a PGO/LTO build is one.

More generally, as a dev, I want to test LLVM in a variety of different configurations using options documented at https://llvm.org/docs/CMake.html.

Some options may (or may not, I don't know if nixpkgs has a policy on this) deserve being parameters in their own right (like for example the PGO/LTO build maybe), but in general it is useful to be able to access arbitrary cmake parameters consistently across the whole of LLVM for the sake of development and testing.

Copy link
Member

@alyssais alyssais left a comment

Choose a reason for hiding this comment

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

Makes sense to be as long as other LLVM maintainers are happy.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 15, 2024
Copy link
Member

@RossComputerGuy RossComputerGuy left a comment

Choose a reason for hiding this comment

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

Any improvements we can make to the dev shell experience, the better.

@RossComputerGuy RossComputerGuy added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Sep 16, 2024
@paparodeo
Copy link
Contributor

This is useful in development for experimenting with the effect of various flags, hence the chosen name devCmakeFlags.

globalCmakeFlags? idk. i am neutral-negative on the change. as it is a no-op my preference is to usually delete code that does nothing. though, i haven't touched then llvm stuff in some time so if this change makes life easier for further llvm work it seems positive.

@pwaller
Copy link
Contributor Author

pwaller commented Sep 20, 2024

Thanks for the reviews!

globalCmakeFlags? idk.

The word 'global' is probably too strong since it might imply 'beyond the llvm packageset' or 'influencing the cmake flags of other projects', which this is not. My thinking of including 'dev' in the name was to discourage general use. (Although if one were found, it could be renamed).

as it is a no-op my preference is to usually delete code that does nothing.

There are lots of things which are effectively no-op, but their existence enables additional use cases through overriding. This is such a case where lacking the passthrough it is quite difficult to achieve without patching nixpkgs, which is inconvenient and hinders experimentation and development.

@RossComputerGuy RossComputerGuy added 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people and removed 12.approvals: 2 This PR was reviewed and approved by two reputable people labels Sep 22, 2024
@RossComputerGuy
Copy link
Member

I assume we're good to merge?

@RossComputerGuy RossComputerGuy merged commit 464d8f5 into NixOS:master Sep 22, 2024
35 checks passed
@pwaller pwaller deleted the devExtraCmakeFlags branch October 10, 2024 16:25
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 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants