-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
expand RUSTC_WRAPPER docs #10896
expand RUSTC_WRAPPER docs #10896
Conversation
r? @ehuss (rust-highfive has picked a reviewer for you, use r? to override) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks nicer to me! Thank you.
[`build.rustc-wrapper`] to set via config. Setting this to the empty string | ||
overwrites the config and resets cargo to not use a wrapper. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Workspace wrapper can also be reset with the same trick. However, this seems to be an undocumented behaviour, attributed to these lines.
IMHO, if users start to rely on an undocumented but reasonable behaivour, we should treat it as a feature and document it well, like what you just did.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the reset is reasonable to document. Could you mention it for rustc-workspace-wrapper as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. If this is going to be documented, adding some tests to prevent regression would be better. Sorry for the reply bombing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you mention it for rustc-workspace-wrapper as well?
Done.
However I won't have the time to add tests, sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is what you were referring to, but I opened #10899 to update the test to make this behavior explicitly tested.
Looks good, thanks! @bors r+ |
☀️ Test successful - checks-actions |
Update cargo 5 commits in d8d30a75376f78bb0fabe3d28ee9d87aa8035309..85b500ccad8cd0b63995fd94a03ddd4b83f7905b 2022-07-19 13:59:17 +0000 to 2022-07-24 21:10:46 +0000 - Make the empty rustc-wrapper test more explicit. (rust-lang/cargo#10899) - expand RUSTC_WRAPPER docs (rust-lang/cargo#10896) - Stabilize Workspace Inheritance (rust-lang/cargo#10859) - Fix typo in unstable docs: s/PROGJCT/PROJECT/ (rust-lang/cargo#10890) - refactor(source): Open query API for adding more types of queries (rust-lang/cargo#10883)
Fixes #10886