Fix doctests linking too many libs.

[ehuss]

Fixes #5650. cc #5435

As part of my recent work on profiles, I introduced some situations where a
library can be compiled multiple times with different settings. Doctests were
greedily grabbing all dependencies for a package, regardless of which target
is was for. This can cause doctests to fail if it links multiple copies of
the same library.

One way to trigger this is cargo test --release if you have dependencies, a
build script, and panic="abort". There are other (more obscure) ways to
trigger it with profile overrides.

[rust-highfive]

r? @matklad

(rust_highfive has picked a reviewer for you, use r? to override)

[ehuss]

Notes:

• Compilation.libraries is no longer used by cargo, but I left it as-is. I noticed that cargo-bloat is using it, I'm not sure if there are other external users needing it. I'm not sure if we should just leave it, remove it, or replace it with something better.
• Let me know if you want a more detailed description of what was happening, this is a little subtle.

[Eh2406]

Don't take this as any kind of review. For my own reasons, I'd appreciate a more detailed explanation. Specifically someday the resolver is going to grow private dependencies witch will lead to the same crate with different features ending up in the build plan. And I want a heads up of the corner cases we will have to fix when we get there. :-P

[matklad]

What do you think about changing this triple to a struct, so as to make comment not-necessary? It might also be worthwhile to move the

if lib.extension() != Some(OsStr::new("rlib")) && !target.for_host() {

logic to the place where we generate these dependencies?

[matklad]

Thanks @ehuss ! The code looks correct to me, but I wonder if we could centralize decisions about doctest dependencies in one place? Currently we have two filtering steps: when genreating deps and then when we construct command line for rustdoc.

I'm not sure if we should just leave it, remove it, or replace it with something better.

In general, it's fine to remove stuff. I'd love to hear @alexcrichton opinion about this specifically though, because I don't know the original rational behind this field.

As this is a bug fix, let's not remove stuff right now, but leave a comment noting that this field is probably unused.

Should this be backported to beta?

[ehuss]

@Eh2406 sure, it's very particular to how the rustdoc libs were collected and a side effect of build scripts, and dealing with panic. I'll try to walk through the issues:

• Scenario: Library foo with panic set in profile.release, a build.rs script, and one dependency named bar. Run cargo test --release. It will fail when running doctests.
• When panic is set in the profile, the code ensures that tests and all their dependencies do not set panic when building. This is done with the flag ProfileFor in unit_dependencies.rs to ensure that all the dependencies get the correct profile (without panic set).
• When computing build.rs dependencies, this code attempts to find dependencies with links set. However, it is setting ProfileFor incorrectly here and poisons the dep-map with incorrect entries -- in this case it adds a dependency from foo-lib (needed by doctests) to bar and the bar entry has panic enabled.
  • This is the part that requires --release, because the Profile used for the "temp" entry matches the foo-lib unit.
• bar gets incorrectly compiled twice:
  • Once as a dependency of the foo unittest.
  • Once as a dependency of the foo lib being compiled for doctests (and this incorrectly sets panic).
• The code for running doctests collects a naive map in Compilation.libraries of the Package mapped to a list of dependencies (here). However, in general we can't guarantee that a library is only compiled once. In this case, it grabs the two bars and rustdoc fails.

There are multiple parts to the solution:

• Don't poison the dep map when checking build script dependencies.
• Ensure that a Doctest Unit is created in the target list so that we can accurately fetch the dependencies needed for running doctests.
• Use a new list in Compilation.to_doc_test that includes the correct, minimal set of correct dependency information for a doctest.

I'm not aware of any other current issues with compiling a crate multiple times with different settings. There is an issue with rmeta files getting stomped on, because the names are not unique, but that's slightly different.

[ehuss]

Should this be backported to beta?

I think that might be a good idea. I'm not sure how often people run test --release, though.

[ehuss]

Note: This exacerbated an existing bug with how "Fresh" is displayed with doctests. There are some scenarios today where cargo test --doc -v would randomly print "Fresh". The issue is that if a "Fresh" Doctest unit happens to be the last to run in the JobQueue, it would always print "Fresh", even if other units for that package had already run. Whether or not it was last was kinda random, so it was unpredictable. The solution was to check for this scenario and skip printing "Fresh" (only if something was already printed, otherwise "Fresh" is printed).

[alexcrichton]

ping @matklad, did you want to take another look at this?

[matklad]

LGTM!

@bors r+

@alexcrichton, should we remove compilation.iibraires? #5651 (comment)

[bors]

📌 Commit 478d1ce has been approved by matklad

[alexcrichton]

Sounds fine by me!

[bors]

⌛ Testing commit 478d1ce with merge 4a8495c...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing 4a8495c to master...