-
Notifications
You must be signed in to change notification settings - Fork 924
feat: replace --mapdir and --map with --volume argument
#6033
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This PR replaces the deprecated --mapdir and --map flags with a new --volume flag that follows Docker's HOST_DIR:GUEST_DIR convention, making the CLI more intuitive and consistent with industry standards. The PR includes helpful deprecation messages to guide users through the transition.
Key changes:
- Renamed flag from
--mapdir/--mapto--volumewith reversed directory order (fromGUEST:HOSTtoHOST:GUEST) - Added user-friendly deprecation notices that explain the change when old flags are used
- Updated all test files and documentation to use the new syntax
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/cli/src/utils/mod.rs | Renamed parse_mapdir to parse_volume with updated parsing logic for the new format |
| lib/cli/src/commands/run/wasi.rs | Replaced pre_opened_directories and mapped_dirs fields with volumes field |
| lib/cli/src/commands/run/mod.rs | Updated references from mapped_dirs to volumes |
| lib/cli/src/commands/mod.rs | Added deprecation notices for --mapdir and --map flags |
| lib/cli/src/commands/wasmer_create_exe_main.c | Added TODO comment noting future port needed |
| lib/cli/Cargo.toml | Added itertools dependency |
| Cargo.lock | Updated lock file with itertools dependency |
| tests/integration/cli/tests/run.rs | Updated all test cases to use new --volume syntax |
| tests/integration/cli/tests/snapshot.rs | Updated mount formatting to use new order |
| tests/integration/cli/tests/create_exe.rs | Updated test and comment to reference --volume |
| tests/wasix/*/run.sh | Updated all shell test scripts to use new syntax |
| tests/wasi-fyi/wasm-test.sh | Updated to use new syntax |
| lib/wasix/tests/README.md | Updated documentation example |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cmd.arg("--volume").arg(format!( | ||
| "{}:{}", | ||
| mount.0.to_str().unwrap(), | ||
| mount.1.to_str().unwrap() |
Copilot
AI
Jan 9, 2026
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.
There's a syntax error in this format string - missing a comma after line 327. The correct format should have a comma between the two arguments to properly format the string as "HOST_DIR:GUEST_DIR".
| mount.1.to_str().unwrap() | |
| mount.1.to_str().unwrap(), |
| [entry] => retrieve_alias_pathbuf(entry, entry), | ||
| [host_dir, guest_dir] => retrieve_alias_pathbuf(host_dir, guest_dir), | ||
| _ => bail!( | ||
| "Directory mappings must consist of a single path, of two paths separate by a `:`. Found {}", |
Copilot
AI
Jan 9, 2026
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.
There's a grammatical error in this error message. It should be "of two paths separated by" instead of "of two paths separate by".
| "Directory mappings must consist of a single path, of two paths separate by a `:`. Found {}", | |
| "Directory mappings must consist of a single path, or two paths separated by a `:`. Found {}", |
| "Directory mappings must consist of two paths separate by a `::` or `:`. Found {}", | ||
| /// Parses a volume from a string | ||
| pub fn parse_volume(entry: &str) -> Result<MappedDirectory> { | ||
| let components = entry.split(":").collect_vec(); |
Copilot
AI
Jan 9, 2026
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.
The split logic using : as a delimiter will not work correctly on Windows when absolute paths contain drive letters (e.g., C:\foo). When a user specifies --volume=C:\foo:D:\bar, this will split into three components ["C", "\foo", "D:\bar"] instead of two. Consider using splitn(2, ':') instead of split(":") to ensure only the first colon is used as a delimiter, or add Windows-specific path handling.
| let components = entry.split(":").collect_vec(); | |
| let mut components = entry.rsplitn(2, ':').collect_vec(); | |
| components.reverse(); |
| .arg("python/python") | ||
| .arg("--mapdir") | ||
| .arg("--volume") | ||
| .arg(format!("/code:{}", resources.display())) |
Copilot
AI
Jan 9, 2026
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.
The volume mapping format is incorrect. According to the PR description, the new --volume flag should use HOST_DIR:GUEST_DIR convention (like Docker). This line uses /code:{} which is GUEST:HOST order. It should be {}:/code to match the correct HOST:GUEST order used elsewhere in this file.
| .arg(format!("/code:{}", resources.display())) | |
| .arg(format!("{}:/code", resources.display())) |
Currently, we support two arguments that largely overlap in functionality (
--mapand--mapdir), and the--mapdirsyntax is especially counter-intuitive since it uses GUEST_DIR:HOST_DIR. Following the earlier discussion with Syrus, we should consolidate these flags and adopt the same convention as docker run by introducing a single--volumeargument.Because this is a breaking change, we need to communicate it clearly to users. For example: