Copy link

Contributor

r? @alexcrichton

(rust-highfive has picked a reviewer for you, use r? to override)

jonhoo commented 18 days ago

PR failures are due to new clap 3.1.0 deprecations combined with -Dwarnings. Fixing separately in #10396.

jonhoo commented 17 days ago

/cc @jameshilliard too since you contributed the original feature :)

jameshilliard commented 17 days ago

1. This changes the interface for target-applies-to-host so that it does not need to be specified twice to be used. And it can still be set through configuration files using the [unstable] table. However, we may(?) want to pick a stable format for in-file configuration of this setting unless we intend for it to only ever be a command-line flag.

Why did you remove the stable format? The only reason you have to specify target-applies-to-host twice is because the nightly feature is not yet stabilized.

jameshilliard commented 17 days ago

3. It's not entirely clear how target-applies-to-host-kind should interact with -Zmultitarget. If, for example, requested_kinds = [HostTarget, SomeOtherTarget] and kind.is_host(), should RUSTFLAGS take effect or not? For the time being I've just hard-coded the behavior for single targets, and the answer would be "no".

Target configs are always triple specific, so setting the config flag target-applies-to-host = true is just intended to use/apply the target specific triple configs as if they were also host triple specific configs, the idea is you would basically get equivalent behavior by copying the target triple configs manually as host triple configs(this is effectively cargo's current default behavior). Setting target-applies-to-host = true should never apply a target triple config unless the host triple being built matches the target triple config.

jonhoo commented 17 days ago

I gave the original discussion for the feature another reading, and I agree with you that having the top-level configuration option is probably the way to go. It has a similar flavor to resolver = "2", where we may want to flip the default in the future. So I've restored the past logic in that regard

The reason this is hairy is because the behavior of Cargo today is inconsistent. Only linker and runner from [target.<host triple>] are applied to host artifacts, but not rustflags. The way I've represented that in the latest revision of this PR that I just pushed is that target-applies-to-host really has three states: unset, true, and false. When unset, the current (inconsistent) behavior is preserved. When set to true, everything from [target.<host triple>] is respected for host artifacts. And when set to true, [target.<host triple>] is ignored for host artifacts. That strikes the right balance to me at least.

I'm personally also not in favor of having -Zhost-config change the default for target-applies-to-host. Instead, I'd prefer for -Zhost-config to produce an error if target-applies-to-host isn't set to false. That way we start to incentivize affected users to put target-applies-to-host = false in their configuration files. But I've left that logic as-is for now.

jameshilliard commented 16 days ago

The reason this is hairy is because the behavior of Cargo today is inconsistent.

Yeah, it's def a hairy mess.

Only linker and runner from [target.] are applied to host artifacts, but not rustflags.

Ugh, that's pretty bad, I only tested the linkers myself since that's what was causing build errors for us. That should probably just be fixed outright as it's clearly a bug.

When unset, the current (inconsistent) behavior is preserved.

IMO the best behavior when unset would be to apply target triples to host if --target is not set but don't apply if it is set. In addition if a host table is present without --target set I would have it override that and disable apply target configs to host.

So defaults like this for cases where target-applies-to-host is not explicitly specified is probably the least likely to break configurations IMO:

1. if --target unset && no host table present => target-applies-to-host = true
2. if --target unset && host table present => target-applies-to-host = false
3. if --target set => target-applies-to-host = false

And when set to true, [target.<host triple>] is ignored for host artifacts.

I assume you mean when set to false here right?

I'm personally also not in favor of having -Zhost-config change the default for target-applies-to-host. Instead, I'd prefer for -Zhost-config to produce an error if target-applies-to-host isn't set to false.

Well the main reason for having it change the default was mostly to minimize the amount of combinations(branch conditions essentially) to test, I only wanted the host table feature to be tested with the new defaults for target-applies-to-host. By having it change the default it cuts down the amount of combination testing needing as you then no longer have to test the host table combined with old defaults.

jonhoo commented 16 days ago

Only linker and runner from [target.] are applied to host artifacts, but not rustflags.

Ugh, that's pretty bad, I only tested the linkers myself since that's what was causing build errors for us. That should probably just be fixed outright as it's clearly a bug.

When unset, the current (inconsistent) behavior is preserved.

IMO the best behavior when unset would be to apply target triples to host if --target is not set but don't apply if it is set.

I don't think we can just fix it, sadly, as that would likely break a bunch of cross-compiling users. Hence the "legacy" mode this PR implements. I think we almost certainly have to preserve backwards compatibility here, or cause a bunch of unnecessary churn for users. Though if the Cargo team is okay with the breakage, I'm okay with removing the legacy mode. I just assumed they wouldn't be.

In addition if a host table is present without --target set I would have it override that and disable apply target configs to host.

So defaults like this for cases where target-applies-to-host is not explicitly specified is probably the least likely to break configurations IMO:

1. if `--target` unset && no host table present => `target-applies-to-host = true`

2. if `--target` unset && host table present => `target-applies-to-host = false`

3. if `--target` set => `target-applies-to-host = false`

I really dislike dynamic defaults, since they tend to be hard to discover and hard to debug. I'd much rather (as this PR implements) have -Zhost-config require target-applies-to-host = false. I also don't think we can change today's behavior with respect to --target being set/unset except at something like an edition boundary, since a fair number of users probably rely on the current behavior (though here, too, if the Cargo team is okay with the breakage, we should do it). Which then leads to the choices this PR makes:

1. If -Zhost-config and target-applies-to-host != false, error.
2. If target-applies-to-host is true, pick up linker, runner, and rustflags (or, really, all keys) from target.<host> for host artifacts.
3. If target-applies-to-host is false, do not pick up target.<host> for host artifacts.
4. If target-applies-to-host is unset (which implies no host table), pick up linker, runner, and any future fields from target.<host> for host artifacts. Pick up rustflags only if --target isn't passed.

Then, come next edition, we make target-applies-to-host = "false" the default. And maybe the edition after we remove target-applies-to-host as a user-visible configuration option.

And when set to true, [target.<host triple>] is ignored for host artifacts.

I assume you mean when set to false here right?

Sorry, yes, false.

I'm personally also not in favor of having -Zhost-config change the default for target-applies-to-host. Instead, I'd prefer for -Zhost-config to produce an error if target-applies-to-host isn't set to false.

Well the main reason for having it change the default was mostly to minimize the amount of combinations(branch conditions essentially) to test, I only wanted the host table feature to be tested with the new defaults for target-applies-to-host. By having it change the default it cuts down the amount of combination testing needing as you then no longer have to test the host table combined with old defaults.

The number of combinations should be the same, since = true is just outright disallowed with -Zhost-config.

jameshilliard commented 16 days ago

I don't think we can just fix it, sadly, as that would likely break a bunch of cross-compiling users.

We're talking about fixing the rustflags bug in the nightly only host-config feature right? Isn't the entire point of the feature being nightly only to allow breaking changes/fixes?

Hence the "legacy" mode this PR implements. I think we almost certainly have to preserve backwards compatibility here, or cause a bunch of unnecessary churn for users. Though if the Cargo team is okay with the breakage, I'm okay with removing the legacy mode. I just assumed they wouldn't be.

I'm highly skeptical changing the target-applies-to-host defaults would be all that problematic mostly since the target-applies-to-host = false default behavior already effectively exists for target triple != host triple cross compilation scenarios.

But this target-applies-to-host default change is an entirely separate issue AFAIU from fixing the host-config rustflags bug, I think it's probably best to fix the rustflags bug separately then worry about the defaults/stabilization path for the host-config and target-applies-to-host features after that's taken care of.

Build scripts typically don't usually need custom build flags at all to function(we don't even need/use the host-config feature in buildroot, we only use the target-applies-to-host feature to override the existing default buggy target-applies-to-host = true behavior so that our x86_64 cross builds don't break due to linker errors) AFAIU in most cases build scripts just need to be able to execute on the host and cargo doesn't even have a way to provide a host artifact specific configs without enabling -Zhost-config anyways, only triple specific configs are supported in stable toolchains which are inherently buggy for host artifact cross compilation configuration.

Arguably anyone cross compiling who has a config that changing this default breaks has a buggy config already, they just happen to have things working by accident, either way the point of the target-applies-to-host config flag is to provide an escape hatch to restore the previous buggy cross compilation behavior for anyone who actually depends on it.

From my understanding the Cargo team is fine with this approach provided there is a transition period before changing the default(although I personally don't actually think this is necessary since fixing the buggy behavior is only likely to break obscure configurations anyways).

I really dislike dynamic defaults, since they tend to be hard to discover and hard to debug.

I mean, if the goal is to have host triple == target triple cross compilation work as expected by default while breaking the least amount of existing configs then dynamic defaults here is probably the least terrible option.

I'd much rather (as this PR implements) have -Zhost-config require target-applies-to-host = false.

What are you trying to do here? Requiring a config option that's intended to be stabilized in order to enable setting a nightly gate for a feature also intended to be stabilized makes no sense to me.

Setting -Zhost-config isn't something that will do anything when the host config feature is stabilized, you would use the host config options once stabilized just by adding a host config table, -Zhost-config is a nightly feature gate option that will be removed.

I also don't think we can change today's behavior with respect to --target being set/unset except at something like an edition boundary, since a fair number of users probably rely on the current behavior (though here, too, if the Cargo team is okay with the breakage, we should do it).

Either way this seems to be something separate from the host.rustflags fix. Can we not fix the host.rustflags bug in the nightly host-config feature and then worry about --target behavior separately? The host/target split logic in cargo is rather complex/confusing in general and having all these changes intertwined makes things difficult to review here.

Which then leads to the choices this PR makes:

1. If `-Zhost-config` and `target-applies-to-host != false`, error.

This doesn't make sense since -Zhost-config is just a nightly feature gate flag that will not even exist when running builds with stable toolchains.

2. If `target-applies-to-host` is `true`, pick up `linker`, `runner`, and `rustflags` (or, really, all keys) from `target.<host>` for host artifacts.

This sounds fine.

3. If `target-applies-to-host` is `false`, do not pick up `target.<host>` for host artifacts.

This sounds fine.

4. If `target-applies-to-host` is unset (which implies no host table), pick up `linker`, `runner`, and any future fields from `target.<host>` for host artifacts. Pick up `rustflags` only if `--target` _isn't_ passed.

This is extremely confusing, AFAIU isn't not even possible to set host artifact specific rustflags at all without this fix, or any host artifact specific config options at all(only triple specific flags seem possible without nightly features) for that matter without enabling the nightly -Zhost-config feature gate.

The report #10206 that this fixes seems to be entirely referencing a bug in the nightly host-config feature and not something applicable to a stabilized feature, which should be able to be fixed without a migration period as that feature is not stabilized yet.

Is there also a different bug with rustflags for target triple config flags(ie without -Zhost-config being used at all).

Then, come next edition, we make target-applies-to-host = "false" the default. And maybe the edition after we remove target-applies-to-host as a user-visible configuration option.

I think the important part is just changing the default before we stabilize the host-config feature to reduce the amount of potential branch conditions we have to handle/test.

The number of combinations should be the same, since = true is just outright disallowed with -Zhost-config.

We want to test the host-config feature behavior as if it were already stabilized. You appear to be effectively trying to test the -Zhost-config nightly feature gating logic which isn't particularly useful as nightly feature gating isn't applicable to features once stabilized at all.

jonhoo commented 11 days ago

@alexcrichton I believe I've modified this to be more in line with your expectations now. As mentioned above, I think we should adjust the -Z flag and (potentially) change the logic in get_target_applies_to_host in a separate PR, so I've reverted my changes related to those two in this PR.

jonhoo commented 11 days ago

Is the latest patch more in line with what you were expecting?

alexcrichton commented 10 days ago

Ok I think I understand what you mean by the tri-state behavior, I never fully understood that but seeing the test cases now and how they're working clarifies this for me at least. My main priority in reviewing all of this rustflags/target/host related stuff is to ensure that breakage is quanitified and either mitigated or deemed acceptable. In that sense I'm pretty confident that this PR doesn't change the current behavior, which is good in the sense that it mitigates possible breakage. For the future, though, I think we'll want the tri-state idea but not necessarily super-well-supported.

I don't think anyone will really argue that today's behavior is correct and should be codified into Cargo for all time. Today's behavior is the amalgamation of growing over years as PRs touched various portions and we as Cargo reviewers didn't do a great job of seeing how everything fits together. What's done is done though, we move past it. This I think means that the stabilization story for target-applies-to-host may be a bit different though. In my mind the "tri-state" is "not configured", true, and false. The "not configured" case is whatever Cargo does today to prevent breaking any existing builds. In the future I was assuming we would stabilize as true and then in the future switch to false at some point later. I believe, though, that at least in my desired interpretation of true then stabilizing as true would be a breaking change because as noted here some tests aren't behaving as I'd expect (where when you say target-applies-to-host that doesn't actually happen). Instead this may mean that we need to bite the bullet on breakage and simply stabilize the false option first (and maybe even just switch internal defaults and leave the target-applies-to-host option unstable).

Basically what I'm trying to do is to chart a course from where we are to where we want to be with minimal breakage to the ecosystem. Ideally this would mean that at any time if someone hits a breakage they can flip a switch to go back to some previous behavior or something like that. I've been working under the assumption that this is possible but it may simply not be possible. This may all simply mean that breakage is unavoidable and if the breakage doesn't work for someone all we can say is "sorry you can hold back on upgrading Cargo for a bit while the breakage is worked through".

jonhoo commented 10 days ago

I completely agree — that matches my thinking on the subject.

I think the right question then becomes is "how hard is mitigation". If target-applies-to-host = false is made the default, then that will break users with no recourse until [host] is stabilized — they'll be unable to set the rustflags and linker for build scripts when --target is passed. Which seems untenable to me. We could perhaps (as you suggested) group target-applies-to-host and host-config together under a single -Z so they land together, but I'll leave that up to you. In the meantime, I actually wonder if the right thing to do short-term is to change the default to = true so that Cargo's behavior is at least consistent, and so that there is a way to accomplish what users might want, even if it's not exactly the behavior we think is least likely to cause problems.

In any case, I also agree with you that this PR doesn't change current behavior beyond fixing #10206, and so should be possible to safely land.

alexcrichton commented 6 days ago

@bors: r+

Ok well we can get all this sorted out during the stabilization of the features here. I agree this is fine to go for now. Personally I continue to feel that these unstable features are also gated on the fact that cargo build is "all host" rather than the expected "some host some target" builds, because if we were to stabilize target-applies-to-host = false, which I think is what we want, then RUSTFLAGS="" cargo build wouldn't actually apply any flags anywhere.

bors commented 6 days ago

Commit 56db829 has been approved by alexcrichton

bors commented 6 days ago

Testing commit 56db829 with merge 3d6970d...

bors commented 6 days ago

Test successful - checks-actions
Approved by: alexcrichton
Pushing 3d6970d to master...

jonhoo commented 6 days ago

It's a good point. I wonder if the way to go here is to take a lesson from alexcrichton/cc-rs#9 and add support for RUSTFLAGS_$TARGET and the like. That way we can say that RUSTFLAGS applies to all targets (including the host), TARGET_RUSTFLAGS applies only to the target(s), HOST_RUSTFLAGS applies only to the host, RUSTFLAGS_$TARGET applies only to $TARGET, etc. What do you think?

alexcrichton commented 5 days ago

Maybe? That's something I think needs discussion outside of just this PR and input from others as well.

jonhoo commented 5 days ago

Oh, yes, definitely. I just don't see a way around doing that, or something like it, if we want cargo build to work like cargo build --target <host>.