Add rustc folder to dynamic linker search path #1499

06393993 · 2024-05-13T15:32:40Z

... so that we can run cargo-nextest with proc-macro projects on Windows to fix #1493.

This commit also adds integration tests for running cargo-nextest for proc-macro projects.

codecov · 2024-05-13T17:53:09Z

Codecov Report

Attention: Patch coverage is 98.89503% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 79.14%. Comparing base (f8a5918) to head (c83105f).

❗ Current head c83105f differs from pull request most recent head e9d36d9

Please upload reports for the commit e9d36d9 to get more accurate results.

Files	Patch %	Lines
nextest-runner/src/list/rust_build_meta.rs	98.07%	4 Missing ⚠️
nextest-runner/src/rustc_cli.rs	97.87%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1499      +/-   ##
==========================================
+ Coverage   78.46%   79.14%   +0.68%     
==========================================
  Files          75       76       +1     
  Lines       18076    18569     +493     
==========================================
+ Hits        14183    14697     +514     
+ Misses       3893     3872      -21

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sunshowers

This looks really, really great. Thank you for all your hard work! Just a few comments but I think this is ready to go.

One general comment is that this won't yet work with archived builds, because we don't archive the sysroot. But I'd like to document that for now, and address that after getting a fix out to stop the bleeding.

fixtures/nextest-tests/proc-macro-test/src/lib.rs

nextest-metadata/src/test_list.rs

sunshowers · 2024-05-13T22:45:54Z

nextest-metadata/src/test_list.rs

@@ -485,7 +485,7 @@ pub struct RustBuildMetaSummary {

    /// The target platforms used while compiling the Rust artifacts.
    #[serde(default)]
-    pub target_platforms: Vec<PlatformSummary>,
+    pub target_platforms: Vec<BuildPlatformsSummary>,


Ahhh, I see. I misunderstood what compatibility meant in this context, and I apologize for that. I think we do want to keep compatibility around in this case.

Also, this was my terrible oversight when I first put in target_platforms, I wish I'd have added this BuildPlatformsSummary struct at the time. I humbly apologize for the extra work you have to do here.

As far as handling this goes, I can think of three general directions.

Write out the old target_platforms field, and add a new field (say targets) which has the full context. Deprecate target_platforms and ask users to use the new field if it's present. (This may want to be an Option<Vec<BuildPlatformsSummary>>. (This is similar to target_platform below.)

Hopefully, this adds enough indirection that will be the last time we'd need to do that.

Retain compatibility with the old fields in https://docs.rs/target-spec/3.1.0/src/target_spec/summaries.rs.html#33 by manually duplicating them, and add the sysroot as another field next to it.

One way to do this would be to use serde(flatten) -- but that requires internal buffering, and in general, serde has numerous issues with internal buffering.

Another way would be to manually duplicate the fields. But this is a challenge for two reasons. One is that the struct is #[non_exhaustive] https://docs.rs/target-spec/3.1.0/src/target_spec/summaries.rs.html#32, and two is that new semantically-important fields could be added to target-spec in the future. (I'm the maintainer of target-spec as well, so I can make changes to it if they make sense.)

The former we could probably add some kind of builder for, but the latter is uncomfortably easy to miss details for.

We could have some kind of convoluted PlatformSummaryBuilder in target-spec that produces an error if not all fields were present, I guess. But I'm not convinced of the value of all that extra work.

Add another vector next to target_platforms called (say) target_platforms_extra, and promise that it's the same length as target_platforms and corresponds 1:1 to it (i.e. users can zip the two vectors).

I think between all of these options I like 1 the most. It is a little bit of extra work, but sets us up best for the future. (At some point in the future, we'll probably remove both target_platform and target_platforms with the appropriate warnings.)

Adopt approach 1, but name the field as target_platforms_v2 to make it explicit that target_platforms is deprecated. WDYT?

nextest-runner/src/platform.rs

nextest-runner/src/list/rust_build_meta.rs

fixtures/nextest-tests/proc-macro-test/src/lib.rs

nextest-runner/src/platform.rs

cargo-nextest/src/dispatch.rs

06393993

We probably can only merge this PR late this week, as I don't have much extra time working on this. The plan for the next step is:

Create another commit that creates a builder for BuildPlatforms and RustCli
- BuildPlatformsBuilder will make the ctor of BuildPlatforms better, so that we can create from
  - only target(useful for test and backward comaptible with the old version of RustBuildMeta);
  - target and host libdir without target libdir; and
  - target, host libdir and any number of the target libdir
- RustCli to streamline the call to rustc as we now probably will call it multiple times.
Use rustc --print target-libdir instead of sysroot.
Integrate the new types with BaseApp::new. However I currently don't plan to actually support multiple targets here, as this is outside the scope of this PR.

nextest-runner/src/platform.rs

sunshowers · 2024-05-14T18:24:35Z

I really, really appreciate all the work you've done here. Thank you for this kindness.

Totally understand if you don't have more time, there's just a fractal of complexity here. Please feel free to do as much as you can, and if you don't have time before this weekend, I'll probably pick it up on Saturday or Sunday and finish it up.

nextest-runner/src/platform.rs

nextest-runner/src/lib.rs

nextest-runner/src/list/rust_build_meta.rs

06393993 · 2024-05-18T03:09:42Z

nextest-runner/src/platform.rs

+    pub host_libdir: Option<Utf8PathBuf>,
+
+    /// The target platform, if specified.
+    pub target: Option<BuildPlatformTarget>,


@sunshowers This design makes the builder complicate, e.g.:

let mut builder = BuildPlatformBuilder::default(); let mut target_builder = builder.set_target(TargetTriple::x86_64_unknown_linux_gnu()); target_builder.set_libdir(Cursor::new("/fake/target/libdir")); target_builder.add(); let build_platform = builder .build() .expect("should build with target successfully");

The alternative design is a flat struct

pub struct BuildPlatform { pub host: Platform, pub host_libdir: Option<Utf8PathBuf>, pub target_libdir: Option<Utf8PathBuf>, pub target_triple: Option<TargetTriple>, }

and the previous build code can be rewritten as

let mut builder = BuildPlatformBuilder::default(); builder .set_target_triple(TargetTriple::x86_64_unknown_linux_gnu()) .set_target_libdir(Cursor::new("/fake/target/libdir")); let build_platform = builder .build() .expect("should build with target successfully");

Which way do you think is better?

Well -- the flat struct, as your proposed it, allows for the invalid state where target_libdir is Some(_) but target_triple is None. So I'd strongly prefer the BuildPlatformTarget pattern you have.

I'm not quite sure the builder pattern is pulling its weight though. Can the setters just live directly on the BuildPlatforms and BuildPlatformTarget structs?

nextest-runner/src/platform.rs

06393993 · 2024-05-18T03:11:27Z

nextest-runner/src/target_runner.rs

@@ -27,11 +27,11 @@ impl TargetRunner {
    /// or via a `CARGO_TARGET_{TRIPLE}_RUNNER` environment variable
    pub fn new(
        configs: &CargoConfigs,
-        build_platforms: &BuildPlatforms,


@sunshowers I rename BuildPlatforms to BuildPlatform as now we decided to not include an array of targets in BuildPlatform. WDYT?

Ah, I've always imagined the plural in BuildPlatforms to consist of "host + target". So I think I'd prefer to keep it that way.

Done. Revert the name change.

06393993 · 2024-05-18T03:17:10Z

There seems to be still something wrong with the path setting per the CI result as of now. I will try to reproduce the issue locally and fix.

06393993 · 2024-05-18T03:32:57Z

Actually the CI "Test with latest nextest release" step fails because the CI upgrades to the latest rustup, and when using the latest nextest release, it suffers from the exact same issue this PR tries to fix. Will try to add the env suggested in #1493 (comment) to pass the CI. Once the latest nextest release includes this PR, we come up with another PR to revert the env change.

However, with the env change, this PR can't be tested in the CI, at least the integration test almost won't catch anything in this change, we have to delay until the next PR to revert the env change to check if this PR has any issue. But we pass the "Test with locally built nextest" step in https://github.com/nextest-rs/nextest/actions/runs/9136713176/job/25125843092 without the env change, and this should give us some confidence.

... to WA nextest-rs#1493 for the "Test with latest nextest release" step on Windows temporarily.

sunshowers

Looks generally great, just a few more comments. Could you let me know within the next day or so whether you'll have time to get this finished up? If not then I can address them myself and land this.

Thanks again for all your work.

nextest-metadata/src/test_list.rs

nextest-runner/src/lib.rs

sunshowers · 2024-05-19T21:00:16Z

nextest-runner/src/config/test_helpers.rs

@@ -4,7 +4,7 @@
 use crate::{
    cargo_config::{TargetDefinitionLocation, TargetTriple, TargetTripleSource},
    config::{CustomTestGroup, TestGroup},
-    platform::BuildPlatforms,
+    platform::{BuildPlatform as NextestBuildPlatform, BuildPlatformTarget},


Any reason for the NextestBuildPlatform alias?

No, because I used to change the name to BuildPlatform and it collides with the name of gnuppy::graph::cargo::BuildPlatform.

Now that we decide to change the name back to BuildPlatforms, no need to alias the name to avoid the collision.

sunshowers · 2024-05-19T21:00:58Z

nextest-runner/src/target_runner.rs

@@ -27,11 +27,11 @@ impl TargetRunner {
    /// or via a `CARGO_TARGET_{TRIPLE}_RUNNER` environment variable
    pub fn new(
        configs: &CargoConfigs,
-        build_platforms: &BuildPlatforms,


Ah, I've always imagined the plural in BuildPlatforms to consist of "host + target". So I think I'd prefer to keep it that way.

nextest-runner/src/platform.rs

sunshowers · 2024-05-19T21:11:51Z

nextest-runner/src/platform.rs

    ///
+    /// Used to set the dynamic linker search path when running the test executables. If we fail to
+    /// parse the input, the libdir won't be set.
+    pub fn set_host_libdir(&mut self, reader: impl io::BufRead) {


I'd rather this just take a Utf8PathBuf, seems a bit too implementation-dependent to take the stdout of rustc here.

Changed to set_host_libdir_from_rustc_output for setting from the output. If we just want to set it, just use mutate the struct. WDYT?

sunshowers · 2024-05-19T21:12:41Z

nextest-runner/src/platform.rs

+    pub host_libdir: Option<Utf8PathBuf>,
+
+    /// The target platform, if specified.
+    pub target: Option<BuildPlatformTarget>,


Well -- the flat struct, as your proposed it, allows for the invalid state where target_libdir is Some(_) but target_triple is None. So I'd strongly prefer the BuildPlatformTarget pattern you have.

I'm not quite sure the builder pattern is pulling its weight though. Can the setters just live directly on the BuildPlatforms and BuildPlatformTarget structs?

06393993 · 2024-05-20T04:15:56Z

Looks generally great, just a few more comments. Could you let me know within the next day or so whether you'll have time to get this finished up? If not then I can address them myself and land this.

Thanks again for all your work.

I do have time to address any further comments in the next week as I don't expect any big changes ahead.

sunshowers

Okay, just a couple more changes to go and then we can get this out there 🥳

sunshowers · 2024-05-21T00:36:07Z

nextest-metadata/src/test_list.rs

+    /// The libdir for the host platform.
+    ///
+    /// Empty if failed to discover.
+    #[serde(default)]
+    pub host_libdir: Option<Utf8PathBuf>,


Hmm, I'm not quite sure this is right (for no fault of your own).

Currently, it looks like we were serializing either the target platform if --target is specified, or the host platform if not. I think that's wrong (and it's my fault I'm pretty sure :) ), and that this libdir stuff forces the issue.

So, in general, there are:

exactly 1 host platform with its libdir

Some N >= 0 target platforms with its libdir

Storing N host libdirs doesn't seem right. I think what we should do here is:

// host and target platforms are the same for now, but it's // quite possible we'll start storing target-specific data at some point pub struct HostPlatformSummary { pub platform: PlatformSummary, pub libdir: Option<Utf8PathBuf>, } pub struct TargetPlatformSummary { pub platform: PlatformSummary, pub libdir: Option<Utf8PathBuf>, } pub struct BuildPlatformsSummary { pub host: HostPlatformSummary, pub targets: Vec<TargetPlatformSummary>, } // and in RustBuildMetaSummary: pub struct RustBuildMetaSummary { // ... #[serde(default)] platforms: Option<BuildPlatformsSummary>, // ... }

I think this solves all the problems, and also is independent justification to remove the "v2" stuff -- we're now clearly storing the host and target platforms separately, rather than smooshing them together.

also is independent justification to remove the "v2" stuff -- we're now clearly storing the host and target platforms separately, rather than smooshing them together.

IIUC, this new platforms field still makes both target_platform and target_platforms deprecated. Is that correct?

Does this implicitly imply that BuildPlatforms::host should respect the new HostPlatformSummary::platform?

In addition, what should be the interface of from_summary be like? from_summary(summary: BuildPlatformsSummary) -> Result<BuildPlatforms> or from_summary(summary: BuildPlatformsSummary) -> Result<Vec<BuildPlatforms>>? In the former case of from_summary(summary: BuildPlatformsSummary) -> Result<BuildPlatforms>, should we change BuildPlatforms::target from Option<BuildPlatformsTarget> to Vec<BuildPlatformsTarget>?

IIUC, this new platforms field still makes both target_platform and target_platforms deprecated. Is that correct?

Yeah.

Does this implicitly imply that BuildPlatforms::host should respect the new HostPlatformSummary::platform?

Yep, while performing deserialization.

In addition, what should be the interface of from_summary be like? from_summary(summary: BuildPlatformsSummary) -> Result<BuildPlatforms> or from_summary(summary: BuildPlatformsSummary) -> Result<Vec<BuildPlatforms>>?

The former.

In the former case of from_summary(summary: BuildPlatformsSummary) -> Result<BuildPlatforms>, should we change BuildPlatforms::target from Option<BuildPlatformsTarget> to Vec<BuildPlatformsTarget>?

Just error out if there's more than one.

Just error out if there's more than one.

Which error should I return? RustBuildMetaParseError::PlatformDeserializeError or RustBuildMetaParseError::UnknownHostPlatform or should I create another enum variant? In case of using existing enum variant, how do I generate the source target_spec::Error or should I change the variant to be another enum which includes a MultipleTargets variant?

Another variant on RustBuildMetaParseError is fine.

Done.

Introduce new types HostPlatformSummary and TargetPlatformSummary

Deserialize BuildPlatforms::host from HostPlatformSummary::platform

Introduce a new RustBuildMetaParseError variant, which is returned if the argument of from_summary(summary: BuildPlatformsSummary) -> Result<BuildPlatforms> has more than one entries in the targets field.

sunshowers · 2024-05-21T00:38:14Z

nextest-runner/src/platform.rs

+/// cargo-nextest represents the host triple with `None` during runtime. However the
+/// build-metadata might be used on a system with a different host triple. Therefore, the host
+/// triple is detected if `target_triple` is `None`.
+impl From<BuildPlatforms> for BuildPlatformsSummary {


Have to be honest, I'm generally hesitant about From impls like this, only liking them for a Liskov-like "is-a" substitutions. Could you turn them back into from_summary/to_summary as before? It's not a huge deal but just makes it slightly more explicit.

Changed to ToSummary and FromSummary trait, while still leave the to_summary_str and from_summary_str in the impl block. Using traits allows me to use the same method name to_summary/from_summary for both the old PlatformSummary type and the new BuildPlatformsSummary type.

WDYT? Let me know if you feel strong that we shouldn't introduce new traits.

* Add a `BuildPlatformsTarget` to store the target triple. We will store target libdir in this struct in a following commit. * Change `BuildPlatforms::new` to be called without any arguments, which make it easer to create `BuildPlatforms` when there is no target, especially for testing. * Create a serialized form of `BuildPlatforms`: `BuildPlatformsSummary` * Change `discover_build_platforms` to `discover_target_triple` which only discovers the `TargetTriple`

... which stream lines the call to the `rustc` binary. `RustcCli` will be used to decide the host and target libdir in a following commit.

... so that we can run cargo-nextest with proc-macro projects on Windows. This commit also adds integration tests that run cargo-nextest with a proc-macro project.

sunshowers

Thanks for all your work. I'll land this and fix up the last couple of things.

nextest-metadata/src/test_list.rs

nextest-runner/src/cargo_config/target_triple.rs

cwfitzgerald · 2024-05-23T05:15:22Z

Appreciate you both for getting this through!

06393993 force-pushed the main branch from 9890eff to 9c98c32 Compare May 13, 2024 17:48

sunshowers reviewed May 13, 2024

View reviewed changes

06393993 commented May 13, 2024

View reviewed changes

nextest-runner/src/list/rust_build_meta.rs Show resolved Hide resolved

fixtures/nextest-tests/proc-macro-test/src/lib.rs Outdated Show resolved Hide resolved

nextest-runner/src/platform.rs Outdated Show resolved Hide resolved

sunshowers reviewed May 14, 2024

View reviewed changes

cargo-nextest/src/dispatch.rs Outdated Show resolved Hide resolved

sunshowers mentioned this pull request May 14, 2024

Effects of RUSTUP_WINDOWS_PATH_ADD_BIN change rust-lang/rustup#3825

Open

06393993 force-pushed the main branch 3 times, most recently from 945b6bc to ccf511a Compare May 14, 2024 18:16

06393993 commented May 14, 2024

View reviewed changes

nextest-runner/src/platform.rs Outdated Show resolved Hide resolved

06393993 commented May 16, 2024

View reviewed changes

nextest-runner/src/platform.rs Show resolved Hide resolved

Sec-ant mentioned this pull request May 16, 2024

fix(ci): nextest on windows biomejs/biome#2887

Merged

06393993 force-pushed the main branch from ccf511a to afeb9b0 Compare May 18, 2024 02:40

06393993 commented May 18, 2024

View reviewed changes

06393993 force-pushed the main branch 3 times, most recently from 24b0f99 to 6628491 Compare May 18, 2024 16:33

build: add RUSTUP_WINDOWS_PATH_ADD_BIN: 1

c7536ff

... to WA nextest-rs#1493 for the "Test with latest nextest release" step on Windows temporarily.

06393993 force-pushed the main branch from 6628491 to fe43b19 Compare May 18, 2024 16:40

sunshowers reviewed May 19, 2024

View reviewed changes

06393993 force-pushed the main branch 4 times, most recently from a18f428 to e2ca0b0 Compare May 20, 2024 04:09

sunshowers reviewed May 21, 2024

View reviewed changes

06393993 added 2 commits May 20, 2024 23:55

Add RustcCli

5fc24e0

... which stream lines the call to the `rustc` binary. `RustcCli` will be used to decide the host and target libdir in a following commit.

Add rustc libdir folders to dynamic linker search path

c83105f

... so that we can run cargo-nextest with proc-macro projects on Windows. This commit also adds integration tests that run cargo-nextest with a proc-macro project.

06393993 force-pushed the main branch from e2ca0b0 to c83105f Compare May 21, 2024 07:25

sunshowers approved these changes May 22, 2024

View reviewed changes

nextest-metadata/src/test_list.rs Outdated Show resolved Hide resolved

nextest-runner/src/cargo_config/target_triple.rs Show resolved Hide resolved

Update nextest-metadata/src/test_list.rs

e9d36d9

sunshowers merged commit 40c2dd3 into nextest-rs:main May 22, 2024
11 checks passed

sunshowers mentioned this pull request May 23, 2024

macos: dyld: Library not loaded: @rpath/libtest-77ee8c29c330e4a3.dylib #267

Closed

EliahKagan mentioned this pull request May 23, 2024

Revert "Work around nextest macro lib compatibility bug" GitoxideLabs/gitoxide#1376

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rustc folder to dynamic linker search path #1499

Add rustc folder to dynamic linker search path #1499

06393993 commented May 13, 2024

codecov bot commented May 13, 2024 •

edited

Loading

sunshowers left a comment •

edited

Loading

sunshowers May 13, 2024

06393993 May 14, 2024

06393993 left a comment •

edited

Loading

sunshowers commented May 14, 2024

06393993 May 18, 2024

sunshowers May 19, 2024

06393993 May 18, 2024

sunshowers May 19, 2024

06393993 May 20, 2024

06393993 commented May 18, 2024 •

edited

Loading

06393993 commented May 18, 2024

sunshowers left a comment

sunshowers May 19, 2024

06393993 May 20, 2024

sunshowers May 19, 2024

sunshowers May 19, 2024

06393993 May 20, 2024

sunshowers May 19, 2024

06393993 commented May 20, 2024

sunshowers left a comment

sunshowers May 21, 2024

06393993 May 21, 2024

06393993 May 21, 2024 •

edited

Loading

06393993 May 21, 2024 •

edited

Loading

sunshowers May 21, 2024

06393993 May 21, 2024

sunshowers May 21, 2024

06393993 May 21, 2024

sunshowers May 21, 2024

06393993 May 21, 2024

sunshowers left a comment

cwfitzgerald commented May 23, 2024

Add rustc folder to dynamic linker search path #1499

Add rustc folder to dynamic linker search path #1499

Conversation

06393993 commented May 13, 2024

codecov bot commented May 13, 2024 • edited Loading

Codecov Report

sunshowers left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

06393993 left a comment • edited Loading

Choose a reason for hiding this comment

sunshowers commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

06393993 commented May 18, 2024 • edited Loading

06393993 commented May 18, 2024

sunshowers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

06393993 commented May 20, 2024

sunshowers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

06393993 May 21, 2024 • edited Loading

Choose a reason for hiding this comment

06393993 May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshowers left a comment

Choose a reason for hiding this comment

cwfitzgerald commented May 23, 2024

codecov bot commented May 13, 2024 •

edited

Loading

sunshowers left a comment •

edited

Loading

06393993 left a comment •

edited

Loading

06393993 commented May 18, 2024 •

edited

Loading

06393993 May 21, 2024 •

edited

Loading

06393993 May 21, 2024 •

edited

Loading