Skip to content

Conversation

@alyssais
Copy link
Member

@alyssais alyssais commented May 4, 2023

Description of changes

Because llvmPackages_latest is used in Nixpkgs, by quite a few packages, it's difficult to keep it up to date, because updating it requires some level of confidence that every package that uses it is going to keep working after the update. The result of this is that llvmPackages_latest is not updated, and so we end up in the situation that "latest" is two versions older than the latest version we actually provide. This is confusing and unexpected.

"But won't this end up fragmenting our LLVM versions, if every package previously using _latest is separately pinned to LLVM 14?", I hear you ask. No. That fragmentation is already happening, even with an llvmPackages_latest, because packages that actually require the latest version of LLVM (15/16), have already been decoupled from llvmPackages_latest since it hasn't been upgraded. So like it or not, we can't escape packages depending on specific recent LLVMs. The only real fix is to get better at keeping the default LLVM up to date (which I'm reasonably confident we're getting into a better position to be feasibly better able to do).

So, unless we want to double down on providing a confusingly named "llvmPackages_latest" attribute that refers to some arbitrary LLVM version that's probably not the latest one (or even the latest one available in Nixpkgs), we only have two options here: either we don't provide such an attribute at all, or we don't use it in Nixpkgs so we don't become scared to bump it as soon as we have a new LLVM available.

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.05 Release Notes (or backporting 22.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.

@alyssais alyssais added the 6.topic: policy discussion Discuss policies to work in and around Nixpkgs label May 4, 2023
@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/` and removed 6.topic: policy discussion Discuss policies to work in and around Nixpkgs labels May 4, 2023
@ofborg ofborg bot added 8.has: clean-up This PR removes packages or removes other cruft 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels May 4, 2023
Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

Agreed with the rationale.

@rrbutani rrbutani added the 6.topic: policy discussion Discuss policies to work in and around Nixpkgs label May 4, 2023
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.

I wholeheartedly agree with the spirit of this change.

I think it makes sense to have packages that are fairly closely coupled with LLVM update their LLVM version at their own cadence and I think it makes sense that deviating from llvmPackages is a good indicator that a package is somewhat closely coupled to LLVM. For packages in the latter category I think it's actually better that they pin an LLVM version than use _latest; bumping LLVM in these packages can be involved and should be something maintainers of these packages can do on their own timeline (I think #213202 and #225197 are a good example of this).

I think the only potential downside to this change is: the packages that aren't super sensitive to their LLVM version that have been able to benefit passively from us bumping llvmPackages_latest will no longer be able to do so if they're ever made to use something other than llvmPackages. For example: if an infrequently maintained package is made to use a newer-than-llvmPackages version but then isn't bumped for many years.

But as @alyssais notes, the way to mitigate this is to bump llvmPackages more aggressively (to reduce the need to pin to newer LLVM versions) and also to perhaps periodically try to prune uses of older LLVM package sets (as @RaitoBezarius and others have been trying to do recently).


I have a couple of nits (packages that aren't sensitive to LLVM version for which I think we should use llvmPackages rather than pinning, lest we forget to update the pinned version — assuming we're okay with the version of LLVM used by these packages/aliases going backwards for a little bit) but nothing that I feel strongly about.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this llvmPackages instead of pinning to llvmPackages_14? This package (just a wrapper) isn't sensitive to changes in LLVM and this seems like exactly the kind of thing we'd forget to bump.

Suggested change
llvmPackages = llvmPackages_14;

Copy link
Member Author

Choose a reason for hiding this comment

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

I think people probably want a recent clang for clangd, clang-tidy, etc. I'd rather bump this to 16.

Comment on lines 14595 to 14596
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto with this package; just another wrapper.

Suggested change
llvmPackages = llvmPackages_14;
inherit (llvmPackages_14) clang;

Copy link
Contributor

Choose a reason for hiding this comment

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

lldb was an outlier in that unlike the other tool aliases (clang, lld, etc) it pointed to llvmPackages_latest instead of llvmPackages.

I believe the motivation (#99365) was to have a newer lldb version by default; if we're planning to bump llvmPackages more aggressively I think we can safely set this to:

Suggested change
lldb = lldb_14;
lldb = llvmPackages.lldb;

Though this would mean that for now lldb in nixpkgs will go backwards from v14 to v11/v12 (depending on platform); I'm not sure if that's acceptable.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like (imo) the one place where picking the latest version might actually be what we want? (assuming that newest LLVM is our best bet at providing support for other/untested platforms)

On the other hand there probably aren't many users relying on this fallback anyways (even if we forget to bump this it's probably okay) and I'd rather see us gain the ability to specify the LLVM version as part of the system config anyways. So I think this is fine.

@rrbutani rrbutani mentioned this pull request May 4, 2023
13 tasks
@alyssais
Copy link
Member Author

alyssais commented May 4, 2023

I have a couple of nits (packages that aren't sensitive to LLVM version for which I think we should use llvmPackages rather than pinning, lest we forget to update the pinned version — assuming we're okay with the version of LLVM used by these packages/aliases going backwards for a little bit) but nothing that I feel strongly about.

These are all valid suggestions, but the point of this work is that we don't need to make decisions about multiple packages at once in a single PR, so in that spirit I'm not going to be making any of those changes here.

@alyssais
Copy link
Member Author

alyssais commented May 4, 2023

I think the only potential downside to this change is: the packages that aren't super sensitive to their LLVM version that have been able to benefit passively from us bumping llvmPackages_latest will no longer be able to do so if they're ever made to use something other than llvmPackages. For example: if an infrequently maintained package is made to use a newer-than-llvmPackages version but then isn't bumped for many years.

This is true in theory, but not in practice, because in practice we have not been bumping llvmPackages_latest at all. It came close to not even getting bumped to 14.

Copy link
Member

Choose a reason for hiding this comment

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

Default seems to make more sense here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I've changed it to pkgs.clangStdenv.

@mweinelt mweinelt added the 12.approvals: 3+ This PR was reviewed and approved by three or more persons. label May 6, 2023
@alyssais alyssais force-pushed the llvmPackages_latest-alias branch from 76754c2 to 3b6d31f Compare May 8, 2023 12:14
@github-actions github-actions bot removed the 6.topic: policy discussion Discuss policies to work in and around Nixpkgs label May 8, 2023
alyssais added 2 commits May 8, 2023 12:20
Because llvmPackages_latest is used in Nixpkgs, by quite a few
packages, it's difficult to keep it up to date, because updating it
requires some level of confidence that every package that uses it is
going to keep working after the update.  The result of this is that
llvmPackages_latest is not updated, and so we end up in the situation
that "latest" is two versions older than the latest version we
actually provide.  This is confusing and unexpected.

"But won't this end up fragmenting our LLVM versions, if every package
previously using _latest is separately pinned to LLVM 14?", I hear you
ask.  No.  That fragmentation is already happening, even with an
llvmPackages_latest, because packages that actually require the
_latest_ version of LLVM (15/16), have already been decoupled from
llvmPackages_latest since it hasn't been upgraded.  So like it or not,
we can't escape packages depending on specific recent LLVMs.  The only
real fix is to get better at keeping the default LLVM up to
date (which I'm reasonably confident we're getting into a better
position to be feasibly better able to do).

So, unless we want to double down on providing a confusingly named
"llvmPackages_latest" attribute that refers to some arbitrary LLVM
version that's probably not the latest one (or even the latest one
available in Nixpkgs), we only have two options here: either we don't
provide such an attribute at all, or we don't use it in Nixpkgs so we
don't become scared to bump it as soon as we have a new LLVM available.
@alyssais alyssais force-pushed the llvmPackages_latest-alias branch from 3b6d31f to 3b88760 Compare May 8, 2023 12:20
@alyssais
Copy link
Member Author

alyssais commented May 8, 2023

This should be merged soon after OfBorg is happy, because since I opened it two more uses of llvmPackages_latest appeared that I've had to fix.

@alyssais alyssais merged commit da41baa into NixOS:master May 8, 2023
@alyssais alyssais deleted the llvmPackages_latest-alias branch May 8, 2023 13:05
@rrbutani rrbutani added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label May 27, 2024
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: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: clean-up This PR removes packages or removes other cruft 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 3+ This PR was reviewed and approved by three or more persons.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants