Track panic in Unit early.
#6170
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 has picked a reviewer for you, use r? to override) |
|
📌 Commit 8648994 has been approved by |
bors
added a commit
that referenced
this pull request
Oct 15, 2018
Track `panic` in Unit early. This is an alternate solution for #5445. It ensures that `panic` is cleared in the Profile for "for_host" targets (proc-macro/plugin/build-scripts) and dependencies. This helps avoid unnecessary recompiles in some situations (though does add extra units in some situations, see below). Some examples where extra rebuilds are now avoided: * A workspace with a dependency shared with normal and build-deps. `build --all` should build everything, and then `build -p foo` was causing a recompile because the shared dep was no longer in the `used_in_plugin` set. Now it should not recompile. * `panic=abort`, with a shared dependency in build and dev, `build` would cause that shared dependency to be built twice (exactly the same without panic set). Now it should only build once. Some examples where `panic` is now set correctly: * `panic=abort`, with a binary with a shared dependency in build and normal, `test` would cause that shared dependency to be built twice (exactly the same without panic set). Now it is still built twice, but the one for the normal (bin) dependency will correctly have `panic` set. Some examples where new units are now generated: * `panic=abort`, with shared dependency between normal and proc-macro or build. Previously the shared dependency was built once with `panic=unwind`. Now it is built separately (once with `panic`, once without). I feel like that this is more correct behavior — that now the normal dependency avoids adding landing pads. For `panic=abort` cross-compiling, this makes no difference in compile time since it was already built twice. Additional notes: - I left build-scripts with the old behavior where `panic` is cleared for it and all its dependencies. There could be arguments made to change that (#5436), but it doesn't seem important to me. - Renamed and refactored `ProfileFor` to `UnitFor`. I expect this API to continue to evolve in the future. Closes #6143, closes #6154.
|
☀️ Test successful - status-appveyor, status-travis |
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.
This is an alternate solution for #5445. It ensures that
panicis cleared in the Profile for "for_host" targets (proc-macro/plugin/build-scripts) and dependencies. This helps avoid unnecessary recompiles in some situations (though does add extra units in some situations, see below).Some examples where extra rebuilds are now avoided:
build --allshould build everything, and thenbuild -p foowas causing a recompile because the shared dep was no longer in theused_in_pluginset. Now it should not recompile.panic=abort, with a shared dependency in build and dev,buildwould cause that shared dependency to be built twice (exactly the same without panic set). Now it should only build once.Some examples where
panicis now set correctly:panic=abort, with a binary with a shared dependency in build and normal,testwould cause that shared dependency to be built twice (exactly the same without panic set). Now it is still built twice, but the one for the normal (bin) dependency will correctly havepanicset.Some examples where new units are now generated:
panic=abort, with shared dependency between normal and proc-macro or build. Previously the shared dependency was built once withpanic=unwind. Now it is built separately (once withpanic, once without). I feel like that this is more correct behavior — that now the normal dependency avoids adding landing pads.For
panic=abortcross-compiling, this makes no difference in compile time since it was already built twice.Additional notes:
panicis cleared for it and all its dependencies. There could be arguments made to change that (Shouldbuild.rsbe compiled like plugins? #5436), but it doesn't seem important to me.ProfileFortoUnitFor. I expect this API to continue to evolve in the future.Closes #6143, closes #6154.