-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
shell.nix
: Support nix-shell -A
#353240
shell.nix
: Support nix-shell -A
#353240
Conversation
It used to be that `nix-shell -A hello` would launch the build shell for the `hello` package. By adding `/shell.nix`, that stopped working, as all versions of `nix-shell` resolve the unspecified file to `$PWD/shell.nix` if it exists, and now it does. I have to admit that this use of `//` is not pretty, but the UX/DX hard to match.
Will test this in a couple hours! |
I hate it, but I don't hate it conceptually. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this is ugly, but I can't think of any better way to achieve the desired behaviour.
Thanks for working on this 🚀
curPkgs | ||
// pkgs.mkShellNoCC { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better/worse to use stdenv's passthru
for this instead of a //
update?
I.e. passthru = curPkgs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my testing this seems to do the exact same thing. I feel like passthru
is a slightly nicer alternative to avoid potentially attrset shadowing from //
, not that mkShellNoCC
and pkgs
have anything in common now, but maybe in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're similar, except for two differences:
- the effect of
pkg // foo
is not preserved in the return value ofpkg.overrideAttrs
or any other such functions that re-evaluate the package function //
is simple and not subject to change
For these reasons I would
- use
passthru
in cases where the new attributes relate strongly to the package (in this case the shell) and if the new attrs should be preserved in the return value ofoverrideAttrs
- use
//
in cases where the addition is circumstantial. Another example would be testing a static build of a package in thetests
attribute. Usingpassthru
for that would create an expectation that overrides are applied to that test, but they're not, so it'd be better for such atests
test to be omitted afteroverrideAttrs
.
In this case, calling overrideAttrs
only needs to produce a shell, and not the whole package set, so we don't need passthru
.
I also think //
is nicer because it makes it clear which attrset wins when there's a conflict; the shell, and we don't need to check how mkDerivation
works or test it to confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes work as expected, LGTM!
Thanks everyone! |
It used to be that
nix-shell -A hello
would launch the build shell for thehello
package.By adding
/shell.nix
, that stopped working, as all versions ofnix-shell
resolve the unspecified file to$PWD/shell.nix
if it exists, and now it does.I have to admit that this use of
//
is not pretty, but the UX/DX is hard to match.Fix the issue raised in Development shell with a pinned nixfmt #322512 (comment)
Seems to have some support, based on Legacy
nix-shell
with a path prefers shell.nix over default.nix nix#11699 (comment)Things done
Tested
nix-shell
andnix-shell -A hello
.nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.