Skip to content

lldb: Switch from llvmPackages_latest to llvmPackages#116736

Closed
primeos wants to merge 1 commit intoNixOS:masterfrom
primeos:lldb-fix-version
Closed

lldb: Switch from llvmPackages_latest to llvmPackages#116736
primeos wants to merge 1 commit intoNixOS:masterfrom
primeos:lldb-fix-version

Conversation

@primeos
Copy link
Member

@primeos primeos commented Mar 18, 2021

I don't think this was ever supposed to use llvmPackages_latest.
Now it's consistent with the other LLVM attributes (lld, llvm, etc.).

Motivation for this change

Noticed this while checking #116629 (comment) (cc @ggreif) now https://discourse.nixos.org/t/llvm-lldb-11-in-20-09-vs-unstable/10837 makes sense as well.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

I don't think this was ever supposed to use llvmPackages_latest.
Now it's consistent with the other LLVM attributes (lld, llvm, etc.).
@primeos
Copy link
Member Author

primeos commented Mar 18, 2021

Seems like this was intentional: f396492 (#99365).
@Mic92 the current approach seems very inconsistent to me do we really want it to default to the latest version?

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Mar 18, 2021
@primeos
Copy link
Member Author

primeos commented Mar 18, 2021

For some more context:

  clang = llvmPackages.clang;
  clang-manpages = llvmPackages.clang-manpages;
  lld = llvmPackages.lld;
  llvm = llvmPackages.llvm;
  llvm-manpages = llvmPackages.llvm-manpages;

But I just noticed the following is also somewhat inconsistent (though here it makes way more sense IMO):

  clang-tools = callPackage ../development/tools/clang-tools {
    llvmPackages = llvmPackages_latest;
  };

  clang-analyzer = callPackage ../development/tools/analysis/clang-analyzer {
    llvmPackages = llvmPackages_latest;
    inherit (llvmPackages_latest) clang;
  };

IMO all of the aliases (i.e. the attributes that reference llvmPackages.XYZ or llvmPackages_latest.XYZ) should point to the same version.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/llvm-lldb-11-in-20-09-vs-unstable/10837/4

@Mic92
Copy link
Member

Mic92 commented Mar 18, 2021

Seems like this was intentional: f396492 (#99365).
@Mic92 the current approach seems very inconsistent to me do we really want it to default to the latest version?

From my understanding we cannot use the latest llvm/clang version because the ecosystem does not work with it, but why does this apply to lldb? LLVM 7 was released in 2018, which is three years ago.

@primeos
Copy link
Member Author

primeos commented Mar 18, 2021

From my understanding we cannot use the latest llvm/clang version because the ecosystem does not work with it, but why does this apply to lldb?

It doesn't apply to lldb, I only mind that some LLVM attributes point to llvmPackages_latest and others to llvmPackages (which is inconsistent and could cause unexpected issues). The alternative, which might be even better, would be to switch everything to llvmPackages_latest instead.

The important part for the Nixpkgs ecosystem is the following:

  llvmPackages = recurseIntoAttrs (with targetPlatform;
    if isDarwin then
      llvmPackages_7
    else if isFreeBSD then
      llvmPackages_7
    else if isLinux then
      llvmPackages_7
    else if isWasm then
      llvmPackages_8
    else
      llvmPackages_latest);

IMO most stuff in Nixpkgs should use stdenv = llvmPackages.stdenv or e.g. llvmPackages.llvm (instead of llvm - it might actually be best to move those into pkgs/top-level/aliases.nix).

@primeos primeos added the 9.needs: community feedback This needs feedback from more community members. label Mar 26, 2021
@primeos
Copy link
Member Author

primeos commented Mar 26, 2021

So basically the question is do we point the aliases for the individual LLVM packages to llvmPackages or llvmPackages_latest?
I guess llvmPackages_latest would make more sense (i.e. I'd need to change this PR) but I'm fine with both as long as it isn't a mix of both (which IMO only causes unnecessary problems and confusion).

I'd also like to move them to pkgs/top-level/aliases.nix to ensure they aren't used in Nixpkgs (they're convenient for users but IMO packages in Nixpkgs should use (mainly) llvmPackages.stdenv). Any objections?

@Mic92
Copy link
Member

Mic92 commented Mar 29, 2021

So basically the question is do we point the aliases for the individual LLVM packages to llvmPackages or llvmPackages_latest?
I guess llvmPackages_latest would make more sense (i.e. I'd need to change this PR) but I'm fine with both as long as it isn't a mix of both (which IMO only causes unnecessary problems and confusion).

I'd also like to move them to pkgs/top-level/aliases.nix to ensure they aren't used in Nixpkgs (they're convenient for users but IMO packages in Nixpkgs should use (mainly) llvmPackages.stdenv). Any objections?

That makes sense to me.

@primeos
Copy link
Member Author

primeos commented Mar 30, 2021

I tried to implement #116736 (comment) in #118076 but unfortunately llvm and clang are used much more than I've expected.

@primeos primeos closed this Mar 30, 2021
@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 9.needs: community feedback This needs feedback from more community members. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants