Error message for transitive artifact dependencies with targets the package doesn't directly interact with

[jofas]

Address #11260. Produces an error message like described by @weihanglo here:

error: could not find specification for target "x86_64-windows-msvc"
  Dependency `bar v0.1.0` requires to build for target "x86_64-windows-msvc".

Note that this is not a complete fix for #11260.

[rustbot]

r? @ehuss

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

[weihanglo]

Really neat and complete! Love to see people putting more doc comments during development! Thank you!

Just some picky styling issues left. Let me know if you disagree or have a better idea :)

[ehuss]

I suggest that this doesn't entirely fix #11260. I would expect artifact dependencies to work, not generate an error message.

[weihanglo]

Oops that's my fault. I though it was just target spec not found but it turns out not. Cargo should learn all targets required by artifact dependencies, probably after unit graph is created.

Thanks Eric for calling out and sorry for the wrong pointer, @jofas 😞.

[jofas]

I would expect artifact dependencies to work

Me too 😅

sorry for the wrong pointer

No worries. In the issue it sounded like getting all the necessary targets for building transitive artifact dependencies isn't trivial and I assumed a change like that required some more procedural overhead than a simple PR (does cargo have RFCs as well?). With some more hints on where I need to look in order to add the required targets for transitive artifact dependencies to RustcTargetData, I'd love to extent this PR!

[jofas]

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

Lines 919 to 925 in 99b1369

	fn artifact_targets(package: &Package) -> impl Iterator<Item = CompileKind> + '_ {
	package
	.manifest()
	.dependencies()
	.iter()
	.filter_map(|d| d.artifact()?.target()?.to_compile_kind())
	}

AFAICT, if we make this function recursive and call it for each dependency, we should get all the necessary targets for transitive dependencies, no? I haven't found a way yet to get the Package or Manifest objects from Dependency. Maybe from the BuildContext.packages? If we could pass BuildContext down to the RustcTargetData::new, that is. I'll keep digging and see if I can find something more concrete. Any thoughts on my idea?

[weihanglo]

if we make this function recursive and call it for each dependency, we should get all the necessary targets for transitive dependencies, no?

I am afraid it won't work, or requests for too many targets then it really needs. We should also consider optional dependencies and the feature resolution. I believe Cargo doesn't need every disabled target platform toolchain to exist on the host machine.

[jofas]

I am afraid it won't work, or requests for too many targets then it really needs. We should also consider optional dependencies and the feature resolution. I believe Cargo doesn't need every disabled target platform toolchain to exist on the host machine.

Will that really be a problem? Sure, creating TargetInfo (which is where rustc must be executed) seems to be computationally expensive, so it would be unfortunate if we get it for more targets than needed during compilation. But it is "just" performance, whereas now we struggle with correctness. I'm asking, because I can't see how we'd be able to use the UnitGraph (which I'm assuming is the graph containing all the dependencies and how to compile them) for choosing the right targets for RustcTargetData, as for creating the UnitGraph, we need RustcTargetData.

[weihanglo]

r? @weihanglo

[weihanglo]

Hi @jofas! As ehuss said, this is not a complete fix for #11260, but we can still ship this nicer error message. For fixing the root issue, I'll take a deep look next week and see if I can find anything useful.

Let's merge this. Thank you!

[weihanglo]

@bors r+

[bors]

📌 Commit 4bdface has been approved by weihanglo

It is now in the queue for this repository.

[bors]

⌛ Testing commit 4bdface with merge 816772b...

[bors]

💔 Test failed - checks-actions

[ehuss]

I posted #11766 to fix the issue with CI failing.

[weihanglo]

@bors retry

[bors]

⌛ Testing commit 4bdface with merge 524231f...

[bors]

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 524231f to master...

[jofas]

For fixing the root issue, I'll take a deep look next week and see if I can find anything useful.

Great, please let me know if you find anything I could help with.