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

jofas · 2023-01-28T10:16:27Z

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.

…endencies

…rustc

rustbot · 2023-01-28T10:16:32Z

r? @ehuss

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

tests/testsuite/artifact_dep.rs

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 :)

tests/testsuite/artifact_dep.rs

src/cargo/core/compiler/compilation.rs

ehuss · 2023-01-28T15:08:26Z

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

weihanglo · 2023-01-28T15:28:42Z

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 · 2023-01-28T15:45:33Z

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 · 2023-01-30T15:36:51Z

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 · 2023-01-31T11:40:34Z

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.

Co-authored-by: Weihang Lo <weihanglo@users.noreply.github.com>

…+ incorporated changes into corresponding test

jofas · 2023-02-06T15:59:11Z

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 · 2023-02-24T20:24:10Z

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 · 2023-02-25T00:15:11Z

@bors r+

bors · 2023-02-25T00:15:13Z

📌 Commit 4bdface has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-02-25T00:15:21Z

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

Error message for transitive artifact dependencies with targets the package doesn't directly interact with Address #11260. Produces an error message like described by `@weihanglo` [here](#11260 (comment)): ``` 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.

bors · 2023-02-25T00:44:40Z

💔 Test failed - checks-actions

ehuss · 2023-02-25T01:31:00Z

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

weihanglo · 2023-02-25T07:39:02Z

@bors retry

bors · 2023-02-25T07:39:11Z

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

bors · 2023-02-25T08:27:12Z

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

jofas · 2023-02-25T09:39:34Z

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.

10 commits in 9d5b32f503fc099c4064298465add14d4bce11e6..9880b408a3af50c08fab3dbf4aa2a972df71e951 2023-02-22 23:04:16 +0000 to 2023-02-28 19:39:39 +0000 - bump jobserver to respect `--jobserver-auth=fifo:PATH` (rust-lang/cargo#11767) - Addition of support for -F as an alias for --features (rust-lang/cargo#11774) - Added documentation for the configuration discovery of `cargo install` to the man pages (rust-lang/cargo#11763) - Fix Cargo removing the sparse+ prefix from sparse URLs in .crates.toml (rust-lang/cargo#11756) - Fix warning with tempfile (rust-lang/cargo#11771) - Error message for transitive artifact dependencies with targets the package doesn't directly interact with (rust-lang/cargo#11643) - Fix tests with nondeterministic ordering (rust-lang/cargo#11766) - Make some blocking tests non-blocking (rust-lang/cargo#11650) - Suggest cargo add when installing library crate (rust-lang/cargo#11410) - chore: bump is-terminal to 0.4.4 (rust-lang/cargo#11759)

Update cargo 10 commits in 9d5b32f503fc099c4064298465add14d4bce11e6..9880b408a3af50c08fab3dbf4aa2a972df71e951 2023-02-22 23:04:16 +0000 to 2023-02-28 19:39:39 +0000 - bump jobserver to respect `--jobserver-auth=fifo:PATH` (rust-lang/cargo#11767) - Addition of support for -F as an alias for --features (rust-lang/cargo#11774) - Added documentation for the configuration discovery of `cargo install` to the man pages (rust-lang/cargo#11763) - Fix Cargo removing the sparse+ prefix from sparse URLs in .crates.toml (rust-lang/cargo#11756) - Fix warning with tempfile (rust-lang/cargo#11771) - Error message for transitive artifact dependencies with targets the package doesn't directly interact with (rust-lang/cargo#11643) - Fix tests with nondeterministic ordering (rust-lang/cargo#11766) - Make some blocking tests non-blocking (rust-lang/cargo#11650) - Suggest cargo add when installing library crate (rust-lang/cargo#11410) - chore: bump is-terminal to 0.4.4 (rust-lang/cargo#11759) r? `@ghost`

jofas added 3 commits January 24, 2023 12:40

removed trailing whitespaces (done automatically by vim)

ded1a15

added test replicating error encountered with transitive artifact dep…

d5e43bb

…endencies

error message for target specification that couldn't be queried from …

ef227e4

…rustc

rustbot assigned ehuss Jan 28, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 28, 2023

jofas commented Jan 28, 2023

View reviewed changes

tests/testsuite/artifact_dep.rs Show resolved Hide resolved

weihanglo reviewed Jan 28, 2023

View reviewed changes

jofas and others added 4 commits February 6, 2023 14:24

Update src/cargo/core/compiler/compilation.rs

28ec9b1

Co-authored-by: Weihang Lo <weihanglo@users.noreply.github.com>

Update src/cargo/core/compiler/compilation.rs

8f9e44d

Co-authored-by: Weihang Lo <weihanglo@users.noreply.github.com>

Update src/cargo/core/compiler/compilation.rs

96366bd

Co-authored-by: Weihang Lo <weihanglo@users.noreply.github.com>

applying code review to make get_sysroot_target_libdir more readable …

4bdface

…+ incorporated changes into corresponding test

rustbot assigned weihanglo and unassigned ehuss Feb 24, 2023

weihanglo approved these changes Feb 25, 2023

View reviewed changes

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 Feb 25, 2023

bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 25, 2023

bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 25, 2023

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 Feb 25, 2023

bors merged commit 524231f into rust-lang:master Feb 25, 2023

weihanglo mentioned this pull request Feb 28, 2023

Update cargo rust-lang/rust#108582

Merged

ehuss added this to the 1.69.0 milestone Mar 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jofas commented Jan 28, 2023 •

edited by weihanglo

Loading

rustbot commented Jan 28, 2023

weihanglo left a comment

ehuss commented Jan 28, 2023

weihanglo commented Jan 28, 2023

jofas commented Jan 28, 2023 •

edited

Loading

jofas commented Jan 30, 2023

weihanglo commented Jan 31, 2023

jofas commented Feb 6, 2023 •

edited

Loading

weihanglo commented Feb 24, 2023

weihanglo left a comment

weihanglo commented Feb 25, 2023

bors commented Feb 25, 2023

bors commented Feb 25, 2023

bors commented Feb 25, 2023

ehuss commented Feb 25, 2023

weihanglo commented Feb 25, 2023

bors commented Feb 25, 2023

bors commented Feb 25, 2023

jofas commented Feb 25, 2023

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

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

Conversation

jofas commented Jan 28, 2023 • edited by weihanglo Loading

rustbot commented Jan 28, 2023

weihanglo left a comment

Choose a reason for hiding this comment

ehuss commented Jan 28, 2023

weihanglo commented Jan 28, 2023

jofas commented Jan 28, 2023 • edited Loading

jofas commented Jan 30, 2023

weihanglo commented Jan 31, 2023

jofas commented Feb 6, 2023 • edited Loading

weihanglo commented Feb 24, 2023

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented Feb 25, 2023

bors commented Feb 25, 2023

bors commented Feb 25, 2023

bors commented Feb 25, 2023

ehuss commented Feb 25, 2023

weihanglo commented Feb 25, 2023

bors commented Feb 25, 2023

bors commented Feb 25, 2023

jofas commented Feb 25, 2023

jofas commented Jan 28, 2023 •

edited by weihanglo

Loading

jofas commented Jan 28, 2023 •

edited

Loading

jofas commented Feb 6, 2023 •

edited

Loading