-
Notifications
You must be signed in to change notification settings - Fork 51
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
fix(init): Support dots in final path item #1425
Conversation
Test the existing behaviour of running `init` against a path that is a file because I don't want to affect the user facing error message with an intermediate error when implementing support for `-d .`
Resolve any dots in paths when determining the environment name so that `init -d` works when the final path item is a single or double dot. We can't use `canonicalize` because it will return an error if the path doesn't exits which breaks our existing behaviour for init against a directory that doesn't yet exist: - https://doc.rust-lang.org/std/path/struct.Path.html#method.canonicalize The following implementations all don't resolve a single `.` to a name: - https://doc.rust-lang.org/std/path/fn.absolute.html - https://docs.rs/path-absolutize/latest/path_absolutize/ - https://docs.rs/path-clean/latest/path_clean/ - https://github.com/rust-lang/cargo/blob/0.79.0/crates/cargo-util/src/paths.rs#L76-L109 So the `path-dedot` crate seemed like the most appropriate choice. I considered adding unit tests for this behaviour but the integration tests are actually very fast for this functionality, make it simpler to test changing current working directory in the context of running the CLI, and give us some reassurance that we haven't affected the downstream behaviour of creating an environment. Output of the new tests before the implementation was added: ✗ c2.1: `flox init` with `--dir .` will create an environment in current working directory. [37] tags: init (from function `assert_success' in file /nix/store/dhghqmkjk7fqpdbz71clvbh5zxp6p3lf-bats-with-libraries-1.10.0/share/bats/bats-assert/src/assert_success.bash, line 42, in test file init.bats, line 115) `assert_success' failed ❌ ERROR: Can't init in root -- command failed -- status : 1 output : ❌ ERROR: Can't init in root -- Last output: ❌ ERROR: Can't init in root ✗ c2.1: `flox init` with `--dir ..` will create an environment in parent working directory. [39] tags: init (from function `assert_success' in file /nix/store/dhghqmkjk7fqpdbz71clvbh5zxp6p3lf-bats-with-libraries-1.10.0/share/bats/bats-assert/src/assert_success.bash, line 42, in test file init.bats, line 125) `assert_success' failed /var/folders/8q/spckhr654cv4xrcv0fxsrlvc0000gn/T/nix-shell.VLMq5i/bats-run-ZpmkUP/test/9/test/other /var/folders/8q/spckhr654cv4xrcv0fxsrlvc0000gn/T/nix-shell.VLMq5i/bats-run-ZpmkUP/test/9/test /var/folders/8q/spckhr654cv4xrcv0fxsrlvc0000gn/T/nix-shell.VLMq5i/flox-cli-tests-2WdbV0 ❌ ERROR: Can't init in root -- command failed -- status : 1 output : ❌ ERROR: Can't init in root -- Last output: ❌ ERROR: Can't init in root
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 being an issue since 2017 and still not part of std is mind boggling to me everytime i stumble over this :D
@@ -49,6 +49,7 @@ tracing.workspace = true | |||
url.workspace = true | |||
uuid.workspace = true | |||
xdg.workspace = true | |||
path-dedot = "3.1.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.
What's curious to me is that this appears to be the same crate as https://github.com/magiclen/path-absolutize (same author as this: https://github.com/magiclen/path-dedot).
Though the former seems to be at least partially a wrapper around this crate.
Makes me a bit suspicious but may be not too noteworthy.
If we're truly concerned, the meat of this is a single ~100loc function, or we copy in the 25loc cargo implementation which does less but arguably enough if its enough for cargo.
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.
Makes me a bit suspicious but may be not too noteworthy.
Suspicious in what kind of way, like a supply chain attack? 🤔
or we copy in the 25loc cargo implementation which does less but arguably enough if its enough for cargo
It doesn't do what we need which is to resolve the path without any .
or ..
so that we can take a name from the final path item.
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.
For the cargo one you can join the oath with fwd first if it's relative or otherwise normalize if it's already absolute but🤔
And yea suspicious for sums supply chain attacks. Not that I have any reason to expect anything any more than with other crates. Just the fact that this library is in crates.io twice for some reason is unusual that's all
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.
For the cargo one you can join the oath with fwd first if it's relative or otherwise normalize if it's already absolute but🤔
Then we've implemented some portion of path-dedot
(albeit without the caching that we don't use/need) and need our own unit tests around a separate function. My one takeaway from looking at all of the issues and implementations is that correct parsing is deceptively un-trivial and it'd be easier to use something off-the-shelf.
And yea suspicious for sums supply chain attacks. Not that I have any reason to expect anything any more than with other crates. Just the fact that this library is in crates.io twice for some reason is unusual that's all
Gotcha. I think it's OK, in that path-absolutize
is a super-set of path-dedot
, and just happens to expose it if you want access to both.
let name = dir | ||
.parse_dot()? |
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.
nb: while this mitigates the error we're seeing, we could normalize the path earlier either for this method (up in L74) or at the argument parsing level.
Since relative paths have yet to cause issues outside of this context we may as well keep it local here.
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.
It also makes sense to use and report the original path in any error messages so that it matches the user's CLI arguments.
## Proposed Changes Resolve any dots in paths when determining the environment name so that `init -d` works when the final path item is a single or double dot. We can't use `canonicalize` because it will return an error if the path doesn't exits which breaks our existing behaviour for init against a directory that doesn't yet exist: - https://doc.rust-lang.org/std/path/struct.Path.html#method.canonicalize The following implementations all don't resolve a single `.` to a name: - https://doc.rust-lang.org/std/path/fn.absolute.html - https://docs.rs/path-absolutize/latest/path_absolutize/ - https://docs.rs/path-clean/latest/path_clean/ - https://github.com/rust-lang/cargo/blob/0.79.0/crates/cargo-util/src/paths.rs#L76-L109 So the `path-dedot` crate seemed like the most appropriate choice. I considered adding unit tests for this behaviour but the integration tests are actually very fast for this functionality, make it simpler to test changing current working directory in the context of running the CLI, and give us some reassurance that we haven't affected the downstream behaviour of creating an environment. Output of the new tests before the implementation was added: ✗ c2.1: `flox init` with `--dir .` will create an environment in current working directory. [37] tags: init (from function `assert_success' in file /nix/store/dhghqmkjk7fqpdbz71clvbh5zxp6p3lf-bats-with-libraries-1.10.0/share/bats/bats-assert/src/assert_success.bash, line 42, in test file init.bats, line 115) `assert_success' failed ❌ ERROR: Can't init in root -- command failed -- status : 1 output : ❌ ERROR: Can't init in root -- Last output: ❌ ERROR: Can't init in root ✗ c2.1: `flox init` with `--dir ..` will create an environment in parent working directory. [39] tags: init (from function `assert_success' in file /nix/store/dhghqmkjk7fqpdbz71clvbh5zxp6p3lf-bats-with-libraries-1.10.0/share/bats/bats-assert/src/assert_success.bash, line 42, in test file init.bats, line 125) `assert_success' failed /var/folders/8q/spckhr654cv4xrcv0fxsrlvc0000gn/T/nix-shell.VLMq5i/bats-run-ZpmkUP/test/9/test/other /var/folders/8q/spckhr654cv4xrcv0fxsrlvc0000gn/T/nix-shell.VLMq5i/bats-run-ZpmkUP/test/9/test /var/folders/8q/spckhr654cv4xrcv0fxsrlvc0000gn/T/nix-shell.VLMq5i/flox-cli-tests-2WdbV0 ❌ ERROR: Can't init in root -- command failed -- status : 1 output : ❌ ERROR: Can't init in root -- Last output: ❌ ERROR: Can't init in root ## Release Notes Support `flox init -d` where the final part of the path is a `.` or `..`.
## Proposed Changes Resolve any dots in paths when determining the environment name so that `init -d` works when the final path item is a single or double dot. We can't use `canonicalize` because it will return an error if the path doesn't exits which breaks our existing behaviour for init against a directory that doesn't yet exist: - https://doc.rust-lang.org/std/path/struct.Path.html#method.canonicalize The following implementations all don't resolve a single `.` to a name: - https://doc.rust-lang.org/std/path/fn.absolute.html - https://docs.rs/path-absolutize/latest/path_absolutize/ - https://docs.rs/path-clean/latest/path_clean/ - https://github.com/rust-lang/cargo/blob/0.79.0/crates/cargo-util/src/paths.rs#L76-L109 So the `path-dedot` crate seemed like the most appropriate choice. I considered adding unit tests for this behaviour but the integration tests are actually very fast for this functionality, make it simpler to test changing current working directory in the context of running the CLI, and give us some reassurance that we haven't affected the downstream behaviour of creating an environment. Output of the new tests before the implementation was added: ✗ c2.1: `flox init` with `--dir .` will create an environment in current working directory. [37] tags: init (from function `assert_success' in file /nix/store/dhghqmkjk7fqpdbz71clvbh5zxp6p3lf-bats-with-libraries-1.10.0/share/bats/bats-assert/src/assert_success.bash, line 42, in test file init.bats, line 115) `assert_success' failed ❌ ERROR: Can't init in root -- command failed -- status : 1 output : ❌ ERROR: Can't init in root -- Last output: ❌ ERROR: Can't init in root ✗ c2.1: `flox init` with `--dir ..` will create an environment in parent working directory. [39] tags: init (from function `assert_success' in file /nix/store/dhghqmkjk7fqpdbz71clvbh5zxp6p3lf-bats-with-libraries-1.10.0/share/bats/bats-assert/src/assert_success.bash, line 42, in test file init.bats, line 125) `assert_success' failed /var/folders/8q/spckhr654cv4xrcv0fxsrlvc0000gn/T/nix-shell.VLMq5i/bats-run-ZpmkUP/test/9/test/other /var/folders/8q/spckhr654cv4xrcv0fxsrlvc0000gn/T/nix-shell.VLMq5i/bats-run-ZpmkUP/test/9/test /var/folders/8q/spckhr654cv4xrcv0fxsrlvc0000gn/T/nix-shell.VLMq5i/flox-cli-tests-2WdbV0 ❌ ERROR: Can't init in root -- command failed -- status : 1 output : ❌ ERROR: Can't init in root -- Last output: ❌ ERROR: Can't init in root ## Release Notes Support `flox init -d` where the final part of the path is a `.` or `..`.
Proposed Changes
Resolve any dots in paths when determining the environment name so that
init -d
works when the final path item is a single or double dot.We can't use
canonicalize
because it will return an error if the pathdoesn't exits which breaks our existing behaviour for init against a
directory that doesn't yet exist:
The following implementations all don't resolve a single
.
to a name:So the
path-dedot
crate seemed like the most appropriate choice.I considered adding unit tests for this behaviour but the integration
tests are actually very fast for this functionality, make it simpler to
test changing current working directory in the context of running the
CLI, and give us some reassurance that we haven't affected the
downstream behaviour of creating an environment.
Output of the new tests before the implementation was added:
Release Notes
Support
flox init -d
where the final part of the path is a.
or..
.