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_12: Finalize reorganizing the files #120506

Merged
merged 2 commits into from Apr 24, 2021
Merged

Conversation

primeos
Copy link
Member

@primeos primeos commented Apr 24, 2021

Motivation for this change

Now it is more consistent.

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.

This might be a bit debatable but upstream uses "xx" instead of "++"
when using it as identifier / in the code (file/directory names, build
scripts, website URLs, etc.) so we should probably too.
And at least the attribute name and pname will be consistent now.
For consistency. Now all packages will have their own subdirectory
(continuation of 781e69d).
@primeos
Copy link
Member Author

primeos commented Apr 24, 2021

@GrahamcOfBorg build llvmPackages_12

@r-rmcgibbo
Copy link

r-rmcgibbo commented Apr 24, 2021

Result of nixpkgs-review pr 120506 at e4f8498 run on aarch64-linux 1

5 packages built successfully:
  • llvmPackages_12.libcxx
  • llvmPackages_12.libcxxClang
  • llvmPackages_12.libcxxStdenv
  • llvmPackages_12.libcxxabi
  • llvmPackages_12.lldClang

Result of nixpkgs-review pr 120506 at e4f8498 run on x86_64-linux 1

5 packages built successfully:
  • llvmPackages_12.libcxx
  • llvmPackages_12.libcxxClang
  • llvmPackages_12.libcxxStdenv
  • llvmPackages_12.libcxxabi
  • llvmPackages_12.lldClang
1 suggestion:
  • warning: missing-phase-hooks

    installPhase should probably contain runHook preInstall and runHook postInstall.

    Near pkgs/development/compilers/llvm/12/libcxxabi/default.nix:37:3:

       |
    37 |   installPhase = if stdenv.isDarwin
       |   ^
    

@primeos primeos merged commit faf4d48 into NixOS:master Apr 24, 2021
@Ericson2314
Copy link
Member

I would much prefer if changes like this were to all the LLVMs. Maybe it's more consistent within LLVM 12, but it's less consistent across versions, and that makes maintenance harder.

(We should try to dedup some things too, which also would prevent drift.)

@primeos
Copy link
Member Author

primeos commented Apr 27, 2021

Applying it to all LLVM versions might help for some changes (and should at least be easy to do, if necessary) but our LLVM expressions are already pretty unmaintained (I don't think there's even one active maintainer) and seem to need some serious cleanup.
Anyway, agreed that it would be helpful but IMO LLVM is already more (or less) bit rotting and I'm currently attempting to make more aggressive changes to recent versions in the hopes of preventing it from becoming a lost cause. However, what we'd really need is a proper team of active maintainers for LLVM.

@Ericson2314
Copy link
Member

@primeos I have a rather large cleanup I'm doing in #111487. I have normalized many things already. I just would rather this be normalized separately than make that one even bigger.

@primeos
Copy link
Member Author

primeos commented Apr 28, 2021

I would much prefer if changes like this were to all the LLVMs.

I agree with that now. I wasn't aware of PRs like #111487 (the ones I've seen usually only touched a single LLVM version to fix something the author cared about) and I'll keep it in mind for the next time. And thanks for looking after our LLVM packaging! :)

@Ericson2314
Copy link
Member

Ericson2314 commented Apr 28, 2021

@primeos yeah it's fight I've taken to various compilers with lots of copy-paste from time to time. Did GCC and GHC in the past.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants