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

mkShell: introduce packages argument #122180

Merged
merged 1 commit into from May 13, 2021
Merged

Conversation

zimbatm
Copy link
Member

@zimbatm zimbatm commented May 8, 2021

Motivation for this change

The distinction between the inputs doesn't really make sense in the
mkShell context. Technically speaking, we should be using the
nativeBuildInputs most of the time.

So in order to make this function more beginner-friendly, add "packages"
as an attribute, that maps to nativeBuildInputs.

This commit also updates all the uses in nixpkgs.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

The distinction between the inputs doesn't really make sense in the
mkShell context.  Technically speaking, we should be using the
nativeBuildInputs most of the time.

So in order to make this function more beginner-friendly, add "packages"
as an attribute, that maps to nativeBuildInputs.

This commit also updates all the uses in nixpkgs.
@mohe2015
Copy link
Contributor

mohe2015 commented May 8, 2021

I would be more in favor of just updating the documentation and changing to nativeBuildInputs in the code.

@roberth
Copy link
Member

roberth commented May 8, 2021

I'd prefer to rename nativeBuildInputs to tools. I'm biased though, because I'm a tool ;)

@zimbatm
Copy link
Member Author

zimbatm commented May 13, 2021

I guess it was to be expected to get bikeshedding reviews. Since it's a matter of preference and I am the author of mkShell I will just go ahead and merge this.

@zimbatm zimbatm merged commit c6b62f2 into NixOS:master May 13, 2021
@zimbatm zimbatm deleted the mkshell-packages branch May 13, 2021 17:17
@roberth
Copy link
Member

roberth commented May 14, 2021

I guess it was to be expected to get bikeshedding reviews. Since it's a matter of preference and I am the author of mkShell I will just go ahead and merge this.

Go for it. Don't let mere ideas and opinions get in the way of improvement.

@dramforever
Copy link
Contributor

I would like to request some clarification. As someone who enjoys popping up a cross-compilation mkShell from time to time to play with things, this struck me as weird, as mkShell is only a mkDerivation that can't be built:

The distinction between the inputs doesn't really make sense in the
mkShell context.

Whereas this one seems to be at odds with a lot of what I've been doing

Technically speaking, we should be using the
nativeBuildInputs most of the time.

After giving it some thought, I think maybe mkShell has a few competing use cases that should be handled separately.

  • Providing libraries for a development environment
  • Providing build tools for a development environment
  • Providing programs for the user to run in a shell environment

It seems that the first two cases are already handled by buildInputs, nativeBuildInputs. (The other more advanced, so to speak, depsFooBar parameters can be used if needed). They are the same as the ones used in stdenv.mkDerivation, requires no additional effort to learn if you already knew mkDerivation, and matches up with the idea of 'runtime' and 'build-time' dependencies quite well.

The last use case, however, seems to be the one this PR focuses on, like a nix-shell -p. The user here has no intention of using the mkShell as an environment for compiling/building anything, and just needs a shell with some packages provided.

Is that a correct understanding of the purpose of this pull request?


Additionally, there may be the slight issue that the various nativeBuildInputs etc only work when the packages are passed in by callPackage. Otherwise they are all handled as if they were buildInputs. I'm wondering if this one is relevant to the claim that 'the disctinction between the inputs doesn't really make sense in the mkShell context'.

@zimbatm
Copy link
Member Author

zimbatm commented Aug 6, 2021

It's perfectly fine to use stdenv.mkDerivation for all the use-cases above. mkShell is just supposed to be a little sugar on top.

@dramforever
Copy link
Contributor

I see, thanks for the explanation

m15a added a commit to m15a/flake-fennel-tools that referenced this pull request Mar 30, 2024
Attribute `packages` is actually `nativeBuildInputs`.
See NixOS/nixpkgs#122180
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