-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
don't use force-unstable-if-unmarked with x test in standard library doctests #115186
don't use force-unstable-if-unmarked with x test in standard library doctests #115186
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
I don't think the block I removed makes any sense.. But still, I am not certain about regressions with this change. @bors rollup=never |
This comment has been minimized.
This comment has been minimized.
Okay I see how it makes sense... |
src/bootstrap/builder.rs
Outdated
@@ -1722,10 +1722,6 @@ impl<'a> Builder<'a> { | |||
cargo.env("UPDATE_EXPECT", "1"); | |||
} | |||
|
|||
if !mode.is_tool() { | |||
cargo.env("RUSTC_FORCE_UNSTABLE", "1"); | |||
} |
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.
It certainly makes no sense to remove this without also removing the code that receives this env var
rust/src/bootstrap/bin/rustc.rs
Lines 158 to 160 in 795ade0
if env::var_os("RUSTC_FORCE_UNSTABLE").is_some() { | |
cmd.arg("-Z").arg("force-unstable-if-unmarked"); | |
} |
rust/src/bootstrap/bin/rustdoc.rs
Lines 55 to 57 in 795ade0
if env::var_os("RUSTC_FORCE_UNSTABLE").is_some() { | |
cmd.arg("-Z").arg("force-unstable-if-unmarked"); | |
} |
But this is old code, and I think it is very important. For instance it ensures that if people load the libc crate from the sysroot, they get stability errors, even though libc does not contain any unstable
markers.
It's generally a bad idea to remove code if you cannot explain why it was there. :)
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.
However, if removing this env var helps, that should hopefully get us one step closer to solving the puzzle. Just, the env var has to stay.
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.
Yeah, removing it indeed was a bad idea. Will fix the problem by updating the condition instead of removing it.
It's generally a bad idea to remove code if you cannot explain why it was there. :)
Very true, at the first look it didn't make any sense. But there is the CI report so.. 😄
774cac3
to
aa255bd
Compare
This comment has been minimized.
This comment has been minimized.
Just put the PR mode in draft. Tomorrow I will consider the best way to handle this. |
…ary doctests. Using `force-unstable-if-unmarked makes` bootstrap process ignoring errors, potentially leading to the merging of PRs that do not compile successfully. Signed-off-by: ozkanonur <work@onurozkan.dev>
aa255bd
to
6288739
Compare
@rustbot ready |
I'm not sure this is the right appraoch. Shouldn't rustc be fixed to still complain about the missing feature gate despite |
Regarding to your comment here #114838 (comment), no it should not. If you meaning fixing this from the compiler side, I can add a FIXME note to the change I did. This PR at least ensures not merging the broken changes until the rustc fix |
I guess this will make things more complicated. I will keep this PR draft and let the compiler team fix this problem with the right approach |
Using
force-unstable-if-unmarked
onlibrary/
makes bootstrap ignoring doctests errors, potentially leading to the merging of PRs that do not compile successfully.For testing this change, apply the following patch(or remove one of the
error_in_core
inlibrary/core/src/error.rs
):then run
x test library/core --doc
Related #114838