Skip to content
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

Fix passing of linker with target-applies-to-host and an implicit target #14206

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gmorenz
Copy link
Contributor

@gmorenz gmorenz commented Jul 6, 2024

What does this PR try to resolve?

Fixes the linker half of #14195.

The second commit ("Fix linker value with targeta-applies-to-host and implicit target by storing it in Unit") does just that following the same structure as #13900, though the current place where linker is stored is a bit farther away in the code base the logical change and justification for it is the same as in #13900. As part of this we end up passing target_config into TargetInfo::new to get convenient access to the linker argument, there is an alternative here that I felt was worse to simply fetch the linker field from gctx, which is more like how rustflags currently works. This ends up leaving a bit of a footgun though, where that target_config doesn't necessarily have the right rustflags values despite being passed in.

The third commit ("Read rustflags from TargetConfig in TargetInfo::new") changes rustflags to be read from TargetConfig like linker works above. Apart from being arguably cleaner code, this removes the footgun I introduced in the second commit, and removes an existing footgun where the rustflags in RustcTargetData::host_config aren't necessarily correct (though it continues to be the case that TargetConfigs don't necessarily have the right values if there is another source of values). It was also simply made easier to do now since the second commit already conceded to making the change that TargetInfo::new requires a TargetConfig. That said, it is largely independent from this PR and could be removed if it is a sticking point.

The first commit just adds a test.

How should we test and review this PR?

For testing, the second commit comes with it's own test (from the first commit), the third commit relies on existing tests to catch any mistakes.

Additional information

The other half of fixing this issue is #14205, the two PRs will have a merge conflict as a result of field ordering in Unit but are otherwise independent.

@rustbot label: Z-target-applies-to-host

r? @weihanglo if you're willing to look at this

@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler A-build-scripts Area: build.rs scripts A-cargo-targets Area: selection and definition of targets (lib, bins, examples, tests, benches) A-cfg-expr Area: Platform cfg expressions A-rebuild-detection Area: rebuild detection and fingerprinting Command-test S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. Z-target-applies-to-host Nightly: target-applies-to-host labels Jul 6, 2024
@@ -25,7 +25,8 @@ use std::path::{Path, PathBuf};
use std::str::{self, FromStr};
use std::sync::Arc;

/// Information about the platform target gleaned from querying rustc.
/// Information about the platform target gleaned from querying rustc and from
/// merging configs.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment didn't really fit with this struct containing rustflags (which predates me working on target-applies-to-host). Putting linker in it makes the mismatch slightly worse, so I thought I'd update the comment.

I'm not entirely thrilled with the phrasing. If someone has a suggestion for better phrasing I'd be happy to take it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe somthing like?

/// Information and compiler flags for the platform target gleaned from querying rustc
/// and from Cargo configurations.

@weihanglo
Copy link
Member

Going to propose this change to the Cargo team on Zulip t-cargo soon. Probably tomorrow.

I don't think we need to go through an FCP process for updating an unstable feature. Still no harm to get a soft consensus from the team.

@gmorenz
Copy link
Contributor Author

gmorenz commented Jul 10, 2024

Sounds good, I'll keep a tab open to that in case there's anything I can contribute.

@@ -21,7 +21,8 @@ fn bad1() {
p.cargo("check -v --target=nonexistent-target")
.with_status(101)
.with_stderr_data(str![[r#"
[ERROR] expected table for configuration key `target.nonexistent-target`, but found string in [ROOT]/foo/.cargo/config.toml
[ERROR] invalid configuration for key `target.nonexistent-target`
expected a table, but found a string for `target.nonexistent-target` in [ROOT]/foo/.cargo/config.toml
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this test change?

Because we parse this configuration in two places, in two slightly different ways, and we return slightly different error messages depending on what parsing path we went down. Moving target_linker so that it's called much earlier in the process switched which code path tries to parse this first, and thus which error we get.

I don't think the errors are meaningfully different, so I'm proposing just adopting the error that happens to be returned first as correct. If anyone disagrees, I think the best alternative would be to simply make the two error paths return the same string. That's slightly awkward because the error path we are now hitting is generic over types so we'd have to either special case "table" (which wouldn't be hard) or decide we're happy with the same syntax for all types.

If there's any interest, the function call paths to these errors was:

RustcTargetData::new -> TargetInfo::merge_compile_kind -> gctx.target_cfg_triple -> target::load_target_triple -> target::load_config_table -> gctx.get::<OptValue<PathAndArgs>>("{prefix}.runner") -> T::deserialize -> Deserializer::deserialize_option -> gctx.has_key -> self.get_cv -> self.get_cv_helper -> bail!

And is now

RustcTargetData::new -> TargetInfo::new -> target_linker -> gctx.target_cfgs -> target::load_target_cfgs -> gctx.get::<BTreeMap<String, TargetCfgConfig>>("target") -> T::deserialize -> ConfigMapAccess::new_map -> gctx.get_table -> gctx.expected

@gmorenz gmorenz marked this pull request as ready for review July 11, 2024 21:45
@bors
Copy link
Collaborator

bors commented Jul 13, 2024

☔ The latest upstream changes (presumably #14243) made this pull request unmergeable. Please resolve the merge conflicts.

@gmorenz
Copy link
Contributor Author

gmorenz commented Jul 19, 2024

Huh, why isn't CI running?

Rebased on master (at least as of a few hours ago) and changed from using Arc to Rc as suggested by weihanglo in #14205.

@weihanglo
Copy link
Member

@gmorenz
Copy link
Contributor Author

gmorenz commented Jul 19, 2024

That's a better guess than mine that github was unhappy about the fact that there was a few hours between me making the changes and pushing them.

Rebased on master again (no longer 4 hours in the past).

@@ -25,7 +25,8 @@ use std::path::{Path, PathBuf};
use std::str::{self, FromStr};
use std::sync::Arc;

/// Information about the platform target gleaned from querying rustc.
/// Information about the platform target gleaned from querying rustc and from
/// merging configs.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe somthing like?

/// Information and compiler flags for the platform target gleaned from querying rustc
/// and from Cargo configurations.

Comment on lines +933 to +937
if requested_kinds != [CompileKind::Host] {
// When an explicit target flag is passed rustflags are ignored for host artifacts.
config.rustflags = None;
config.rustdocflags = None;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😮‍💨 seems like a bad code smell to me, but I understand it is for maintaining the dual bad behavior that linker is respected but rustflags is not, right?

Copy link
Contributor Author

@gmorenz gmorenz Jul 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly.

And I 100% agree with bad code smell, but this feels less bad to me than the pre-existing reality that host_config is just storing incorrect rustflags in this case. It's one of the things that took me awhile to understand the first time I started working on this code (and why I added that comment on line 943 that I now get to delete).

A more ambitious refactoring option might be to try and push all this logic into something like gctx.host_cfg_triple. I remember trying that when I wrote this and rejecting it, but I don't remember why I rejected it and I'd be happy to give it another shot if you think that it might be cleaner.

@@ -826,6 +836,42 @@ fn rustflags_from_target(
}
}

/// Gets the user-specified linker for a particular host or target from the configuration.
fn target_linker(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this PR the target_linker is called when gleaning target info. Before this, it was called during Compilation::new in compile_ws. This may fail some commands like cargo metadata, cargo tree or cargo -Zunstable-options rustc --print due to bad configuration files.

For example, cargo metadata failed with this configs but not before.

[target.'cfg(target_arch = "x86_64")']
linker = "foo"

[target.'cfg(target_os = "linux")']
linker = "foo"

This might not be ideal because those commands don't need target infos.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I entirely missed that this was a problem.

I don't see a good solution to this, I'll come back with a fresh set of eyes tomorrow and see if that changes.

Right now my take is it's either this (bad), don't fix this issue at all (also bad), or doing something involving not so clean code to defer actually returning the error (also bad). But I think there's a small chance I can find a better way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, the current approach is not too bad for normal users.

However, I know some enterprise users have weird multi-level configuration setup. And may move projects among different directories. This is also bad because you cannot opt out directory-probing for configuration files (which Cargo may provide an escape patch?).

@bors
Copy link
Collaborator

bors commented Jul 20, 2024

☔ The latest upstream changes (presumably #14273) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-execution Area: anything dealing with executing the compiler A-build-scripts Area: build.rs scripts A-cargo-targets Area: selection and definition of targets (lib, bins, examples, tests, benches) A-cfg-expr Area: Platform cfg expressions A-rebuild-detection Area: rebuild detection and fingerprinting Command-test S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. Z-target-applies-to-host Nightly: target-applies-to-host
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants