-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Configure hosts separately from targets when --target is specified. #9322
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
Changes from all commits
083cc9e
e51d151
0a603fd
46f9541
e24bf92
e797526
62e2fd4
6dcfe51
222f6ea
4994b5f
b8be3af
5338bb2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,7 @@ pub struct TargetCfgConfig { | |
pub other: BTreeMap<String, toml::Value>, | ||
} | ||
|
||
/// Config definition of a `[target]` table. | ||
/// Config definition of a `[target]` table or `[host]`. | ||
#[derive(Debug, Clone)] | ||
pub struct TargetConfig { | ||
/// Process to run as a wrapper for `cargo run`, `test`, and `bench` commands. | ||
|
@@ -64,18 +64,62 @@ pub(super) fn load_target_cfgs(config: &Config) -> CargoResult<Vec<(String, Targ | |
Ok(result) | ||
} | ||
|
||
/// Returns true if the `[target]` table should be applied to host targets. | ||
pub(super) fn get_target_applies_to_host(config: &Config) -> CargoResult<bool> { | ||
if config.cli_unstable().target_applies_to_host { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can an entry in the unstable book get added for this? (personally I think it's fine if this is gated by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Added.
Well we require There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you detail more why these should be stabilized separately? I would imagine that there's no need for them to be separate and they should be stabilized at the same time. It seems like it would be odd if we provide a way to configure host/target separately but then we don't provide a way to configure the host. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reason is that we don't want to stabilize There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @alexcrichton This came out of discussions about the path to future stabilization, and the path to changing the defaults, and how those will interlock. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The issue is that the implementation for one of these features(the
Well they are deliberately separated due to the dependency of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think To reiterate my thoughts from before, I disagree that these two features should be stabilized separately. They're pretty intertwined and all the hard stuff with one is still present for the other. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sure.
That by itself isn't the reason for having the features separated, it's a testing issue mostly for both automated tests and those testing with the
To some degree they are intertwined but it's mostly a one way dependency, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I've said many times before, I do not think it is useful to split up these features. I do not think that we can stabilize them on separate schedules, also as I've mentioned before. This means, to me, the only two states of Cargo that we care about are both features on and both features off. Having one feature on and one off is not a useful state in Cargo since we'll never actually ship that to users. This, to me, means it's not worthwhile to test. It's testing functionality that no one should use, so I don't think we should even expose it and that would simplify things. I'm honestly not really entirely certain why you're insistent on keeping these separate? As a maintainer of Cargo I'm offering my thoughts to help guide this implementation, but you seem very resolute in your ways that you want precisely the PR as-is right now. I don't really know how to reconcile that honestly. I've tried to respond to all the points you're bringing up but the discussion seems to be very circular where it's just coming around to the same points again and again. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I guess I'm missing what the issue with this is I guess.
There's still the default change as well, which I also wanted to test, sure, we could combine the others and separate that if you want or just not test the new defaults I supposed but that seemed to me to result in a more confusing migration process as only allowing the host config table feature with the new defaults seem to simplify testing of that feature+default combination.
Well after the new defaults are applied one shouldn't need to set
I'm just trying to clarify the issues I'm running into as it's unclear what the best approach to dealing with them in a satisfactory way is. Well we inherently have two sets of changes either way, the It's unclear how you want me to implement the change though, is either of these approaches what you have in mind:
I think the existing approach is less confusing but either of those would still work fine if that's what you think is best. |
||
if let Ok(target_applies_to_host) = config.get::<bool>("target-applies-to-host") { | ||
Ok(target_applies_to_host) | ||
} else { | ||
Ok(!config.cli_unstable().host_config) | ||
} | ||
} else { | ||
if config.cli_unstable().host_config { | ||
anyhow::bail!( | ||
"the -Zhost-config flag requires the -Ztarget-applies-to-host flag to be set" | ||
); | ||
} else { | ||
Ok(true) | ||
} | ||
} | ||
} | ||
|
||
/// Loads a single `[host]` table for the given triple. | ||
pub(super) fn load_host_triple(config: &Config, triple: &str) -> CargoResult<TargetConfig> { | ||
if config.cli_unstable().host_config { | ||
jameshilliard marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let host_triple_prefix = format!("host.{}", triple); | ||
let host_triple_key = ConfigKey::from_str(&host_triple_prefix); | ||
let host_prefix = match config.get_cv(&host_triple_key)? { | ||
Some(_) => host_triple_prefix, | ||
None => "host".to_string(), | ||
}; | ||
load_config_table(config, &host_prefix) | ||
} else { | ||
Ok(TargetConfig { | ||
runner: None, | ||
rustflags: None, | ||
linker: None, | ||
links_overrides: BTreeMap::new(), | ||
}) | ||
} | ||
} | ||
|
||
/// Loads a single `[target]` table for the given triple. | ||
pub(super) fn load_target_triple(config: &Config, triple: &str) -> CargoResult<TargetConfig> { | ||
load_config_table(config, &format!("target.{}", triple)) | ||
} | ||
|
||
/// Loads a single table for the given prefix. | ||
fn load_config_table(config: &Config, prefix: &str) -> CargoResult<TargetConfig> { | ||
// This needs to get each field individually because it cannot fetch the | ||
// struct all at once due to `links_overrides`. Can't use `serde(flatten)` | ||
// because it causes serde to use `deserialize_map` which means the config | ||
// deserializer does not know which keys to deserialize, which means | ||
// environment variables would not work. | ||
let runner: OptValue<PathAndArgs> = config.get(&format!("target.{}.runner", triple))?; | ||
let rustflags: OptValue<StringList> = config.get(&format!("target.{}.rustflags", triple))?; | ||
let linker: OptValue<ConfigRelativePath> = config.get(&format!("target.{}.linker", triple))?; | ||
let runner: OptValue<PathAndArgs> = config.get(&format!("{}.runner", prefix))?; | ||
let rustflags: OptValue<StringList> = config.get(&format!("{}.rustflags", prefix))?; | ||
let linker: OptValue<ConfigRelativePath> = config.get(&format!("{}.linker", prefix))?; | ||
// Links do not support environment variables. | ||
let target_key = ConfigKey::from_str(&format!("target.{}", triple)); | ||
let target_key = ConfigKey::from_str(prefix); | ||
let links_overrides = match config.get_table(&target_key)? { | ||
Some(links) => parse_links_overrides(&target_key, links.val, config)?, | ||
None => BTreeMap::new(), | ||
|
Uh oh!
There was an error while loading. Please reload this page.