cargo icon indicating copy to clipboard operation
cargo copied to clipboard

Support per pkg target for `-Zbuild-std`

Open fee1-dead opened this issue 3 years ago • 14 comments

Fixes #9451.

cc @alexcrichton

To review this PR, you can look at the commit summaries and descriptions.

fee1-dead avatar Jan 26 '22 03:01 fee1-dead

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

rust-highfive avatar Jan 26 '22 03:01 rust-highfive

r? @alexcrichton

alexcrichton avatar Feb 01 '22 16:02 alexcrichton

:umbrella: The latest upstream changes (presumably #9992) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Feb 22 '22 04:02 bors

Coming back to this, I don't believe build-std currently has a problem with proc-macro and buildscript builds confusing host-defined std with custom-built std. These lines:

https://github.com/rust-lang/cargo/blob/c9a8199e927192d4f03f73a14f97faa29c9df956/src/cargo/core/compiler/unit_dependencies.rs#L159-L168

.. already avoid attaching custom std deps for host compile kinds. This means that if a user did not specify the compile kind to use for a specific unit, it will just use the host-defined std. Only when specifying a target, through per-pkg-target or --target is when build-std will build a std for that specific target and attach std to those crates that requested those.

fee1-dead avatar Feb 26 '22 03:02 fee1-dead

:umbrella: The latest upstream changes (presumably #10434) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Feb 28 '22 21:02 bors

Hi! @fee1-dead I'm really looking forward to see this change, we really need it in https://github.com/aya-rs/aya ecosystem. Could you please rebase it?

vadorovsky avatar Mar 29 '22 09:03 vadorovsky

r? @ehuss @rustbot ready

fee1-dead avatar Mar 30 '22 10:03 fee1-dead

There seems to be a discussion up at https://github.com/rust-lang/cargo/pull/10330#discussion_r794815070 that doesn't appear to be resolved. I don't think the --target flag requirement can just be removed without major effort. There's a bit more discussion at https://github.com/rust-lang/wg-cargo-std-aware/issues/25.

Also, similar to #10405, I don't think this is a complete solution since per-package targets might come from registry dependencies. Those won't get std built correctly.

ehuss avatar Apr 11 '22 22:04 ehuss

@rustbot author

fee1-dead avatar Apr 12 '22 01:04 fee1-dead

:umbrella: The latest upstream changes (presumably #10129) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar May 04 '22 03:05 bors

Okay so I did some testing and found that no --target basically works the same as --target $HOST. I have updated some tests to make sure that this is indeed true for proc macro crates to use sysroot crates instead of the custom built one.

@rustbot ready

fee1-dead avatar May 24 '22 12:05 fee1-dead

:umbrella: The latest upstream changes (presumably #9892) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Aug 01 '22 01:08 bors

@ehuss I am unfamiliar with how artifact dependencies work and I would like this to land. Since these two are both unstable features now, could we note the certain incompatibilities and just merge it for now for further testing?

fee1-dead avatar Aug 27 '22 13:08 fee1-dead

Hm, I was perhaps not understanding the intent of the per-package-target feature. I did not realize that it doesn't work with dependencies at all. That doesn't match the motivation for the original issue (#7004), so I'm not really clear how per-package-target is intended to be used. If you or others can put some clear use cases in the tracking issue, that would be helpful.

As for moving forward with this, I think that may be possible. There are still issues around resolution, features, and target-info collection. But those aren't new.

Just some general feedback from a quick look:

  • The restriction on specifying --target can't be removed at this time.
  • The use of package_set.packages() to fetch a list of root packages of interest seems fragile. I'd like to see a more principled way of being explicit that "these are all of the kinds of the root set".
  • It isn't clear why there is a change related to --build-plan

ehuss avatar Aug 28 '22 18:08 ehuss

Interaction with artifact dependencies can be ruled with requirement for each artifact dep to always have its target specified. Implicit behavior here can always be added later and is unclear now (should it be inherited from dependent/workspace/the package itself???)

tema3210 avatar Sep 22 '22 09:09 tema3210