Skip to content
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

Doctests do not set profile codegen options #6570

Open
bspeice opened this issue Jan 20, 2019 · 5 comments
Open

Doctests do not set profile codegen options #6570

bspeice opened this issue Jan 20, 2019 · 5 comments
Labels
A-doctests Area: rustdoc --test A-profiles Area: profiles C-bug Category: bug S-blocked-external Status: ❌ blocked on something out of the direct control of the Cargo project, e.g., upstream fix

Comments

@bspeice
Copy link

bspeice commented Jan 20, 2019

Problem
As currently stands, there's an inconsistency in doctests running with debug_assertions on even when code was compiled through cargo test --release. I would've expected that cargo test --release means that the doctests get compiled in release mode as well, but that doesn't appear to be the case.

Steps

//! Here's a doctest:
//!
//! ```rust
//! use doctest_da::my_function;
//!
//! if cfg!(debug_assertions) {
//!     assert!(my_function().is_ok());
//! } else {
//!     assert!(my_function().is_err());
//! }
//! ```

pub fn my_function() -> Result<(), ()>{
    if cfg!(debug_assertions) {
        Ok(())
    } else {
        Err(())
    }
}

Notes

I'm a bit confused on where exactly this issue may need to be filed. It seems related to #5777 and #5218, but because this hits doctests, #4669 and rust-lang/rust#45599 may take precedence. Let me know if I should move.

Output of cargo version:

cargo 1.32.0 (8610973aa 2019-01-02)

@bspeice bspeice added the C-bug Category: bug label Jan 20, 2019
bspeice added a commit to bspeice/qadapt that referenced this issue Jan 20, 2019
Uses `qadapt::is_active` to work around rust-lang/cargo#6570.
Currently assumes that *some* allocation will have happened
by the time `fn main()` is called, which is sub-par.
@ehuss
Copy link
Contributor

ehuss commented Jan 21, 2019

It looks like there are few issues here.

  • Cargo doesn't pass codegen options to rustdoc. I don't immediately see a reason not to.
  • In this specific case, it wouldn't help. rustdoc does not parse options the same way rustc does, and seems to always set debug_assertions to true.

@QuietMisdreavus (or @GuillaumeGomez) it looks like there is some command-line parsing and validation done in rustc in build_session_options_and_crate_config that rustdoc does not do. In this example, debug_assertions looks to get unset here. Is there a reason rustdoc doesn't share that code and set up the Options the same?

@Lokathor
Copy link

-C target-cpu in the .cargo/config file also doesn't seem to get passed to doc tests.

@ehuss ehuss changed the title Doctests and test --release disagree about debug_assertions Doctests do not set profile codegen options Oct 25, 2023
@ehuss ehuss added the A-doctests Area: rustdoc --test label Oct 25, 2023
@ehuss ehuss added S-blocked-external Status: ❌ blocked on something out of the direct control of the Cargo project, e.g., upstream fix S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review labels Oct 25, 2023
@ehuss
Copy link
Contributor

ehuss commented Oct 25, 2023

I'm not sure what the current status is of rustdoc handling -C flags in doctests. Someone will need to do an audit to make sure that they are all handled the same way as rustc does.

Cargo's handling of these arguments for rustc is done in a function called build_base_args. The flags that are shared between rustc and rustdoc should probably be split off into a separate function. Arguments for doctests are collected here, that place would likely need to also include the -C codegen options that are appropriate for the doctest (by calling the "separate function" that factored out the appropriate codegen options).

S-blocked-external can be removed if we conclude that rustdoc is handling all the options correctly.

There is some risk that adding these flags will start breaking some projects (like if they change behavior on debug_assertions). I don't know if that risk is great enough for us to do something different (like maybe doing a crater run?). I'm inclined to just change it and respond if people run into problems, but I'd like to hear what others think.

For historical perspective: rustdoc originally did not honor the -C flag at all. At some point, it was refectored to use the same option parsing as rustc, but the codegen options themselves weren't wired into the doctest generation. Over time, those have been added, but I am uncertain if all of them are wired up now, or if all the options are handled the same way as rustc (since there are some subtle issues like how some flags imply other flags).

@ehuss
Copy link
Contributor

ehuss commented Oct 25, 2023

There might also be performance considerations (either positive or negative). Doctests are particularly slow since they essentially require compiling an executable for every test. For some projects with lots of doctests (like the standard library), this can be relatively slow. Compiling doctests with different codegen settings might be faster or slower to compile, and may be faster or slower to execute. I don't think that will be a significant concern, but something I wanted to highlight as something to keep an eye on.

@Lokathor
Copy link

I can say that personally, I've begun actively avoiding all doc tests, and even ripping out existing doc tests, because they are so slow compared to normal tests.

However, assuming your number of doctests doesn't change, then altering the codegen options applied to particular doctests seems unlikely to have a significant effect on compilation time. Any particular doctest is, in general, likely to be extremely small.

@epage epage removed the S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review label Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-doctests Area: rustdoc --test A-profiles Area: profiles C-bug Category: bug S-blocked-external Status: ❌ blocked on something out of the direct control of the Cargo project, e.g., upstream fix
Projects
Status: No status
Development

No branches or pull requests

4 participants