-
Notifications
You must be signed in to change notification settings - Fork 30
Append cargo config rustflags to CliConfig
#540
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
Conversation
@@ -53,6 +53,9 @@ dialoguer = { version = "0.11.0", default-features = false } | |||
# Cargo like styling for clap | |||
clap-cargo = "0.16.0" | |||
|
|||
# Load and resolve Cargo configuration. | |||
cargo-config2 = "0.1" |
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.
Some bigger crates depend on this so i think its fine: https://lib.rs/crates/cargo-config2/rev
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.
IMO well worth it to keep consistent with Cargo and handle all the complexity
Co-authored-by: Jan Hohenheim <[email protected]>
src/config.rs
Outdated
let target = if is_web { | ||
"wasm32-unknown-unknown" | ||
} else { | ||
config.host_triple().ok()? |
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.
This seems super useful. For that alone the crate is probably worth it!
src/config.rs
Outdated
@@ -184,6 +184,30 @@ impl CliConfig { | |||
|
|||
self | |||
} | |||
|
|||
pub fn merge_cargo_config_rustflags( |
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 unsure where to put this. First, I had the merging logic inside of CliConfig::rustflags
, but I don't like that because:
- merging rustflag configs is not what I would expect to happen if I create a
CliConfig
. I expect to... just get the config :) - It's harder to test
While not 100% happy, I think having this live inside of CliConfig
is the best place for now
a460a9a
to
ea977d9
Compare
…ustflags` We do not actually merge anything yet so this fits better.
CliConfig
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.
Just one code quality comment, but otherwise it looks good!
Tested and seems to work. Though it should be noted that a |
Oh and another nit, we should check if any documentation should be updated. |
Yes see here: https://doc.rust-lang.org/cargo/reference/config.html#hierarchical-structure |
# Objective Closes #546. Automatically configure the `getrandom` web backend when necessary, to avoid build failures. # Solution - Detect usage of `getrandom` (and whether the backend is applied) via `cargo metadata` - Configure the feature flag via `--config` flags to pass to `cargo build` - Configure the backend by adding to the `RUSTFLAGS` Note: Blocked on #540 to add to the `RUSTFLAGS` instead of overwriting them! # Testing First, install the CLI from this branch: ```sh cargo install --git https://github.com/TheBevyFlock/bevy_cli --branch 546-getrandom-fix --locked bevy_cli ``` Then execute `bevy run web` on a package that doesn't have the `getrandom` backend configured. This most often happens on the in-development Bevy 0.17. One good example is the `breakout` example in the `bevy` repository itself. Try running this: ```sh bevy run --example=breakout web --verbose ``` You should see an info log "automatically configuring `getrandom` web backend" and also see that the fix is applied to the `cargo build` command.
Support merging rustflags from the resolved cargo config together with the rustflags passed in the
CliConfig
using cargo_config2Closes #537