-
Notifications
You must be signed in to change notification settings - Fork 33
add --bundle-dir option to web subcommand
#650
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
DaAlbrecht
left a comment
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'm open to this but do you think this is still needed even if the Bundle path is part of the API?
| pub wasm_opt: Vec<String>, | ||
|
|
||
| /// Copy packed bundle directory to this directory | ||
| #[arg(long = "bundle-dir", allow_hyphen_values = true)] |
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.
Nit: While technically directories could start with a - I think we can avoid problems by not allowing that, what do you think?
| #[arg(long = "bundle-dir", allow_hyphen_values = true)] | |
| #[arg(long = "bundle-dir")] |
| fs::create_dir_all(target).context("failed to create target directory")?; | ||
| dir::copy(path, target, &CopyOptions::new().content_only(true)) | ||
| .context("failed to copy packed bundle directory to target directory")?; | ||
| } |
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.
Nit: I would like an additional log line that explains that the cli copied the files to the new dir.
| cmd.current_dir(test_path()).args([ | ||
| "build", | ||
| "-p=bevy_default", | ||
| "--release", |
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.
Nit: We could save CI time by not using the release profile here.
| @@ -1,3 +1,6 @@ | |||
| #[cfg(feature = "web")] | |||
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.
Does it make sense to support this for cargo run?
| #[arg(long = "wasm-opt", allow_hyphen_values = true)] | ||
| pub wasm_opt: Vec<String>, | ||
|
|
||
| /// Copy packed bundle directory to this directory |
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.
| /// Copy packed bundle directory to this directory | |
| /// The directory to copy final packed bundle to. Note that a copy of the Bundle can still be found at `target/bevy_web`. |
DaAlbrecht
left a comment
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.
My bad, wanted to just leave some comments, didn't mean to approve already
Add an option for copying a packed web bundle directory to a user-specified directory. This lets you skip any error-prone parsing/guessing when locating the bundle directory, which is usually useful for CI-related tasks.
The behavior is modeled after the unstable
--artifact-diroption fromcargo:--artifact-dir(nee--out-dir) Tracking Issue rust-lang/cargo#6790Fix: #649