Skip to content

stdenv: fix propagatedUserEnvPkgs when __structuredAttrs is true again#392928

Merged
ShamrockLee merged 2 commits intoNixOS:stagingfrom
linj-fork:pr/propagatedUserEnvPkgs-__structuredAttrs-fix-2
Mar 25, 2025
Merged

stdenv: fix propagatedUserEnvPkgs when __structuredAttrs is true again#392928
ShamrockLee merged 2 commits intoNixOS:stagingfrom
linj-fork:pr/propagatedUserEnvPkgs-__structuredAttrs-fix-2

Conversation

@jian-lin
Copy link
Contributor

@jian-lin jian-lin commented Mar 25, 2025

This is a follow-up of #388908.

Previously, $out/nix-support/propagated-user-env-packages was not created when __structuredAttrs was enabled, the first element of propagatedUserEnvPkgs was null and the length of propagatedUserEnvPkgs was at least 2.

Fixes #388829

With this patch applied, condition check result for each propagatedUserEnvPkgs is shown below.

propagatedUserEnvPkgs __structuredAttrs enabled __structuredAttrs disabled
null false false
[ ] false false
[ null ] false false
[ null "foo" ] true ( previously was false) true

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 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.

Add a 👍 reaction to pull requests you find important.

@jian-lin
stdenv: fix propagatedUserEnvPkgs when __structuredAttrs is true
75281ae 
This is a follow-up of NixOS#388908.

Previously, $out/nix-support/propagated-user-env-packages was not
created when __structuredAttrs is true, the first element of
propagatedUserEnvPkgs is null and the length of propagatedUserEnvPkgs
is at least 2.

Fixes NixOS#388829
@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Mar 25, 2025
@github-actions github-actions bot added 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. labels Mar 25, 2025
@jian-lin jian-lin mentioned this pull request Mar 25, 2025
Closed
3 tasks
@github-actions github-actions bot added the 6.topic: emacs Text editor label Mar 25, 2025
@ShamrockLee
Copy link
Contributor

ShamrockLee commented Mar 25, 2025

The setup.sh change looks good.

wolfgangwalther
wolfgangwalther approved these changes Mar 25, 2025
View reviewed changes
Copy link
Contributor

@wolfgangwalther wolfgangwalther left a comment

Choose a reason for hiding this comment

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

Ah, I even thought about this part in the fir PR - but then assumed that "nobody is going to pass an empty string as the first pkg anyway, so it should be fine". Didn't think about null.

RossComputerGuy
RossComputerGuy approved these changes Mar 25, 2025
View reviewed changes
Copy link
Member

@RossComputerGuy RossComputerGuy left a comment

Choose a reason for hiding this comment

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

LGTM

@ShamrockLee ShamrockLee added the 12.approvals: 2 This PR was reviewed and approved by two persons. label Mar 25, 2025
@ShamrockLee
Copy link
Contributor

ShamrockLee commented Mar 25, 2025 •
edited
Loading

To err on the safe side before merging, has anyone built and confirmed that it works?

I originally planned to test it tonight, but my tablet is temporarily unavailable.

@RossComputerGuy
Copy link
Member

RossComputerGuy commented Mar 25, 2025

What attribute do you want tested? I'll spin up a build on my Ampere system at home.

@ShamrockLee
Copy link
Contributor

ShamrockLee commented Mar 25, 2025 •
edited
Loading

What attribute do you want tested?

I would like to see if

pkgs.hello.overrideAttrs { __structuredAttrs = true; propagatedUserEnvPkgs = [ null pkgs.bash ]; }

built output produces the correct $out/nix-support/propagated-user-env-packages file.

I would be curious about the result for [ null null pkgs.bash ] and [ null null ] as well.

I'll spin up a build on my Ampere system at home.

Thanks a lot!

@RossComputerGuy
Copy link
Member

RossComputerGuy commented Mar 25, 2025

I would like to see if

pkgs.hello.overrideAttrs { __structuredAttrs = true; propagatedUserEnvPkgs = [ null pkgs.bash ]; }

built output produces the correct $out/nix-support/propagated-user-env-packages file.

I would be curious about the result for [ null null pkgs.bash ] and [ null null ] as well.

Ok it built and only has bash interactive in it.

 /nix/store/63j885krhx6yc0dv8z5z5csg97ihz7hy-bash-interactive-5.2p37

ShamrockLee
ShamrockLee approved these changes Mar 25, 2025
View reviewed changes
Copy link
Contributor

@ShamrockLee ShamrockLee left a comment

Choose a reason for hiding this comment

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

I know little about Emacs and its plugins.

Nevertheless, since stdenv's setup.sh works as expected, plut two other approavals, it should be safe to merge.

@ShamrockLee ShamrockLee added 12.approvals: 3+ This PR was reviewed and approved by three or more persons. and removed 12.approvals: 2 This PR was reviewed and approved by two persons. labels Mar 25, 2025
@ShamrockLee ShamrockLee merged commit bc12294 into NixOS:staging Mar 25, 2025
28 of 29 checks passed
@jian-lin jian-lin deleted the pr/propagatedUserEnvPkgs-__structuredAttrs-fix-2 branch March 25, 2025 21:26
jian-lin added a commit to linj-fork/nixpkgs that referenced this pull request Mar 26, 2025
@jian-lin
emacs: disable __structuredAttrs for elisp packages for now
0e1c769 
The issue NixOS#392928 fixes seems to affect many elisp packages.  For
example, about 900 MELPA packages are affected.  This commit is a
temporary fix and should be reverted after NixOS#392928 reaches master.

Close: NixOS#388829
@jian-lin jian-lin mentioned this pull request Mar 26, 2025
Merged
13 tasks
@jian-lin jian-lin changed the title stdenv: fix propagatedUserEnvPkgs when __structuredAttrs is true stdenv: fix propagatedUserEnvPkgs when __structuredAttrs is true again Mar 26, 2025
@jian-lin jian-lin mentioned this pull request Mar 26, 2025
Open
4 tasks
@jian-lin jian-lin mentioned this pull request Apr 15, 2025
Merged
13 tasks
jian-lin added a commit that referenced this pull request Apr 16, 2025
@jian-lin
Revert "emacs: disable __structuredAttrs for elisp packages for now"
52bd64e 
This reverts commit 0e1c769.

The proper fix #392928 is on master now so this temp workaround is not
needed any more.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wolfgangwalther wolfgangwalther wolfgangwalther approved these changes

@RossComputerGuy RossComputerGuy RossComputerGuy approved these changes

@ShamrockLee ShamrockLee ShamrockLee approved these changes

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@philiptaron philiptaron Awaiting requested review from philiptaron

@emilazy emilazy Awaiting requested review from emilazy

@reckenrode reckenrode Awaiting requested review from reckenrode

@Artturin Artturin Awaiting requested review from Artturin

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@AndersonTorres AndersonTorres Awaiting requested review from AndersonTorres

@ttuegel ttuegel Awaiting requested review from ttuegel

@adisbladis adisbladis Awaiting requested review from adisbladis

Assignees

No one assigned

Labels

6.topic: emacs Text editor 6.topic: stdenv Standard environment 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 12.approvals: 3+ This PR was reviewed and approved by three or more persons.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jian-lin @ShamrockLee @RossComputerGuy @wolfgangwalther