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

stdenv: move overriden stdenv in closure #147544

Merged
merged 2 commits into from
Dec 10, 2021

Conversation

cab404
Copy link
Member

@cab404 cab404 commented Nov 26, 2021

Before that, base stdenv passed non-makeOverridable version of itself
inside. This caused it to be lost on package-name.stdenv.

Motivation for this change

Make package.stdenv.override work again.

Read commit message for more info.

Fixes #136756

Also I think it does not actually change stdenv in hash-breaking way, so it does not require world-rebuild.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 21.11 Release Notes (or backporting 21.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Before that, base stdenv passed non-makeOverridable version of itself
inside. This cause it to be lost on package-name.stdenv.
@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Nov 26, 2021
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Nov 26, 2021
@cab404 cab404 mentioned this pull request Nov 26, 2021
@cab404
Copy link
Member Author

cab404 commented Nov 26, 2021

oof, okay, that prompts a lot of buzz.
I just guessed that I've got deep enough in stdenv to be able to review related stuff, so I would like to be a codeowner of that section

@r-burns
Copy link
Contributor

r-burns commented Nov 27, 2021

Haven't looked at this in depth, but thanks :)

Will this make #136804 unnecessary?

@cab404
Copy link
Member Author

cab404 commented Nov 27, 2021

Haven't looked at this in depth, but thanks :)

Will this make #136804 unnecessary?

My solution does not add back __bootPackages and __hatPackages, so your fix is still probably relevant.

@rski
Copy link
Contributor

rski commented Nov 27, 2021

with this patch, env NIX_PATH=nixpkgs=$HOME/Code/nix/nixpkgs nix-build -E 'with import<nixpkgs> {}; enableDebugging pkgs.emacs' starts building for me.
The other derivation that I used to build before the breakage does not however,

env NIX_PATH=nixpkgs=$HOME/Code/nix/nixpkgs nix-build -E 'with import<nixpkgs> {}; enableDebugging(firefox.overrideAttrs (old: rec {  name = "myfirefox"; }))'
error: 'wrapper' at /home/rski/Code/nix/nixpkgs/pkgs/applications/networking/browsers/firefox/wrapper.nix:23:5 called with unexpected argument 'stdenv'

       at /home/rski/Code/nix/nixpkgs/lib/customisation.nix:79:63:

           78|       # Change the result of the function call by applying g to it
           79|       overrideResult = g: makeOverridable (copyArgs (args: g (f args))) origArgs;

I used to use this kind of thing to set separateDebugInfo and meta.outputsToInstall. This is with nix2.4 and nixpkgs@d4839a7e572f5ada860ad3b7f0d2d76ebb860ffb + this PR

@rski
Copy link
Contributor

rski commented Nov 27, 2021

actually, with the above setup firefox fails, but emacs does not:

env NIX_PATH=nixpkgs=$HOME/Code/nix/nixpkgs nix-build --show-trace -E 'with import<nixpkgs> {}; enableDebugging pkgs.firefox'
error: 'wrapper' at /home/rski/Code/nix/nixpkgs/pkgs/applications/networking/browsers/firefox/wrapper.nix:23:5 called with unexpected argument 'stdenv'

       at /home/rski/Code/nix/nixpkgs/lib/customisation.nix:69:16:

           68|     let
           69|       result = f origArgs;
             |                ^
           70|

       … while evaluating 'makeOverridable'

       at /home/rski/Code/nix/nixpkgs/lib/customisation.nix:67:24:

           66|   */
           67|   makeOverridable = f: origArgs:
             |                        ^
           68|     let

       … from call site

       at /home/rski/Code/nix/nixpkgs/lib/customisation.nix:77:41:

           76|       # Re-call the function but with different arguments
           77|       overrideArgs = copyArgs (newArgs: makeOverridable f (overrideWith newArgs));
             |                                         ^
           78|       # Change the result of the function call by applying g to it

       … while evaluating anonymous lambda

       at /home/rski/Code/nix/nixpkgs/lib/customisation.nix:77:32:

           76|       # Re-call the function but with different arguments
           77|       overrideArgs = copyArgs (newArgs: makeOverridable f (overrideWith newArgs));
             |                                ^
           78|       # Change the result of the function call by applying g to it

       … from call site

       at /home/rski/Code/nix/nixpkgs/pkgs/top-level/all-packages.nix:762:26:

          761|   # intended to be used like nix-build -E 'with import <nixpkgs> {}; enableDebugging fooPackage'
          762|   enableDebugging = pkg: pkg.override { stdenv = stdenvAdapters.keepDebugInfo pkg.stdenv; };
             |                          ^
          763|

       … while evaluating 'enableDebugging'

       at /home/rski/Code/nix/nixpkgs/pkgs/top-level/all-packages.nix:762:21:

          761|   # intended to be used like nix-build -E 'with import <nixpkgs> {}; enableDebugging fooPackage'
          762|   enableDebugging = pkg: pkg.override { stdenv = stdenvAdapters.keepDebugInfo pkg.stdenv; };
             |                     ^
          763|

       … from call site

       at «string»:1:26:

            1| with import<nixpkgs> {}; enableDebugging pkgs.firefox
            ```

@cab404
Copy link
Member Author

cab404 commented Nov 27, 2021

actually, with the above setup firefox fails, but emacs does not:

env NIX_PATH=nixpkgs=$HOME/Code/nix/nixpkgs nix-build --show-trace -E 'with import<nixpkgs> {}; enableDebugging pkgs.firefox'
error: 'wrapper' at /home/rski/Code/nix/nixpkgs/pkgs/applications/networking/browsers/firefox/wrapper.nix:23:5 called with unexpected argument 'stdenv'

       at /home/rski/Code/nix/nixpkgs/lib/customisation.nix:69:16:

           68|     let
           69|       result = f origArgs;
             |                ^
           70|

       … while evaluating 'makeOverridable'

       at /home/rski/Code/nix/nixpkgs/lib/customisation.nix:67:24:

           66|   */
           67|   makeOverridable = f: origArgs:
             |                        ^
           68|     let

       … from call site

       at /home/rski/Code/nix/nixpkgs/lib/customisation.nix:77:41:

           76|       # Re-call the function but with different arguments
           77|       overrideArgs = copyArgs (newArgs: makeOverridable f (overrideWith newArgs));
             |                                         ^
           78|       # Change the result of the function call by applying g to it

       … while evaluating anonymous lambda

       at /home/rski/Code/nix/nixpkgs/lib/customisation.nix:77:32:

           76|       # Re-call the function but with different arguments
           77|       overrideArgs = copyArgs (newArgs: makeOverridable f (overrideWith newArgs));
             |                                ^
           78|       # Change the result of the function call by applying g to it

       … from call site

       at /home/rski/Code/nix/nixpkgs/pkgs/top-level/all-packages.nix:762:26:

          761|   # intended to be used like nix-build -E 'with import <nixpkgs> {}; enableDebugging fooPackage'
          762|   enableDebugging = pkg: pkg.override { stdenv = stdenvAdapters.keepDebugInfo pkg.stdenv; };
             |                          ^
          763|

       … while evaluating 'enableDebugging'

       at /home/rski/Code/nix/nixpkgs/pkgs/top-level/all-packages.nix:762:21:

          761|   # intended to be used like nix-build -E 'with import <nixpkgs> {}; enableDebugging fooPackage'
          762|   enableDebugging = pkg: pkg.override { stdenv = stdenvAdapters.keepDebugInfo pkg.stdenv; };
             |                     ^
          763|

       … from call site

       at «string»:1:26:

            1| with import<nixpkgs> {}; enableDebugging pkgs.firefox
            ```

That does get fixed by adding "..." to wrapper in firefox, alas I am not sure why it was broken in the first place.
(that also probably does not apply overridden stdenv somewhere, from that point it is fixable)

@cab404
Copy link
Member Author

cab404 commented Nov 27, 2021

@rski Fun fact is that enableDebugging was not working in firefox since at least fix-stdenv-override~85240. (enableDebugging hello) worked back then. So I guess that's on firefox side, and will require separate PR.

@rski
Copy link
Contributor

rski commented Nov 27, 2021

hah ok, that's fine then, thanks for checking and thank you so much for trying to fix this

@cab404
Copy link
Member Author

cab404 commented Nov 29, 2021

@NixOS/nixos-release-managers please?

Copy link
Member

@balsoft balsoft left a comment

Choose a reason for hiding this comment

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

The change looks correct; I'm not sure what the policy on updating codeowners is.

@cab404
Copy link
Member Author

cab404 commented Nov 30, 2021

@Ericson2314, can you please review the change?)

@symphorien
Copy link
Member

This does not change hashes so it seems low risk for a stdenv change.

@symphorien symphorien merged commit 4a8f997 into NixOS:master Dec 10, 2021
@Artturin
Copy link
Member

Backport after testing it on unstable for a while?

@symphorien
Copy link
Member

after a while, why not

@ajs124
Copy link
Member

ajs124 commented Jan 11, 2022

has a while passed?

@symphorien
Copy link
Member

Yes I think so, and no issue was linked here in the meantime.

@github-actions
Copy link
Contributor

Successfully created backport PR #154652 for release-21.11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: policy discussion 6.topic: stdenv Standard environment 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enableDebugging does not work anymore: error: attribute 'override' missing
7 participants