Avoid declaring a fake dependency edge #68298
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
|
To be honest I'm not super happy with this change - feels a bit ad-hoc and I worry it could change under us and we wouldn't notice. Not sure we can do better though. cc @petrochenkov as well |
|
Can you explain more how changing the order the files are printed should fix the issue? |
|
Oh, I was over eager in a refactor, the code should call next() on the iterator instead of pushing the whole iterator on. |
Mark-Simulacrum
force-pushed
the
binary-depdep-fix
branch
from
1b047ff to
9457681
Compare
|
Fixed. |
|
I think an alternate solution, which may be more of an "at the source" fix, might be to avoid reading these files entirely? For example if we find I feel like this sort of route would protect against potential bugs where we don't say we depend on the |
|
I can try to experiment with that path -- I think @petrochenkov planned on trying to do so based on comments on the issue. I do think it's better :) |
Mark-Simulacrum
force-pushed
the
binary-depdep-fix
branch
2 times, most recently
from
12bdd0e to
90ba750
Compare
|
Initial implementation for the proposed "at the source" change is up, though I've not run tests locally. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
This is what I wanted to do, basically. |
petrochenkov
added
S-waiting-on-author
|
I've just realized that if the dependency is a proc macro, then in addition to its (Same applies to plugin dependencies, but |
2 tasks
|
I will refactor/correct the code as you've suggested. I believe it is indeed the case that we don't care about plugin dependencies (yet, at least). I will not try to solve that problem in this PR. |
Mark-Simulacrum
added
S-waiting-on-review
Mark-Simulacrum
force-pushed
the
binary-depdep-fix
branch
from
90ba750 to
0f9f382
Compare
|
The failing test case (https://github.com/rust-lang/rust/blob/master/src/test/ui/rmeta-rpass.rs) appears to assert that when we find an rmeta and rlib in the linker path, we should prefer the rlib. I'm not sure why this is true. Per #10786, we would generally want this to be an error (as the metadata in the two files is incompatible), but I don't think we should load both -- I consider it in an unstable implementation detail which file is ultimately used if you have incompatible files. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
From the PR introducing the test it looks like just something to ensure that the behavior doesn't change in unnoticed way. |
Mark-Simulacrum
force-pushed
the
binary-depdep-fix
branch
from
0f9f382 to
0d0006b
Compare
I wanted to write some short overview of this situation and dependencies, perhaps this weekend. Right now if two versions of the "same library", e.g. "rmeta vs rlib" or "target vs host" in case of "dual proc macros" become unsynchronized, it's pretty much undefined behavior. |
|
Before r+'ing I'd like to get a confirmation from Cargo people that something like cargo check
modify lib.rs
cargo buildwon't produce unsynchronized rmeta (older) and rlib (newer) in the same directory. It looks like the disambiguators (like |
|
r=me otherwise |
petrochenkov
added
S-waiting-on-team
When we're producing an rlib, we do not need anything more than an rmeta file for each of our dependencies (this is indeed utilized by Cargo for pipelining). Previously, we were still storing the paths of possible rlib/dylib crates, which meant that they could still plausibly be accessed. With -Zbinary-dep-depinfo, that meant that Cargo thought that rustc was using both the rlib and an (earlier emitted) rmeta, and so needed a recompile, as the rlib may have finished writing *after* compilation started (for more detail, see issue 68149). This commit changes metadata loading to not store the filepaths of dylib/rlib if we're going to end up creating an rlib only.
Mark-Simulacrum
force-pushed
the
binary-depdep-fix
branch
from
0d0006b to
be663bf
Compare
|
Let's r? @alexcrichton then to hopefully resolve the Cargo question (or maybe @ehuss knows better, not sure :) |
|
@bors: r=petrochenkov Code here looks great to me, thanks!
I believe this is correct, due to all the trickiness involved In any case if I'm remembering incorrectly I suspect we'll figure out quickly! |
|
📌 Commit be663bf has been approved by |
bors
added
S-waiting-on-bors
|
@bors rollup=never to make sure we really figure out quickly. |
bors
added a commit
that referenced
this pull request
Jan 23, 2020
Avoid declaring a fake dependency edge When we're producing an rlib, we do not need anything more than an rmeta file for each of our dependencies (this is indeed utilized by Cargo for pipelining). Previously, we were still storing the paths of possible rlib/dylib crates, which meant that they could still plausibly be accessed. With -Zbinary-dep-depinfo, that meant that Cargo thought that rustc was using both the rlib and an (earlier emitted) rmeta, and so needed a recompile, as the rlib may have finished writing *after* compilation started (for more detail, see issue 68149). This commit changes metadata loading to not store the filepaths of dylib/rlib if we're going to end up creating an rlib only. Fixes #68149.
|
☀️ Test successful - checks-azure |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we're producing an rlib, we do not need anything more than an rmeta file
for each of our dependencies (this is indeed utilized by Cargo for pipelining).
Previously, we were still storing the paths of possible rlib/dylib crates, which
meant that they could still plausibly be accessed. With -Zbinary-dep-depinfo,
that meant that Cargo thought that rustc was using both the rlib and an (earlier
emitted) rmeta, and so needed a recompile, as the rlib may have finished writing
after compilation started (for more detail, see issue 68149).
This commit changes metadata loading to not store the filepaths of dylib/rlib if
we're going to end up creating an rlib only.
Fixes #68149.