-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
nixos/ldso: avoid instance of nixpkgs #288509
Conversation
@yu-re-ka mind trying this out? |
How to reproduce? |
Not the same error, but my issue can be reproduced by setting
The exact error is described here: #269551 (comment) |
I wasn't able to create a repro, both our use cases are related to creating these new instances of nixpkgs. |
I still think it would be nicer to test for the evaluation of the stdenv of pkgsi686Linux somehow and check that it is supported (and then do that check instead of the But for now this would give me a workaround other than cutting out the ldso module entirely. |
@zimbatm does it still make such a difference when other 32-bit libs are already being evaluated like hardware.pulseaudio.support32Bit or hardware.opengl.driSupport32Bit ? |
What I don't yet understand is why it would eval pkgsi686Linux when Another thing we might look into is to see whether these variables we need are available through other means. Since this is x86_64-only, we could take I don't like defining such values in multiple places. OTOH, this value is pretty much constant. |
Because it still needs to generate a rule deleting the previously existing link, and the file path of that rule is determined from
I like this idea better. Perhaps we can add the other needed information to the system elaboration as well?
I also don't like hardcoding it here, in the long term. But it's a reasonable stopgap measure, in my view. |
Could we? Some of that might be linked (heh) to the libc you're using. |
The more I think about it, the more I think this is actually fine. The entire way multiarch stuff on x86 is handled is pretty ad-hoc anyway, without a proper abstraction within nixpkgs and/or nixos. Trying to grab the info from elsewhere really might not add much of anything in terms of cleanliness/robustness, until such a general multiarch abstraction exists, at which point all this will need reworking anyway. Good to leave a comment about where the hardcoded information came from, and why it's hardcoded, though. |
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.
With minor tweaks, looks good to me.
Follow-up to NixOS#269551 Avoid creating a new instance of nixpkgs to access two variables. `pkgs.pkgsi686Linux` was being accessed whenever the feature is being used or not. A second instance of nixpkgs is being created in `nixos/modules/config/stub-ld.nix` and can be disabled by setting `environment.ldso32 = null` or `environment.stub-ld.enable = false`. Both combined fixes this error: error: attribute 'i686-linux' missing
Ok, I'm glad we managed to find some compromise. Given that it's legacy support, I don't expect those constants to change. Probably a better name for
It still saves 1.4% of GC in my non-scientific test. I'm also thinking of server scenarios where those options would not be enabled. |
Follow-up to NixOS#269551 Avoid creating a new instance of nixpkgs to access two variables. `pkgs.pkgsi686Linux` was being accessed whenever the feature is being used or not. A second instance of nixpkgs is being created in `nixos/modules/config/stub-ld.nix` and can be disabled by setting `environment.ldso32 = null` or `environment.stub-ld.enable = false`. Both combined fixes this error: error: attribute 'i686-linux' missing
Follow up to #269551
Avoid creating a new instance of nixpkgs to access two variables.
pkgs.pkgsi686Linux
was being accessed whenever the feature is beingused or not.
A second instance of nixpkgs is being created in
nixos/modules/config/stub-ld.nix
and can be disabled by settingenvironment.ldso32 = null
orenvironment.stub-ld.enable = false
.Both combined fixes this error:
Description of changes
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.