-
-
Notifications
You must be signed in to change notification settings - Fork 18k
stdenv: fix eval for native LLVM #463361
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
base: staging
Are you sure you want to change the base?
stdenv: fix eval for native LLVM #463361
Conversation
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/adding-usellvm-to-a-nixos-system-flake/66487/2 |
9c768e9 to
737f6b0
Compare
| runCommand "llvm-binutils-${version}" | ||
| { | ||
| pname = "llvm-binutils"; | ||
| inherit version; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this have to do with fixing eval?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes the expression I mentioned actually evaluate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the nixpkgs configuration fails here with missing attribute errors and it's needed for #463395
nixpkgs/pkgs/stdenv/linux/default.nix
Lines 559 to 560 in 411faf4
| pname = prevStage.bintools.bintools.pname + "-patchelfed-ld"; | |
| inherit (prevStage.bintools.bintools) version; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the commit message please explain that stdenv needs those attributes? Apart from that LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
LordGrimmauld
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm this does fix eval. I am building my toy VM against this just to see how far i get. I don't feel confident approving this, stdenv is a bit out of my expertise, but this does look useful
|
Yes, #463395 actually makes it possible. This PR just makes eval not broken. My idea is to have the native LLVM not cause any side effects so the eval fix is in its own PR. |
Adds the pname & version since `pkgs/stdenv/linux/default.nix` inherits the version & appends a string to the name.
737f6b0 to
25908a8
Compare
| installPhase = '' | ||
| mkdir -p "$out"/bin | ||
| cp -a '${prevStage.bintools.bintools}'/bin/* "$out"/bin/ | ||
| cp -a '${prevStage.binutils.bintools}'/bin/* "$out"/bin/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this do the wrong thing when we're using LLVM bintools?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't binutils an alias for whatever linker is configured via linker in {local,cross}System?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that's what bintools is. binutils is GNU binutils.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, I'll have to take a look at what I did. It might've been an infinite recursion thing. I'm traveling right now so I won't be able to look at it for some time.

Things done
Fixes evaluation of:
This does not make it usable. Making it usable will be a follow up PR.
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.