Clear out target directory if compiler has changed #67760
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
|
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 |
|
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 |
|
Tentatively suspecting #67077 to (indirectly) be at fault; previously we were always clearing out stage1 and so forth due to codegen backends getting rebuilt, but that's no longer something we can depend on. The test failure in this PR seems to come from clearing out directories too aggressively (and too broadly, I suspect); I'm still investigating locally a better fix. |
Mark-Simulacrum
force-pushed
the
rustc-dirty
branch
from
e19b756
to
da8d24b
Compare
|
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 |
Mark-Simulacrum
force-pushed
the
rustc-dirty
branch
from
d56cbcd
to
0a09df7
Compare
Previously, we relied fully on Cargo to detect that the compiler had changed and it needed to rebuild the standard library (or later "components"). This used to not quite be the case prior to moving to LLVM be a separate cargo invocation; subsequent compiles would recompile std and friends if LLVM had changed (rust-lang#67077 is the PR that changes things here). This PR moves us to clearing out libstd when it is being compiled if the rustc we're using has changed. We fairly harshly limit the cases in which we do this (e.g., ignoring dry run mode, and so forth, as well as rustdoc invocations). This is primarily because when we're not using the compiler directly, so clearing out in other cases is likely to lead to bugs, particularly as our deletion scheme is pretty blunt today (basically removing more than is needed, i.e., not just the rustc artifacts). In practice, this targeted fix does fix the known bug, though it may not fully resolve the problem here. It's also not clear that there is a full fix hiding here that doesn't involve a more major change (like -Zbinary-dep-depinfo was). As a drive-by fix, don't delete the compiler before calling Build::copy, as that also deletes the compiler.
Mark-Simulacrum
force-pushed
the
rustc-dirty
branch
from
0a09df7
to
ccd8c8c
Compare
|
Okay, this has been updated with a working version. I'm not fully happy with the very 'odd' looking fix, but as the PR description (and commit) indicate, I don't know of a better approach here, nor do I really have the will to try to dig in further unless it's needed (e.g., we find another case where this PR's approach falls down). Feedback is of course appreciated :) |
|
@bors: r+ I don't have a bunch of time to investigate this too closely right now so I'm largely just trusting your investigation! |
|
📌 Commit ccd8c8c has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jan 8, 2020
Clear out target directory if compiler has changed Previously, we relied fully on Cargo to detect that the compiler had changed and it needed to rebuild the standard library (or later "components"). This used to not quite be the case prior to moving to LLVM be a separate cargo invocation; subsequent compiles would recompile std and friends if LLVM had changed (#67077 is the PR that changes things here). This PR moves us to clearing out libstd when it is being compiled if the rustc we're using has changed. We fairly harshly limit the cases in which we do this (e.g., ignoring dry run mode, and so forth, as well as rustdoc invocations). This is primarily because when we're not using the compiler directly, so clearing out in other cases is likely to lead to bugs, particularly as our deletion scheme is pretty blunt today (basically removing more than is needed, i.e., not just the rustc artifacts). In practice, this targeted fix does fix the known bug, though it may not fully resolve the problem here. It's also not clear that there is a full fix hiding here that doesn't involve a more major change (like -Zbinary-dep-depinfo was). As a drive-by fix, don't delete the compiler before calling Build::copy, as that also deletes the compiler.
|
☀️ Test successful - checks-azure |
|
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
tmandry
added a commit
to tmandry/rust
that referenced
this pull request
Jan 24, 2020
…ing, r=alexcrichton Clear out std, not std tools This was a typo that slipped in, and meant that we were still not properly clearing out std. This is basically rust-lang#67760 but actually correct...
tmandry
added a commit
to tmandry/rust
that referenced
this pull request
Jan 24, 2020
…ing, r=alexcrichton Clear out std, not std tools This was a typo that slipped in, and meant that we were still not properly clearing out std. This is basically rust-lang#67760 but actually correct...
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.
Previously, we relied fully on Cargo to detect that the compiler had changed and
it needed to rebuild the standard library (or later "components"). This used to
not quite be the case prior to moving to LLVM be a separate cargo invocation;
subsequent compiles would recompile std and friends if LLVM had changed
(#67077 is the PR that changes things here).
This PR moves us to clearing out libstd when it is being compiled if the rustc
we're using has changed. We fairly harshly limit the cases in which we do this
(e.g., ignoring dry run mode, and so forth, as well as rustdoc invocations).
This is primarily because when we're not using the compiler directly, so
clearing out in other cases is likely to lead to bugs, particularly as our
deletion scheme is pretty blunt today (basically removing more than is needed,
i.e., not just the rustc artifacts).
In practice, this targeted fix does fix the known bug, though it may not fully
resolve the problem here. It's also not clear that there is a full fix hiding
here that doesn't involve a more major change (like -Zbinary-dep-depinfo was).
As a drive-by fix, don't delete the compiler before calling Build::copy, as that
also deletes the compiler.