-
Notifications
You must be signed in to change notification settings - Fork 33
Enabled required features from examples automatically #636
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
|
Yay! That's very nice. |
BD103
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 haven't had a chance to look over the code yet, but could you please add some basic docs on this feature? I don't want to diverge from Cargo without documenting it :)
yea good point! Alice mentioned she would like a yes/no prompt i think that would be self documenting and no longer silently diverge from cargo what do you think? |
While I think a prompt would be good with users who are unaware of the feature, I think it would also be really annoying for users who are aware and actively want the example's required features to be enabled. Maybe a good alternative would be switching the But I also don't think that replaces having documentation on our website. A user shouldn't have to install the CLI or read the changelog to see all the features supported by a program. I can draft up something if you want me to! |
I added a section but I always appreciate your docs/writing style, so feel free to delete it ^^ |
BD103
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.
Very good, the implementation is especially clean after #622 was merged!
| args.cargo_args | ||
| .feature_args | ||
| .features | ||
| .push("--all-features".to_owned()); | ||
|
|
||
| info!("automatically added `--all-features` to build examples with all features enabled"); |
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 not sure we should always enable --all-features when --examples is passed. When I run cargo build --examples --all-features in Bevy's repo, it fails with a compile error. That's not a great user experience, especially when the primary use of this feature will be by Bevy devs.
In @alice-i-cecile's original comment when I asked how to deal with --examples and --all-targets, she said:
Based on the design proposed in the linked issue, I would build these with
--all-features(uh, except where mutually incompatible features exist) and diverge fromcargo. Or at least warn users that they probably want to enable all the features and try again.
I think we either need to be more intelligent with how we handle mutually incompatible features, or just cut this for now. The alternative is Bevy devs being unable to compile the Bevy workspace with bevy build --examples, having to switch to cargo build --examples, which I don't think is acceptable.
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.
If we update this, make sure to also update the section in examples.md and lint/mod.rs.
| - [**Build and running Bevy Web Apps**](web.md) | ||
| - [**Linter Integration**](linter.md) | ||
| - [**Enabling required features for examples**](examples.md) |
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.
| - [**Build and running Bevy Web Apps**](web.md) | |
| - [**Linter Integration**](linter.md) | |
| - [**Enabling required features for examples**](examples.md) | |
| - [**Build and Running Bevy Web Apps**](web.md) | |
| - [**Linter Integration**](linter.md) | |
| - [**Enabling Required Features for Examples**](examples.md) |
(Nit) Let's make all of these title case
| ```sh | ||
| # Run the `web_asset` example from https://github.com/bevyengine/bevy that requires the feature `https`. | ||
| bevy run --example web_asset | ||
| info: enabling required_features: ["https"], for example: web_asset | ||
|
|
||
| # Build the `web_asset` example in the web. | ||
| bevy build --example web_asset web | ||
| info: enabling required_features: ["https"], for example: web_asset | ||
| ``` |
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.
| ```sh | |
| # Run the `web_asset` example from https://github.com/bevyengine/bevy that requires the feature `https`. | |
| bevy run --example web_asset | |
| info: enabling required_features: ["https"], for example: web_asset | |
| # Build the `web_asset` example in the web. | |
| bevy build --example web_asset web | |
| info: enabling required_features: ["https"], for example: web_asset | |
| ``` | |
| Take Bevy's `web_asset` example, for instance. It requires the `https` feature in `Cargo.toml`: | |
| ```toml | |
| [[example]] | |
| name = "web_asset" | |
| path = "examples/asset/web_asset.rs" | |
| required-features = ["https"] | |
| ``` | |
| Running `cargo build --example web_asset` will fail with Cargo complaining that the `https` feature was not enabled. The Bevy CLI differs by automatically enabling the feature for you: | |
| ```sh | |
| # The CLI will automatically add `--feature https`, as that feature is required to run the example. | |
| bevy run --example web_asset | |
| # It also works when building for the web. | |
| bevy build --example web_asset web | |
| ``` |
I think it would be good to show how required features are specified in Cargo.toml :)
Objective
Enable the required features from examples automatically. If all examples should be built/checked, enable
--all-features.Testing
Note
Building and lining all examples does not work until #629
Closes #622