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.

Sign up for GitHub

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

Jump to bottom

fix(init): Support dots in final path item #1425

Merged
merged 2 commits into from
May 9, 2024
Merged

fix(init): Support dots in final path item #1425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f1ee6f
fix(init): Add integration test for file path
dcarley May 9, 2024
9e361c2
fix(init): Support dots in final path item
dcarley May 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions cli/Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions cli/flox/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ tracing.workspace = true
url.workspace = true
uuid.workspace = true
xdg.workspace = true
path-dedot = "3.1.1"
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.


[dev-dependencies]
pretty_assertions.workspace = true
Expand Down
2 changes: 2 additions & 0 deletions cli/flox/src/commands/init/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use flox_rust_sdk::models::pkgdb::scrape_input;
use flox_rust_sdk::models::search::{do_search, PathOrJson, Query, SearchParams, SearchResult};
use indoc::formatdoc;
use log::debug;
use path_dedot::ParseDot;
use toml_edit::{DocumentMut, Formatted, Item, Table, Value};
use tracing::instrument;

Expand Down Expand Up @@ -79,6 +80,7 @@ impl Init {
EnvironmentName::from_str(DEFAULT_NAME)?
} else {
let name = dir
.parse_dot()?
Comment on lines 82 to +83
Copy link
Contributor

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.

Copy link
Contributor Author

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.

.file_name()
.map(|n| n.to_string_lossy().to_string())
.context("Can't init in root")?;
Expand Down
30 changes: 30 additions & 0 deletions cli/tests/init.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,28 @@ function check_with_dir() {
assert_output ".flox"
}

@test "c2.1: \`flox init\` with \`--dir .\` will create an environment in current working directory." {
run "$FLOX_BIN" init -d .
assert_success
run ls -A
assert_output ".flox"
}

@test "c2.1: \`flox init\` with \`--dir ..\` will create an environment in parent working directory." {
mkdir -p "$PROJECT_DIR/other"

pushd other
run "$FLOX_BIN" init -d ..
assert_success
popd

run ls -A
assert_output - <<EOF
.flox
other
EOF
}

@test "c2.1: \`flox init\` with \`--dir <path>\` will create an environment in \`<path>\`. (relative)" {
mkdir -p "$PROJECT_DIR/other"

Expand All @@ -118,6 +140,14 @@ function check_with_dir() {
check_with_dir
}

@test "c2.1: \`flox init\` with \`--dir <path>\` will not create an environment where \`<path>\` is a file" {
touch "$PROJECT_DIR/other"

run "$FLOX_BIN" init -d ./other
assert_failure
assert_line --partial "Could not prepare a '.flox' directory: Not a directory"
}

@test "c2.1: \`flox init\` with \`--dir <path>\` will create an environment in \`<path>\`. (absolute)" {
mkdir -p "$PROJECT_DIR/other"

Expand Down
Loading