Refactor `Kind` to carry target name in `Target` #7425

alexcrichton · 2019-09-24T18:54:15Z

This commit is an internal refactoring of Cargo's compilation backend to
eventually support compiling multiple target simultaneously. The
original motivation for this came up in discussion of #7297 and this has
long been something I've intended to update Cargo for. Nothing in the
backend currently exposes the ability to actually build multiple target
simultaneously, but this should have no function change with respect to
all current consumers. Eventually we'll need to refactor APIs of how you
enter the compilation backend to compile for multiple targets.

rust-highfive · 2019-09-24T18:54:19Z

r? @Eh2406

(rust_highfive has picked a reviewer for you, use r? to override)

Eh2406 · 2019-09-24T19:04:59Z

Looks ok to me, but I am not confident in my understanding.
@ehuss How does it look to you?

ehuss · 2019-09-24T21:40:19Z

So I've been thinking about renaming Kind to something like TargetKind or PlatformKind. I sometimes find it confusing which Kind is being used. There are currently 4 kinds in cargo's codebase (dependency, target, source, registry). They don't usually overlap, but I think it would be clearer if they had distinct names. WDYT? ("target" is also a bit confusing since it has 4 different meanings).

The other thing I was wondering about as I mentioned in #7421 is to wrap the requested target in a newtype struct to prevent misuse for json targets. I think the only misuse is in TargetInfo::sysroot_libdir where it incorrectly puts the .json extension in the path. I think wrapping it in a type would force each use of the "target triple" to think about whether it wants the string that is passed to --target or the path component used in various places. What do you think of that?

alexcrichton · 2019-09-24T22:01:19Z

Alternatively we could optimize for the number of enum Kind within Cargo. I see 6 enum *Kind that could be candidates for being renamed to Kind.

aka those sound like excellent suggestions, will do!

alexcrichton · 2019-09-25T15:16:28Z

Ok, updated!

r? @ehuss

bors · 2019-09-25T16:35:23Z

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

src/cargo/core/compiler/compile_kind.rs

ehuss · 2019-09-25T18:39:46Z

src/cargo/core/compiler/build_context/target_info.rs

