Support `per-pkg-target` for `-Zbuild-std`, take two #11969

fee1-dead · 2023-04-13T05:46:07Z

This is a simplified minimal change, which changes the build-std to iterate over the packages to collect a list of targets std should be built for. Does the iterating over packages part have issues with artifact dependencies? I am not very familiar with artifact deps to know so any help would be appreciated. But it should be fine for now since artifact dependencies don't work with build-std yet. #10444

rustbot · 2023-04-13T05:46:11Z

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. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

This is a simplified minimal change, which changes the build-std to iterate over the packages to collect a list of targets std should be built for. Does the iterating over packages part have issues with artifact dependencies? I am not very familiar with artifact deps to know so any help would be appreciated. But it should be fine for now since artifact dependencies don't work with build-std yet. rust-lang#10444

ehuss · 2023-04-26T17:44:22Z

src/cargo/ops/cargo_compile/mod.rs

    // Passing `build_config.requested_kinds` instead of
    // `explicit_host_kinds` here so that `generate_root_units` can do


There's a comment that looks like it needs to be updated here due to the removal of explicit_host_kinds.

ehuss · 2023-04-26T18:14:11Z

tests/build-std/main.rs

@@ -227,3 +227,50 @@ fn custom_test_framework() {
        .build_std_arg("core")
        .run();
 }
+
+/// like cross-custom but uses per-package-target instead
+#[cargo_test(build_std_real)]


Can you say why this needs to be a real build-std test? I would prefer to avoid those if possible. The mock tests should be able to use .arg("target").arg(alternate()) and use the alternate function to specify a target different from the host (and not using a JSON target).

ehuss · 2023-04-26T18:14:44Z

tests/build-std/main.rs

+                name = "foo"
+                version = "0.1.0"
+                edition = "2018"
+                default-target = "custom-target.json"


I don't think this is testing this target. Because build-std requires --target to be specified, the default target is never used. I might suggest using forced-target.

ehuss · 2023-04-26T18:15:26Z

tests/build-std/main.rs

+        .build();
+
+    p.cargo("build -v")
+        .build_std_arg("core")


Preferably this would contain a with_stderr to validate that the --target flag is being passed correctly for each invocation.

ehuss · 2023-04-26T18:16:42Z

tests/testsuite/mock-std/library/proc_macro/src/lib.rs

+// Don't re-export everything in the root so that the mock std can be distinguished from the real one.
 #[stable(since = "1.0.0", feature = "dummy")]
-pub use proc_macro::*;
+pub mod exported {
+    #[stable(since = "1.0.0", feature = "dummy")]
+    pub use proc_macro::*;
+}


Can you clarify why this is needed?

In general, I'm not seeing the connection with proc-macro changes and support for per-pkg-target.

ehuss · 2023-04-26T18:17:38Z

tests/testsuite/standard_lib.rs

+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "pm"
+                version = "0.1.0"
+                [lib]
+                proc-macro = true
+            "#,
+        )


Can you say why this was added?

ehuss · 2023-04-26T18:24:40Z

tests/testsuite/standard_lib.rs

+    p.cargo("build -v")
+        .build_std(&setup)
+        .target_host()
+        .run_expect_error();


This should use with_status(), and also have with_stderr() to verify which error is happening.

Also, similar to the other questions, I'm not clear why this should fail. What's wrong with having a library access the proc_macro crate?

ehuss · 2023-04-26T18:39:21Z

I think this should be fine to move forward with, but I'm a little concerned that this is ultimately not the correct way for this information to be gathered.

The set of targets influences feature resolution, so the collection will need to be done before the standard library is resolved. So from a very high level:

bindeps: Support registry dependencies #10405 needs to be addressed, along with including the per-pkg-target target information in the index.
The RustcTargetData will need to be collected for all targets from the combination of the --target settings and the dependency tree. This will require some restructuring so that the RustcTargetData is collected just before feature resolution. (I think.)
The feature resolver needs to be updated. When it encounters a package that needs different targets (either due to per-pkg-target, or bindeps), it will need to switch which targets it is collecting.
Instead of collecting the set of targets in generate_std_roots as is done in this PR, it should use the list from step 2.

One thing I would like to note, not required for this PR, is that I often have difficulty tracking which circumstances are allowed to have CompileKind::Host and which ones aren't. That is something I think we should eventually clean up. It looks like there is some inconsistency with how that is handled. I'm not sure how best to change that, though.

fee1-dead · 2023-04-27T02:16:03Z

@rustbot author

bors · 2023-11-29T19:08:24Z

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

rustbot assigned ehuss Apr 13, 2023

rustbot added A-cargo-targets Area: selection and definition of targets (lib, bins, examples, tests, benches) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 13, 2023

fee1-dead force-pushed the per-pkg-target-build-std branch 2 times, most recently from 7afcaea to 11bf15f Compare April 13, 2023 07:51

fee1-dead force-pushed the per-pkg-target-build-std branch from 11bf15f to c49c40b Compare April 13, 2023 08:02

ehuss reviewed Apr 26, 2023

View reviewed changes

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 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `per-pkg-target` for `-Zbuild-std`, take two #11969

Support `per-pkg-target` for `-Zbuild-std`, take two #11969

fee1-dead commented Apr 13, 2023

rustbot commented Apr 13, 2023

ehuss Apr 26, 2023

ehuss Apr 26, 2023

ehuss Apr 26, 2023

ehuss Apr 26, 2023

ehuss Apr 26, 2023

ehuss Apr 26, 2023

ehuss Apr 26, 2023

ehuss commented Apr 26, 2023

fee1-dead commented Apr 27, 2023

bors commented Nov 29, 2023

		// Passing `build_config.requested_kinds` instead of
		// `explicit_host_kinds` here so that `generate_root_units` can do

Support per-pkg-target for -Zbuild-std, take two #11969

Are you sure you want to change the base?

Support per-pkg-target for -Zbuild-std, take two #11969

Conversation

fee1-dead commented Apr 13, 2023

rustbot commented Apr 13, 2023

ehuss Apr 26, 2023

Choose a reason for hiding this comment

ehuss Apr 26, 2023

Choose a reason for hiding this comment

ehuss Apr 26, 2023

Choose a reason for hiding this comment

ehuss Apr 26, 2023

Choose a reason for hiding this comment

ehuss Apr 26, 2023

Choose a reason for hiding this comment

ehuss Apr 26, 2023

Choose a reason for hiding this comment

ehuss Apr 26, 2023

Choose a reason for hiding this comment

ehuss commented Apr 26, 2023

fee1-dead commented Apr 27, 2023

bors commented Nov 29, 2023

Support `per-pkg-target` for `-Zbuild-std`, take two #11969

Support `per-pkg-target` for `-Zbuild-std`, take two #11969