search

TheBevyFlock / bevy_cli

A Bevy CLI tool.
Apache License 2.0
32 stars 6 forks source link

Add wrapper for cargo build command #76

Closed TimJentzsch closed 1 month ago

TimJentzsch commented 1 month ago

Further work towards #24.

This PR adds wrappers to call cargo build.

Note that we are redefining (almost) all arguments that can be passed to cargo build. This may be quite verbose, but it allows us to output a useful help message for bevy build as well as using some of the cargo arguments for our own customization. For example, we need access to the --target and --release arguments.

BD103 commented 1 month ago

Out of curiosity, does wrapping cargo build provide any benefit over just calling cargo directly? (Do you have any plans to enhance this command in the future, or is it just abstraction for abstraction's sake?)

janhohenheim commented 1 month ago

I think if we allow bevy run (which is needed for nicer web builds), it seems kinda expected to also have bevy build, doesn't it? I remember @TimJentzsch mentioning that we should make it possible to create a web-deployable package without running it, so that could go into bevy build

TimJentzsch commented 1 month ago

I think if we allow bevy run (which is needed for nicer web builds), it seems kinda expected to also have bevy build, doesn't it? I remember @TimJentzsch mentioning that we should make it possible to create a web-deployable package without running it, so that could go into bevy build

Exactly. My current implementation of bevy build (which is not contained in this PR yet, but you can see it in #24) mostly calls wasm-bindgen in addition to cargo build, but with #68 it would also support creating an entire bundle for deployment e.g. on itch.io.

BD103 commented 1 month ago

Makes sense, thanks for the clarification!

janhohenheim commented 1 month ago

@BD103 would that not hide the web specific stuff we offer?

BD103 commented 1 month ago

Fair! Maybe the help page can just say: "This command supports all options that can be passed to cargo build." That way, users will know how to configure the command, and it won't clutter up the help screen nearly as much.

TimJentzsch commented 1 month ago

Fair! Maybe the help page can just say: "This command supports all options that can be passed to cargo build." That way, users will know how to configure the command, and it won't clutter up the help screen nearly as much.

The problem is that we also need access to some of the cargo options (such as--profile, --release) and modify some of them (such as target). I think that will get more complicated when we don't define them explicitly.

I also think having them in the help output (and with consistent formatting) is rather an advantage so the user doesn't have to remember the cargo options to apply them.

I agree the code is quite verbose, but do we expect the cargo options to change frequently? And when they change, shouldn't we consider how these changes affect the bevy CLI as well?

janhohenheim commented 1 month ago

@TimJentzsch's argument is quite convincing to me. Voting to keeping it as-is.

BD103 commented 1 month ago

Ok, I'm convinced! I haven't done a full code review, but @janhohenheim feel free to click merge once you think this is ready. :)

janhohenheim commented 1 month ago

@TimJentzsch I took the liberty of renaming command to build_command. The function seems to be unused, I assume that is intentional? Anyways, merging so that this is not sitting in review purgatory :)

TimJentzsch commented 1 month ago

@TimJentzsch I took the liberty of renaming command to build_command. The function seems to be unused, I assume that is intentional? Anyways, merging so that this is not sitting in review purgatory :)

Ah right, in the PR I used the functions like this cargo::build::command()

But I'm fine to change to a different naming scheme :)

The functions will be used in follow up PRs

janhohenheim commented 1 month ago

@TimJentzsch I like that usage! Changing it back.