@@ -81,18 +81,11 @@ impl FileType {
 impl TargetInfo {
    pub fn new(
        config: &Config,
-        requested_target: &Option<String>,
+        requested_kind: CompileKind,


I'm having trouble unraveling why there are two CompileKinds being passed in here. Can there just be 1 value passed in? There seems to be some dancing around in env_args based on which is which. But BuildContext::target_info shouldn't ever be used for a host-only build, so I don't know why it supports a requested_target=Host and a kind=Target.

The rationale is basically that RUSTFLAGS plus --target shouldn't pass the flags to build scripts. This is needed here I think because we calculate TargetInfo for both the host and target, right?

Ah, I see now.

ehuss · 2019-09-25T19:00:22Z

src/cargo/core/compiler/compilation.rs

-            host_dylib_path: bcx.host_info.sysroot_libdir.clone(),
-            target_dylib_path: bcx.target_info.sysroot_libdir.clone(),
+            host_dylib_path: bcx.info(CompileKind::Host).sysroot_libdir.clone(),
+            target_dylib_path: bcx.info(default_kind).sysroot_libdir.clone(),


In a world were cargo supports multiple targets, wouldn't this need to be a map?

It would, yeah. Compilation would need some larger refactorings to support multi-target builds for sure. I'm thinking those would probably want to happen if we were to ever expose this at the CLI level, but before then it may be premature to apply the refactorings here. I don't mind going either way though!

No, I just wanted to make sure I was understanding it correctly.

src/cargo/core/compiler/build_context/mod.rs

src/cargo/core/compiler/build_context/target_info.rs

src/cargo/core/compiler/compile_kind.rs

ehuss · 2019-09-25T19:36:37Z

src/cargo/core/compiler/compile_kind.rs

+}
+
+#[derive(PartialEq, Eq, Hash, Debug, Clone, Copy, PartialOrd, Ord, Serialize)]
+pub struct CompileTarget {


This could use a brief doc comment explaining that it may be a target triple or path (absolute?) to a .json file.

Also, I'm curious, did you consider making it an enum to differentiate the two? If it is an enum, it could pre-compute rustc_target so it is not repeatedly recomputed.

Comment added! I didn't actually consider using an enum, but I was a bit overzealous in that I was trying to keep this the same size as an InternedString. There's definitely not a great reason for that though.

I tried to remove the Copy bound on this to store something like PathBuf, but unfortunately that would require a good deal of work I think.

I'm sort of fine either way, I think having it encapsulated is fine for now, but I don't mind either way. Would you prefer to switch to an enum?

No, it was just a thought to simplify short_name, but I don't think it is necessary.

src/cargo/core/compiler/mod.rs

bors · 2019-09-26T15:22:01Z

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

This commit is an internal refactoring of Cargo's compilation backend to eventually support compiling multiple target simultaneously. The original motivation for this came up in discussion of rust-lang#7297 and this has long been something I've intended to update Cargo for. Nothing in the backend currently exposes the ability to actually build multiple target simultaneously, but this should have no function change with respect to all current consumers. Eventually we'll need to refactor APIs of how you enter the compilation backend to compile for multiple targets.

Rename `Kind` to `CompileKind` to reflect that it's intended for compilation. Additionally change the `Target` variant to have a newtype `CompileTarget` instead of just being a raw string. This new `CompileTarget` type has a fallible constructor and handles custom json target files internally. Two accessors are available for `CompileTarget`, one is `rustc_target()` which goes straight to rustc and everything else uses `short_name()` which is the raw target or file stem for json files. The `short_name` is used everywhere in Cargo for all purposes like configuration, env vars, target directory naming, etc.

ehuss · 2019-09-26T17:11:57Z

@bors r+

bors · 2019-09-26T17:11:58Z

📌 Commit f745ca7 has been approved by ehuss

Refactor `Kind` to carry target name in `Target` This commit is an internal refactoring of Cargo's compilation backend to eventually support compiling multiple target simultaneously. The original motivation for this came up in discussion of #7297 and this has long been something I've intended to update Cargo for. Nothing in the backend currently exposes the ability to actually build multiple target simultaneously, but this should have no function change with respect to all current consumers. Eventually we'll need to refactor APIs of how you enter the compilation backend to compile for multiple targets.

bors · 2019-09-26T17:12:06Z

⌛ Testing commit f745ca7 with merge d8e62ee...

bors · 2019-09-26T17:47:50Z

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing d8e62ee to master...

This fixes an accidental regression from rust-lang#7425 where `PATH` was being augmented on Windows with the wrong search path for target/host libraries. This commit fixes the issue by simply always calculating the host/target library paths for `TargetInfo`, and then we explicitly use the same `TargetInfo` for filling out information in `Compilation`. Closes rust-lang#7475

Fix wrong directories in PATH on Windows This fixes an accidental regression from #7425 where `PATH` was being augmented on Windows with the wrong search path for target/host libraries. This commit fixes the issue by simply always calculating the host/target library paths for `TargetInfo`, and then we explicitly use the same `TargetInfo` for filling out information in `Compilation`. Closes #7475

rust-highfive assigned Eh2406 Sep 24, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 24, 2019

rust-highfive assigned ehuss and unassigned Eh2406 Sep 25, 2019

alexcrichton force-pushed the kind-string branch from 563569b to 43f3952 Compare September 25, 2019 17:05

ehuss reviewed Sep 25, 2019

View reviewed changes

alexcrichton added 3 commits September 26, 2019 08:23

Address review about doc blocks

f745ca7

alexcrichton force-pushed the kind-string branch from 43f3952 to f745ca7 Compare September 26, 2019 15:30

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 26, 2019

bors merged commit f745ca7 into rust-lang:master Sep 26, 2019

bors deleted the kind-string branch September 26, 2019 17:47

This was referenced Sep 26, 2019

Support for named profiles (RFC 2678) #6989

Merged

Fast clone graph #7168

Closed

Removing hash from output files when using MSVC #7400

Merged

ehuss mentioned this pull request Oct 3, 2019

Wrong PATH override on Windows #7475

Closed

alexcrichton mentioned this pull request Oct 4, 2019

Fix wrong directories in PATH on Windows #7482

Merged

ehuss mentioned this pull request Nov 21, 2019

Regression in beta: failed to parse slash in a custom cfg name #7613

Closed

ehuss mentioned this pull request Aug 24, 2020

The TARGET environment variable does not point to the file when a target JSON file is used #8646

Open

ehuss added this to the 1.40.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `Kind` to carry target name in `Target` #7425

Refactor `Kind` to carry target name in `Target` #7425

alexcrichton commented Sep 24, 2019

rust-highfive commented Sep 24, 2019

Eh2406 commented Sep 24, 2019

ehuss commented Sep 24, 2019

alexcrichton commented Sep 24, 2019

alexcrichton commented Sep 25, 2019

bors commented Sep 25, 2019

ehuss Sep 25, 2019

alexcrichton Sep 26, 2019

ehuss Sep 26, 2019

ehuss Sep 25, 2019

alexcrichton Sep 26, 2019

ehuss Sep 26, 2019

ehuss Sep 25, 2019

alexcrichton Sep 26, 2019

ehuss Sep 26, 2019

bors commented Sep 26, 2019

ehuss commented Sep 26, 2019

bors commented Sep 26, 2019

bors commented Sep 26, 2019

bors commented Sep 26, 2019

Refactor Kind to carry target name in Target #7425

Refactor Kind to carry target name in Target #7425

Conversation

alexcrichton commented Sep 24, 2019

rust-highfive commented Sep 24, 2019

Eh2406 commented Sep 24, 2019

ehuss commented Sep 24, 2019

alexcrichton commented Sep 24, 2019

alexcrichton commented Sep 25, 2019

bors commented Sep 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Sep 26, 2019

ehuss commented Sep 26, 2019

bors commented Sep 26, 2019

bors commented Sep 26, 2019

bors commented Sep 26, 2019

Refactor `Kind` to carry target name in `Target` #7425

Refactor `Kind` to carry target name in `Target` #7425