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

Backport "Improve composability of mkShell" to release-19.03 #63718

Conversation

basvandijk
Copy link
Member

Motivation for this change

Backport of #63701 to release-19.03.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

mergeInputs is now simply defined in terms of `concatLists` and
`catAttrs` instead of a more complicated `foldr`.

Note that the order of PATH has also changed. For example running the
following with nix-shell:

  let
    pkgs = import <nixpkgs> {};

    shell1 = pkgs.mkShell {
      buildInputs = [ pkgs.htop ];
    };

    shell2 = pkgs.mkShell {
      buildInputs = [ pkgs.hello ];
    };

    shell3 = pkgs.mkShell {
      inputsFrom = [ shell1 shell2 ];
      buildInputs = [ pkgs.tree ];
    };

  in shell3

Results in the following PATH:

$ echo $PATH
...
/nix/store/yifq4bikf7m07160bpia7z48ciqddbfi-tree-1.8.0/bin:
/nix/store/vhxqk81234ivqw1a7j200a1c69k8mywi-htop-2.2.0/bin:
/nix/store/n9vm3m58y1n3rg3mlll17wanc9hln58k-hello-2.10/bin
...

Previously the order was:

/nix/store/n9vm3m58y1n3rg3mlll17wanc9hln58k-hello-2.10/bin
/nix/store/vhxqk81234ivqw1a7j200a1c69k8mywi-htop-2.2.0/bin:
/nix/store/yifq4bikf7m07160bpia7z48ciqddbfi-tree-1.8.0/bin:

I think the new order makes more sense because it allows to override
the PATH in the outermost mkShell.

(cherry picked from commit cee3573)
Running the following expression with nix-shell:

  let
    pkgs = import <nixpkgs> {};

    shell1 = pkgs.mkShell {
      shellHook = ''
        echo shell1
      '';
    };

    shell2 = pkgs.mkShell {
      shellHook = ''
        echo shell2
      '';
    };

    shell3 = pkgs.mkShell {
      inputsFrom = [ shell1 shell2 ];
      shellHook = ''
        echo shell3
      '';
    };
  in shell3

Will now results in:
shell2
shell1
shell3

Note that packages in the front of inputsFrom have precedence over
packages in the back. The outermost mkShell has precedence over all.

(cherry picked from commit 76ef802)
@edolstra
Copy link
Member

edolstra commented Jun 24, 2019

I'm not sure that this kind of change is suitable for backporting since there is a risk that it introduces a regression. The stable branch is for bug fixes, not new features.

@basvandijk
Copy link
Member Author

I actually expected mkShell to compose shellHooks like it composes build inputs (it's for shells after all) and got surprised when it didn't. So this could be considered a fix bringing mkShell to expected behaviour. However I know this is stretching it and I agree this has a risk of introducing a regression for someone relying on the old behaviour not adding more shellHooks to their nix-shell.

Shall I revert?

@zimbatm
Copy link
Member

zimbatm commented Jun 25, 2019

As long as it doesn't break back-compat it doesn't need to be reverted, but I agree with Eelco, it's a new feature

@adamtulinius
Copy link
Member

adamtulinius commented Jul 1, 2019

However I know this is stretching it and I agree this has a risk of introducing a regression for someone relying on the old behaviour not adding more shellHooks to their nix-shell.

It broke our setup at least. This is effectively an API change, and not one I would expect to have to deal with during a normal update. :-)

Edit: We basically did the composing of multiple hooks ourself, and threw a list of a single string after pkgs.mkshell's shellHook attribute.

edolstra added a commit that referenced this pull request Jul 1, 2019
edolstra added a commit that referenced this pull request Jul 1, 2019
@edolstra
Copy link
Member

edolstra commented Jul 1, 2019

@adamtulinius Thanks for the report. I've reverted this on release-19.03.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants