Add host dependency path via -L for cargo_test. #4447

lucaskolstad · 2017-08-29T11:22:21Z

Proc-macro crates' dependencies in the host dependency directory cannot
be found when running cargo test with the --target {target} flag
set as reported in #4224. This adds the host dependency directory to the
search path to find those missing dependencies with -L when building tests
and adds a test case that fails before and passes after this patch.

A new function find_host_deps(..) is added to accomplish this which can
determine the path of the host dependencies directory from within
run_doc_tests(..) where the missing library search path is needed.

Fixes #4224
Fixes #4254

Modeled after a similar patch: a298346

Concerns

The test case seems to require a non-local crate from crates.io to example the failure before this patch. Couldn't make it fail with simply another local subcrate, but if others think it's possible that'd be great. This means that during tests for the cargo project itself that this test case actually downloads and compiles a crate, which I don't think any other tests do and is obviously not ideal and is perhaps even unacceptable. I've used the base64 crate pretty arbitrarily, but which crate it is really doesn't matter to test the case's content. So if anyone knows a tiny or empty crate on crates.io to use instead that'd speed this up and if someone can figure out how to make it fail before this patch is applied without downloading an external crate that would help as well.

Also, I'm not 100% confident about the find_host_deps(..) style and whether it's the best way to find the host dependencies directory with just the TestOptions and Compilation structs available since I'm new to this project. Any comments are appreciated.

Proc-macro crates' dependencies in the host dependency directory cannot be found when running `cargo test` with the `--target {target}` flag set. This adds the host dependency directory with -L when building tests and a test case that fails before and passes after this patch. A new function find_host_deps(..) is added to accomplish this which can determine the path of the host dependencies directory from within run_doc_tests(..) where the missing library search path is needed. Fixes #4224 Fixes #4254

rust-highfive · 2017-08-29T11:22:33Z

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

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

lucaskolstad · 2017-08-29T18:09:16Z

src/cargo/ops/cargo_test.rs

@@ -144,6 +145,8 @@ fn run_doc_tests(options: &TestOptions,
            p.arg("--test").arg(lib)
             .arg("--crate-name").arg(&crate_name);

+            p.arg("-L").arg(&host_deps);


It occurred to me that this line may need to be further down to put the host directory after the others? Does the ordering of the -L options mean this is buggy despite passing tests?

alexcrichton · 2017-08-30T19:29:51Z

tests/test.rs

+            proc-macro = true
+
+            [dependencies]
+            base64 = "^0.6"


Throughout a number of other tests you'll see calls to Package::new(..).publish() which allow having a "fake registry" during testing without actually pulling in a dependency from crates.io, perhaps that can be used here instead?

alexcrichton · 2017-08-30T19:30:24Z

src/cargo/ops/cargo_test.rs

@@ -198,3 +201,24 @@ fn run_doc_tests(options: &TestOptions,
    }
    Ok((Test::Doc, errors))
 }
+
+fn find_host_deps(options: &TestOptions, compilation: &Compilation) -> OsString {


Instead of being calculated here, could this perhaps be a field of the Compilation returned?

alexcrichton · 2017-08-30T19:30:32Z

Thanks so much for the PR!

This patch removes the addition of the find_host_deps() function by adding a host_deps_output field to the Compilation struct instead. The test case is altered to not use an external crate from crates.io but instead use the Package.publish(..) method.

lucaskolstad · 2017-08-30T22:27:05Z

I've removed the find_host_deps(..) function and added a host_deps_output field to the Compilation struct instead that is similar to the deps_output field. I noted that the field is somewhat identical to plugins_dylib_path and is initialized in the same place with the same value even. But I was thinking this is used for different reasons and maybe it's best to be different? Not sure if you want me to just use plugins_dylib_path and rename it or something.

Also, there is a confusing problem with the Package::new(..).publish() method you suggested and I switched to. It seems to make the test case no longer actually test the case! When I comment out the lines in run_doc_tests(..) where the host deps directory linkage is added, it still passes! However, if I add a base64 dependency to proc_macro_dep/Cargo.toml and an extern crate base64 in proc_macro_dep/src/lib.rs, it WILL test the case again and commenting out the lines causes a failure.

No idea what is going on with that tbh. Seems I genuinely might need an external crates.io crate... or maybe I'm just confused.

alexcrichton · 2017-08-30T22:57:04Z

Hm can you gist the error you see with the base64 crate? #4224 seems to indicate the failure is when you cargo test a proc-macro crate itself?

Also yeah actually, let's stick to using plugins_dylib_path or just renaming that to host_deps_output!

lucaskolstad · 2017-08-30T23:28:58Z

Here is an explanatory gist with 1. the exact lines commented out, 2. the exact code for the altered test case, and 3. the error output.

I'm not sure, does this mean the dep_of_proc_macro_dep third crate needs a dependency to be able to test the case?

lucaskolstad · 2017-08-30T23:44:09Z

I tried adding a dependency below dep_of_proc_macro_dep in the same way using Package::new(..).publish() and specifying a Cargo.toml and lib.rs for dep_of_proc_macro_dep that externs the dependency and it just won't fail. With and without commented out lines in the source, I have never seen it fail unless adding an external crate like base64 still.

The plugins_dylib_path field on Compilation is removed because it is identical to host_deps_output, it is only used in one easily replaced location, and because host_deps_output is a more general name that includes its new usage location in cargo_test.rs as well while better matching the corresponding deps_output field. Also de-indents erroneously indented lines in a test case.

lucaskolstad · 2017-08-31T01:13:49Z

I've made the choice and pushed a commit to remove the plugins_dylib_path field entirely and replaced it with host_deps_output in the single place it is used. I think host_deps_output is more general and has a more clear relationship to the deps_output field when used in places like run_doc_tests(..).

This should not affect any of the discussion above about the weird external vs local crate problem in the test case though.

alexcrichton · 2017-08-31T14:35:04Z

Oh I think I see what's going on, try doing:

Package::new("foo", "0.1.0").publish();
Package::new("bar", "0.1.0")
    .dep("foo", "0.1")
    .file("src/lib.rs", "extern crate foo;")
    .publish();

and then have the procedural macro depend on bar?

Minor change causes the test to actually test the case instead of always passing regardless of the fix that adds the host dependency directory to the library search path.

lucaskolstad · 2017-08-31T18:00:22Z

You're right that did it! Thank you so much for helping me figure all this out.

If the CI testing passes, I think this PR is done then. Let me know if you want me to squash the last little commit or make any other changes.

alexcrichton · 2017-08-31T20:26:53Z

@bors: r+

Nice!

Nah leaving as is is fine by me :)

bors · 2017-08-31T20:26:54Z

📌 Commit 95c6e68 has been approved by alexcrichton

bors · 2017-08-31T20:27:01Z

⌛ Testing commit 95c6e68 with merge f519d3d...

…e, r=alexcrichton Add host dependency path via -L for cargo_test. Proc-macro crates' dependencies in the host dependency directory cannot be found when running `cargo test` with the `--target {target}` flag set as reported in #4224. This adds the host dependency directory to the search path to find those missing dependencies with -L when building tests and adds a test case that fails before and passes after this patch. A new function `find_host_deps(..)` is added to accomplish this which can determine the path of the host dependencies directory from within `run_doc_tests(..)` where the missing library search path is needed. Fixes #4224 Fixes #4254 Modeled after a similar patch: a298346 **Concerns** The test case seems to require a non-local crate from crates.io to example the failure before this patch. Couldn't make it fail with simply another local subcrate, but if others think it's possible that'd be great. This means that during tests for the cargo project itself that this test case actually downloads and compiles a crate, which I don't think any other tests do and is obviously not ideal and is perhaps even unacceptable. I've used the base64 crate pretty arbitrarily, but which crate it is really doesn't matter to test the case's content. So if anyone knows a tiny or empty crate on crates.io to use instead that'd speed this up and if someone can figure out how to make it fail before this patch is applied without downloading an external crate that would help as well. Also, I'm not 100% confident about the `find_host_deps(..)` style and whether it's the best way to find the host dependencies directory with just the `TestOptions` and `Compilation` structs available since I'm new to this project. Any comments are appreciated.

bors · 2017-08-31T21:08:07Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing f519d3d to master...

This includes rust-lang/cargo#4447 which fixes a bug in Cargo that is needed to fix rust-lang#44237.

@alexcrichton

Update cargo This includes rust-lang/cargo#4447 which fixes a bug in Cargo that is needed to fix #44237. r? @alexcrichton

rust-highfive assigned alexcrichton Aug 29, 2017

lucaskolstad commented Aug 29, 2017

View reviewed changes

alexcrichton reviewed Aug 30, 2017

View reviewed changes

Tweak proc_macro test to correctly test case.

95c6e68

Minor change causes the test to actually test the case instead of always passing regardless of the fix that adds the host dependency directory to the library search path.

bors merged commit 95c6e68 into rust-lang:master Aug 31, 2017

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Sep 2, 2017

Update cargo.

1b758fb

This includes rust-lang/cargo#4447 which fixes a bug in Cargo that is needed to fix rust-lang#44237.

Mark-Simulacrum mentioned this pull request Sep 2, 2017

Update cargo rust-lang/rust#44259

Merged

bors added a commit to rust-lang/rust that referenced this pull request Sep 2, 2017

Auto merge of #44259 - Mark-Simulacrum:update-cargo, r=alexcrichton

8d83c15

Update cargo This includes rust-lang/cargo#4447 which fixes a bug in Cargo that is needed to fix #44237. r? @alexcrichton

ehuss added this to the 1.22.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add host dependency path via -L for cargo_test. #4447

Add host dependency path via -L for cargo_test. #4447

lucaskolstad commented Aug 29, 2017

rust-highfive commented Aug 29, 2017

lucaskolstad Aug 29, 2017

alexcrichton Aug 30, 2017

alexcrichton Aug 30, 2017

alexcrichton commented Aug 30, 2017

lucaskolstad commented Aug 30, 2017 •

edited

Loading

alexcrichton commented Aug 30, 2017

lucaskolstad commented Aug 30, 2017 •

edited

Loading

lucaskolstad commented Aug 30, 2017

lucaskolstad commented Aug 31, 2017 •

edited

Loading

alexcrichton commented Aug 31, 2017

lucaskolstad commented Aug 31, 2017 •

edited

Loading

alexcrichton commented Aug 31, 2017

bors commented Aug 31, 2017

bors commented Aug 31, 2017

bors commented Aug 31, 2017

Add host dependency path via -L for cargo_test. #4447

Add host dependency path via -L for cargo_test. #4447

Conversation

lucaskolstad commented Aug 29, 2017

rust-highfive commented Aug 29, 2017

lucaskolstad Aug 29, 2017

Choose a reason for hiding this comment

alexcrichton Aug 30, 2017

Choose a reason for hiding this comment

alexcrichton Aug 30, 2017

Choose a reason for hiding this comment

alexcrichton commented Aug 30, 2017

lucaskolstad commented Aug 30, 2017 • edited Loading

alexcrichton commented Aug 30, 2017

lucaskolstad commented Aug 30, 2017 • edited Loading

lucaskolstad commented Aug 30, 2017

lucaskolstad commented Aug 31, 2017 • edited Loading

alexcrichton commented Aug 31, 2017

lucaskolstad commented Aug 31, 2017 • edited Loading

alexcrichton commented Aug 31, 2017

bors commented Aug 31, 2017

bors commented Aug 31, 2017

bors commented Aug 31, 2017

lucaskolstad commented Aug 30, 2017 •

edited

Loading

lucaskolstad commented Aug 30, 2017 •

edited

Loading

lucaskolstad commented Aug 31, 2017 •

edited

Loading

lucaskolstad commented Aug 31, 2017 •

edited

Loading