-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Evaluate nix-shell -i args relative to script #5088
Evaluate nix-shell -i args relative to script #5088
Conversation
What about the
Or is there a preferred alternative way to set the search path? |
I marked this as stale due to inactivity. → More info |
Can this get reviewed? This still is an issue in Nix 2.9.1. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2023-03-03-nix-team-meeting-minutes-37/25998/1 |
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.
This is a net-positive change. Please add release notes as this is a behavior change.
f512ebe
to
6404e2c
Compare
Fixed conflicts, but this also needs tests. |
Please un-draft when ready, this is easier for maintainers to keep overview. |
1b9087d
to
824f50e
Compare
824f50e
to
72fab52
Compare
@matthewbauer I added tests and notes, let me know if thit is what you intended Fixes: #5431 I have some hesitation due to the this being a change in behavior for something that has been around for a while. (also, see the new nix-command shebang just released) |
The worry is someone may have started to depend on these being relative to the call-site. Unfortunate, but might need a flag to control the behavior for the legacy CLI, "--relative"? The new nix-command should not have this design oddity. |
I think this is improbable and a bit of a bike shed. The workarounds (haskell plus bash polyglot! horrible) that people have had to apply would not be broken by this, as they start nix-shell in the same directory every time. I cannot see any conceivable reason someone would want the old behaviour: somehow you would be evaluating a script with respect to the current directory's default.nix? That feels like a use case so contrived I doubt anyone has actually tried it, compared to the 99% use case that doesn't work today and I don't believe the new CLI supports at all. |
When writing a shebang script, you expect your path to be relative to the script, not the cwd. We previously handled this correctly for relative file paths, but not for expressions. This handles both -p & -E args. My understanding is this should be what we want in any cases I can think of - people run scripts from many different working directories. @edolstra is there any reason to handle -p args differently in this case? Fixes NixOS#4232
It seems that this would break (not difficult to fix): https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/ruby-modules/with-packages/test.rb#L61 I am slightly inclined to merge as-is due the niche nature of the breakage, but the proposal to add "--relative" was mentioned during discussion and I thought it worth bringing up for additional comments. |
8801d7d
to
1169b75
Compare
Strongly in favor of merging and deferring backwards compatibility hacks to the improbable case of a spacebar heating outcry. |
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.
Some clarifications, because I had a hard time untangling what the release notes were trying to convey.
@roberth are we holding it correctly? Seems weird that there are two places for release notes now.
prs: #5088 | ||
description: { | ||
|
||
`nix-shell` shebangs use relative paths for files, but expressions were still evaluated using the current working directory. The new behavior is that evalutations are performed relative to the script. |
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.
`nix-shell` shebangs use relative paths for files, but expressions were still evaluated using the current working directory. The new behavior is that evalutations are performed relative to the script. | |
`nix-shell` shebangs use the script file's location to resolve relative paths to files passed as command line arguments, but expression arguments were evaluated using the current working directory as a base path. | |
The new behavior is that Nix language expressions passed to `nix-shell` when used in a `#!` context are now evaluated relative to the script's file system location. |
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.
This is a breaking change for example if you have a repo with a scripts
directory and you invoke them as
$ ./scripts/foo
in which case the foo
script likely has a workaround for this issue,
#!/usr/bin/env nix-shell
#!nix-shell --expr 'import ./scripts/shell.nix { withBar = true; }'
which will cause it to break when Nix is updated to a version after this PR.
error: getting status of '/home/user/src/unicorn/scripts/scripts/shell.nix': No such file or directory
Note the double scripts/
.
I don't know if we should break an established command like this.
We should take this seriously and add --relative
to opt in to the new behavior.
For what it's worth, it's also a way to "declare" that a recent Nix is needed by the script.
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.
prs: #5088 | ||
description: { | ||
|
||
`nix-shell` shebangs use relative paths for files, but expressions were still evaluated using the current working directory. The new behavior is that evalutations are performed relative to the script. |
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.
This is a breaking change for example if you have a repo with a scripts
directory and you invoke them as
$ ./scripts/foo
in which case the foo
script likely has a workaround for this issue,
#!/usr/bin/env nix-shell
#!nix-shell --expr 'import ./scripts/shell.nix { withBar = true; }'
which will cause it to break when Nix is updated to a version after this PR.
error: getting status of '/home/user/src/unicorn/scripts/scripts/shell.nix': No such file or directory
Note the double scripts/
.
I don't know if we should break an established command like this.
We should take this seriously and add --relative
to opt in to the new behavior.
For what it's worth, it's also a way to "declare" that a recent Nix is needed by the script.
I think it's great that we don't dogmatically only work on the new CLI, but we should take the old CLI more seriously when we change it. |
Agreed on being especially careful about stable commands, but we also quite clearly said that what's obviously a bug (in the sense of an implementation error rather than a flawed design) should be just fixed. This is the case here. The current behavior is unintentionally inconsistent. |
1169b75
to
f66f498
Compare
When writing a shebang script, you expect your path to be relative to
the script, not the cwd. We previously handled this correctly for
relative file paths, but not for expressions.
This handles both -p & -E args. My understanding is this should be
what we want in any cases I can think of - people run scripts from
many different working directories. @edolstra is there any reason to
handle -p args differently in this case?
Fixes #4232