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

Re-do: Progress on #9451 #10108

Closed
wants to merge 19 commits into from
Closed

Re-do: Progress on #9451 #10108

wants to merge 19 commits into from

Conversation

kennystrawnmusic
Copy link

Didn't arrive at a complete solution yet, but have been doing some hard work that brings us closer to one ― including, most significantly, storing explicit_host_kind as an enum per the suggestion in the //TODO comment in the std_resolve_features definition.

This code, unlike the previous attempt, has been tested locally. I would be more than honored to have others from upstream tinker with this PR a bit before merging it, given that in its current state a call to target_config() in some as-yet-unknown location mysteriously panics with a "no entry found for key" message. With that out of the way, defining PerPackageTarget as an option with a nested enum as I did here is definitely a step in the right direction.

@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 Nov 22, 2021
@kennystrawnmusic
Copy link
Author

kennystrawnmusic commented Nov 22, 2021

In the event that one can get this to not panic (and hopefully also to help get to the bottom of this problem a bit more quickly), here's a patch to apply to the currently-existing -Zbuild-std test to take into account per-package-target support. I also invited @alexcrichton, @ehuss, and @phil-opp as collaborators on my fork in order to help speed this up further.

From 5e048c7f3893db610c8aabafa68835185a051a88 Mon Sep 17 00:00:00 2001
From: Kenny Strawn <[email protected]>
Date: Sun, 21 Nov 2021 19:55:20 -0800
Subject: [PATCH] Update current -Zbuild-std test to work with
 per-package-target

---
 tests/build-std/main.rs | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/tests/build-std/main.rs b/tests/build-std/main.rs
index c1355b317..3ed416872 100644
--- a/tests/build-std/main.rs
+++ b/tests/build-std/main.rs
@@ -133,12 +133,15 @@ fn cross_custom() {
         .file(
             "Cargo.toml",
             r#"
+                cargo-features = ["per-package-target"]
+
                 [package]
                 name = "foo"
                 version = "0.1.0"
-                edition = "2018"
+                edition = "2021"
+                default-target = "custom-target.json"
 
-                [target.custom-target.dependencies]
+                [dependencies]
                 dep = { path = "dep" }
             "#,
         )
@@ -165,7 +168,7 @@ fn cross_custom() {
         )
         .build();
 
-    p.cargo("build --target custom-target.json -v")
+    p.cargo("build -v")
         .build_std_arg("core")
         .run();
 }
@@ -173,6 +176,21 @@ fn cross_custom() {
 #[cargo_test(build_std)]
 fn custom_test_framework() {
     let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                cargo-features = ["per-package-target"]
+
+                [package]
+                name = "foo"
+                version = "0.1.0"
+                edition = "2021"
+                default-target = "target.json"
+
+                [dependencies]
+                dep = { path = "dep" }
+            "#,
+        )
         .file(
             "src/lib.rs",
             r#"
@@ -222,7 +240,7 @@ fn custom_test_framework() {
     paths.insert(0, sysroot_bin);
     let new_path = env::join_paths(paths).unwrap();
 
-    p.cargo("test --target target.json --no-run -v")
+    p.cargo("test --no-run -v")
         .env("PATH", new_path)
         .build_std_arg("core")
         .run();
-- 
2.34.0

@kennystrawnmusic
Copy link
Author

kennystrawnmusic commented Nov 22, 2021

Interesting that the in-house build_std fails on the server given that it's perfectly OK on my end:

[realkstrawn93@realkstrawn93-miner ~/cargo]$ cargo test --test build-std
   Compiling cargo-test-support v0.1.0 (/home/realkstrawn93/cargo/crates/cargo-test-support)
   Compiling cargo-test-macro v0.1.0 (/home/realkstrawn93/cargo/crates/cargo-test-macro)
   Compiling cargo v0.59.0 (/home/realkstrawn93/cargo)
    Finished test [unoptimized + debuginfo] target(s) in 17.97s
     Running tests/build-std/main.rs (target/debug/deps/build_std-42ea3220a68f2646)

running 3 tests
test basic ... ok
test cross_custom ... ok
test custom_test_framework ... ok

test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.07s

[realkstrawn93@realkstrawn93-miner ~/cargo]$

And when I updated the test file to make it work with per-package-target, then proceeded to run the same test ― same thing:


[realkstrawn93@realkstrawn93-miner cargo]$ ./target/debug/cargo test --test build-std
   Compiling cargo v0.59.0 (/home/realkstrawn93/cargo)
    Finished test [unoptimized + debuginfo] target(s) in 2.96s
     Running tests/build-std/main.rs (target/debug/deps/build_std-42ea3220a68f2646)

running 3 tests
test custom_test_framework ... ok
test basic ... ok
test cross_custom ... ok

test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.07s

[realkstrawn93@realkstrawn93-miner cargo]$

I wonder what's getting the CI servers to treat it differently. I am running an Arch Linux host which does have a more bleeding edge kernel and compiler toolchain than Ubuntu so maybe something to do with that? Update: The version I'm using is a nightly, with good reason. Please try and build on the CI server first before testing, then using ./target/release/cargo test instead of merely cargo test in order for the tests to run correctly. Especially the -Zbuild-std tests ― you can almost guarantee failure if the CI server is using an outdated version of host Cargo to run them.

@kennystrawnmusic
Copy link
Author

kennystrawnmusic commented Nov 22, 2021

Well it's compiling now but not linking. Here's where I definitely need help, because the idea that the linker wouldn't be updating even if a target is specified is weird to say the least. It's also bizarre that the linker only does this for built-in target triples and not for custom ones.

What's also bizarre is that aaa_cross_compile_disabled_check has always been failing on my end even with upstream Cargo from the official rust-lang/cargo repository. So again, @ehuss and @phil-opp: you have write access to my fork if you're available to add some hands on deck. And for anyone else who's available to assist, comment here and I will give you write access to my fork all the same.

@bors
Copy link
Contributor

bors commented Nov 26, 2021

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

@Ekleog
Copy link
Contributor

Ekleog commented Nov 29, 2021

Checking my files, it seems to me like what I needed to do in order to be able to locally run cross tests was to have a cross toolchain installed (with nix, using gccMultiStdenv as the shell's stdenv), and installing the i686-unknown-linux-gnu rust target (with nix, overriding rust with { targets = ["i686-unknown-linux-gnu"]; } — if you're also a nix user I can send you my shell.nix if it can help).

Having had a quick look at your code, I must say I'm not sure I understand what you're trying to do with the PerPackageTarget{,Mode} enums: the toml file is already being parsed, so why are you using regexes to re-parse it? My guess would be that this is the reason why your code won't pass CI: eg. in CompileOptions you're reading from the Cargo.toml file, which is not necessarily the one you're interested in, for instance if there's a workspace.

@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 Dec 14, 2021
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.

5 participants