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

Support per pkg target for -Zbuild-std #10330

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

Conversation

fee1-dead
Copy link
Member

@fee1-dead fee1-dead commented Jan 26, 2022

Fixes #9451.

cc @alexcrichton

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

@rust-highfive
Copy link

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 rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 26, 2022
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

This looks pretty reasonable to me, thanks for this!

src/cargo/ops/cargo_compile.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/standard_lib.rs Outdated Show resolved Hide resolved
src/cargo/core/compiler/standard_lib.rs Outdated Show resolved Hide resolved
// TODO: This should eventually be fixed. Unfortunately it is not
// easy to get the host triple in BuildConfig. Consider changing
// requested_target to an enum, or some other approach.
anyhow::bail!("-Zbuild-std requires --target");
Copy link
Member

Choose a reason for hiding this comment

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

I think that we still need to keep this around in one form or another. IIRC the issue has to do with where if you have cargo build for nothing with a per-package-target or anything like that then build scripts and proc macros will do the wrong thing and try to use the custom-built libstd.

I think though that this check would need to improve because if your manifest has a per-package-target then there's no need to require --target on the command line, so this check may just need to get deferred to later?

Copy link
Member Author

Choose a reason for hiding this comment

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

The todo comment seems to suggest that we would like to see support for -Zbuild-stding the host target though, which would be supported after this change thanks to target selection overhaul in our logic.

then build scripts and proc macros will do the wrong thing and try to use the custom-built libstd.

Ah I see. Then we could avoid attaching the custom std dependencies for proc-macro and build script runs. I will write a test for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

On a second glance I don't think we should add this as a test. We can't just do -Zbuild-std=std because then it doesn't matter if a dependent crate is proc-macro of a build script because what happens if they use the custom std?

But if we do -Zbuild-std=core, the dependent crate will use the default sysroot instead, IIUC. (because it wants to use proc-macro and build-std is not configured to build proc-macro therefore it uses the installed libstd). On my local machine I did try to add some tests but it fails when I add them:

error: no matching package named `pm` found
location searched: registry `crates-io`
required by package `foo v0.1.0 (/shared/develop/cargo/target/tmp/cit/t0/foo)`
', tests/build-std/main.rs:353:70
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    proc_macro_dep_no_explicit_target

Copy link
Member

Choose a reason for hiding this comment

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

There were a number of blockers to enabling cargo build -Zbuild-std. This PR is one of them but it is not the only blocker, and the other blocker (that I know of at least) is that host things (build scripts and proc macros) use the custom-built std. Build scripts and proc macros should always use the host-defined std, never the custom-built std. One major use case is rebuilding the custom-built std with different codegen options that may or may not be applicable to the host (e.g. fuzz options).

I realize that requiring --target obviates this PR somewhat, but this is not a conditional that can be unconditionally removed until this other issue in the behavior is fixed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm stuck. I think we could continue the last PR but keep it also as a cli option? Then we could rewrite the logic handling each crate instead of the current way, handling the whole build.

Copy link
Member

Choose a reason for hiding this comment

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

To clarify, I think "fixing" this to not actually need --target is likely to be somewhat difficult in Cargo. I haven't looked myself to see what it might entail but I don't think it will be trivial.

Otherwise I don't have a preference to go about this. My main point is that we can't simply stop requiring --target due to the consequence of how Cargo treats things like RUSTFLAGS

@alexcrichton
Copy link
Member

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned ehuss Feb 1, 2022
@alexcrichton alexcrichton added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 1, 2022
kennystrawnmusic added a commit to kennystrawnmusic/cargo that referenced this pull request Feb 12, 2022
@bors
Copy link
Collaborator

bors commented Feb 22, 2022

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

@fee1-dead
Copy link
Member Author

fee1-dead commented Feb 26, 2022

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:

/// Add the standard library units to the `unit_dependencies`.
fn attach_std_deps(
state: &mut State<'_, '_>,
std_roots: &HashMap<CompileKind, Vec<Unit>>,
std_unit_deps: UnitGraph,
) {
// Attach the standard library as a dependency of every target unit.
let mut found = false;
for (unit, deps) in state.unit_dependencies.iter_mut() {
if !unit.kind.is_host() && !unit.mode.is_run_custom_build() {

.. 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 fee1-dead force-pushed the support-per-pkg-target-w-buildstd branch from c99ca79 to 7bd59cd Compare February 26, 2022 03:54
@bors
Copy link
Collaborator

bors commented Feb 28, 2022

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

@alexcrichton alexcrichton removed their assignment Mar 8, 2022
@vadorovsky
Copy link
Contributor

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?

@fee1-dead fee1-dead force-pushed the support-per-pkg-target-w-buildstd branch 2 times, most recently from 9873dff to f7e023a Compare March 30, 2022 10:11
@fee1-dead
Copy link
Member Author

r? @ehuss
@rustbot ready

@rustbot rustbot removed the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Mar 30, 2022
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 30, 2022
@ehuss
Copy link
Contributor

ehuss commented Apr 11, 2022

There seems to be a discussion up at #10330 (comment) 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 rust-lang/wg-cargo-std-aware#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.

@fee1-dead
Copy link
Member Author

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 12, 2022
@bors
Copy link
Collaborator

bors commented May 4, 2022

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

@fee1-dead fee1-dead force-pushed the support-per-pkg-target-w-buildstd branch from f7e023a to e5157f9 Compare May 24, 2022 04:54
@fee1-dead fee1-dead force-pushed the support-per-pkg-target-w-buildstd branch from 5a0b75d to 85174f7 Compare May 24, 2022 11:28
@fee1-dead fee1-dead force-pushed the support-per-pkg-target-w-buildstd branch from 85174f7 to 14e8794 Compare May 24, 2022 11:29
@fee1-dead
Copy link
Member Author

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels May 24, 2022
@fee1-dead fee1-dead force-pushed the support-per-pkg-target-w-buildstd branch from 794202b to ea4d6df Compare May 24, 2022 12:42
@bors
Copy link
Collaborator

bors commented Aug 1, 2022

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

@fee1-dead
Copy link
Member Author

fee1-dead commented Aug 27, 2022

@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?

@ehuss
Copy link
Contributor

ehuss commented Aug 28, 2022

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 ehuss added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2022
@tema3210
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The new per-package-target feature does not work with -Zbuild-std
8 participants