Prevent rustdoc feature doctests

[GuillaumeGomez]

Part of #61351

cc @ollie27

[Centril]

.filter(..).cloned() seems cleaner here.

[Mark-Simulacrum]

I continue to be deeply confused why we're altering test_args here.

test_args is the array of arguments that's passed to test::test_main -- that's way too late for cfg to matter, I believe, and test_main certainly is not a compiler.

Can you at least check manually that this works? A comment that explains how this works (e.g., why is this applying to cfg, despite not mentioning cfg at all?) would also be good.

[GuillaumeGomez]

I just thought about it when you said something was wrong in this implementation the other day: it's incorrect since I don't check the cfg. However, test_args are passed to each code sample rustdoc compiles and run. Therefore, it seems accurate to me to have this check here. However I need to fix it.

[Mark-Simulacrum]

But... they're not? These are not the arguments to rustc. These are the arguments to the test 'binary'. I would expect you to never find a --cfg in these arguments; they're not (AFAICT) used at all for compilation. Right now the code is removing a test filter for rustdoc -- not a cfg gate or anything like that.

[GuillaumeGomez]

Then I completely misunderstood this part of the code... I'll take a step back and check what I missed...

[GuillaumeGomez]

Indeed, I wasn't looking at the good place at all... Thanks a lot for catching up my error @Mark-Simulacrum !

[Mark-Simulacrum]

This also feels like the wrong place :)

I would expect us to not add the rustdoc cfg ourselves -- looking at it, I guess here -- not remove it later on. We don't want to remove it if the user has passed it.

[GuillaumeGomez]

That's a good point I didn't think about... Checking more globally why we even need to do this. However, I think we need it at some point to check for the documentation build (not the tests build). I'm looking around to confirm it.

[GuillaumeGomez]

@Mark-Simulacrum Just like you suggested, instead of adding it into the options directly, I removed it from there and add it before the doc build. Sorry about all the fuss...

[GuillaumeGomez]

ping @Mark-Simulacrum

[Mark-Simulacrum]

@bors r+

[bors]

📌 Commit e4e674bec91e364f290ab8b708d960a292d2678a has been approved by Mark-Simulacrum

[ollie27]

Shouldn't cfg(rustdoc) be set when looking for doctests as well:

rust/src/librustdoc/test.rs

Line 67 in bf8491e

crate_cfg: config::parse_cfgspecs(options.cfgs.clone()),

@bors r-

We need tests for this, a rustdoc-ui test like the following should be sufficient:

// build-pass// compile-flags:--test// normalize-stdout-test: "src/test/rustdoc-ui" -> "$$DIR"#![feature(doc_cfg)]// Make sure `cfg(rustdoc)` is set when finding doctests but not inside the doctests./// ```/// #![feature(doc_cfg)]/// assert!(!cfg!(rustdoc));/// ```#[cfg(rustdoc)]pubstructFoo;

[Mark-Simulacrum]

r? @ollie27

I assumed that setting it in core would've done that...

[GuillaumeGomez]

I did as well since we use this one for it. Adding the test.

[GuillaumeGomez]

@ollie27 It seems like the test passed whereas I didn't change the code. Did we misunderstood your message?

[ollie27]

If you look at the stdout it shows that it didn't actually run the doctest. It should say "running 1 test".

[GuillaumeGomez]

Oh I see! Indeed, good catch!

[GuillaumeGomez]

Now we have it as well.

[QuietMisdreavus]

This looks like it's ready to go, and that @ollie27's test has been added and is working, so i'm going to push this along...

@bors r+

[bors]

📌 Commit 366fdeb has been approved by QuietMisdreavus