Skip to content

stdenv: fix propagatedUserEnvPkgs when __structuredAttrs is true#388908

Merged
jian-lin merged 1 commit intoNixOS:stagingfrom
linj-fork:pr/propagatedUserEnvPkgs-__structuredAttrs-fix
Mar 11, 2025
Merged

stdenv: fix propagatedUserEnvPkgs when __structuredAttrs is true#388908
jian-lin merged 1 commit intoNixOS:stagingfrom
linj-fork:pr/propagatedUserEnvPkgs-__structuredAttrs-fix

Conversation

@jian-lin
Copy link
Contributor

@jian-lin jian-lin commented Mar 11, 2025 •
edited
Loading

Previously, when __structuredAttrs is true, only the first element of propagatedUserEnvPkgs was written to
$out/nix-support/propagated-user-env-packages.

part of #205690

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
adad17e 
Previously, when __structuredAttrs is true, only the first element of
propagatedUserEnvPkgs was written to
$out/nix-support/propagated-user-env-packages.
@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Mar 11, 2025
jian-lin added a commit to linj-fork/nixpkgs that referenced this pull request Mar 11, 2025
@jian-lin
Revert "emacs: enable __structuredAttrs by default in elisp build hel…
e9dab0c 
…pers"

This reverts commit d64a233.

A stdenv bug breaks emacs.pkgs.withPackages wrapper.  A fix PR[1] will
take a few weeks to reach users because it has to go through a staging
cycle.  Revert this for now to unbreak emacs.pkgs.withPackages
wrapper.

[1]: NixOS#388908
@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 11, 2025
This was referenced Mar 11, 2025
Merged
Merged
wolfgangwalther
wolfgangwalther previously requested changes Mar 11, 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.

I just read the code, but I think this breaks the structuredAttrs = false case with multiple of those packages.

Those lines of code seemed to rely on the fact that those are separated by spaces and thus passed as separate arguments to printf / printWords - this won't work anymore right now.

@jian-lin jian-lin mentioned this pull request Mar 11, 2025
Closed
3 tasks
@jian-lin
Copy link
Contributor Author

jian-lin commented Mar 11, 2025

@wolfgangwalther I think it works regardless of __structuredAttrs. When __structuredAttrs is false, propagatedUserEnvPkgs is a space separated string. The output file $out/nix-support/propagated-user-env-packages is identical in both cases.

@wolfgangwalther
Copy link
Contributor

wolfgangwalther commented Mar 11, 2025

When __structuredAttrs is false, propagatedUserEnvPkgs is a space separated string. The output file $out/nix-support/propagated-user-env-packages is identical in both cases.

Ah, you're right... I only looked at how the arguments are passed... but those two functions literally do the separation by spaces that we then already have.. so in this specific case it does indeed not make a difference.

@wolfgangwalther wolfgangwalther dismissed their stale review March 11, 2025 08:19

doesn't apply

philiptaron
philiptaron approved these changes Mar 11, 2025
View reviewed changes
Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

I ran the stdenv test suite (tests.stdenv) and they all passed. The PR looks sane to me. Any objections to merging?

@wolfgangwalther
Copy link
Contributor

wolfgangwalther commented Mar 11, 2025

I put a note on my list to check for similar patterns to fix - but that should not block merging this one.

ShamrockLee
ShamrockLee approved these changes Mar 11, 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.

The functionality should be okay, and ShellCheck doesn't complain about these changes.

pkgs/stdenv/generic/builder.sh
mkdir $out/nix-support
if [ "$propagatedUserEnvPkgs" ]; then
printf '%s ' $propagatedUserEnvPkgs > $out/nix-support/propagated-user-env-packages
printf '%s ' "${propagatedUserEnvPkgs[@]}" > $out/nix-support/propagated-user-env-packages
Copy link
Contributor

@ShamrockLee ShamrockLee Mar 11, 2025

Choose a reason for hiding this comment

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

I personally prefer

Suggested change
printf '%s ' "${propagatedUserEnvPkgs[@]}" > $out/nix-support/propagated-user-env-packages
echo -n "${propagatedUserEnvPkgs[*]} " > $out/nix-support/propagated-user-env-packages

but it's not a blocker.

@jian-lin
Copy link
Contributor Author

jian-lin commented Mar 11, 2025

Given that this PR gets approvals, let's merge it to catch the next staging-next which will start in a few days.

@jian-lin jian-lin merged commit 6e17de9 into NixOS:staging Mar 11, 2025
35 checks passed
@jian-lin jian-lin deleted the pr/propagatedUserEnvPkgs-__structuredAttrs-fix branch March 11, 2025 18:22
This was referenced Mar 11, 2025
Open
Merged
sandptel pushed a commit to sandptel/nixpkgs that referenced this pull request Mar 13, 2025
@jian-lin @sandptel
Revert "emacs: enable __structuredAttrs by default in elisp build hel…
febf3a5 
…pers"

This reverts commit d64a233.

A stdenv bug breaks emacs.pkgs.withPackages wrapper.  A fix PR[1] will
take a few weeks to reach users because it has to go through a staging
cycle.  Revert this for now to unbreak emacs.pkgs.withPackages
wrapper.

[1]: NixOS#388908
jian-lin added a commit that referenced this pull request Mar 13, 2025
@jian-lin
Reapply "emacs: enable __structuredAttrs by default in elisp build he…
3ee5cb7 
…lpers"

This reverts commit e9dab0c.

The fix[1] has been merged into staging, so we can enable
__structuredAttrs again.

[1]: #388908
@jian-lin jian-lin mentioned this pull request Mar 25, 2025
Merged
13 tasks
ShamrockLee pushed a commit that referenced this pull request Mar 25, 2025
@jian-lin
stdenv: fix propagatedUserEnvPkgs when __structuredAttrs is true
75281ae 
This is a follow-up of #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 #388829
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@philiptaron philiptaron philiptaron approved these changes

@ShamrockLee ShamrockLee ShamrockLee approved these changes

@wolfgangwalther wolfgangwalther wolfgangwalther left review comments

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@emilazy emilazy Awaiting requested review from emilazy

@reckenrode reckenrode Awaiting requested review from reckenrode

@Artturin Artturin Awaiting requested review from Artturin

@RossComputerGuy RossComputerGuy Awaiting requested review from RossComputerGuy

Assignees

No one assigned

Labels

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jian-lin @wolfgangwalther @philiptaron @ShamrockLee