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

[profile.*] not affecting doctests #4251

Closed
Kixunil opened this issue Jul 6, 2017 · 12 comments
Closed

[profile.*] not affecting doctests #4251

Kixunil opened this issue Jul 6, 2017 · 12 comments
Labels
A-profiles Area: profiles C-bug Category: bug Command-doc

Comments

@Kixunil
Copy link

Kixunil commented Jul 6, 2017

There seems to be no way to set profile for doctests. I need it in my two crates - without opt_level = 3 it fails to compile (this is on purpose of course).

I'd suggest to use one of profile.test, profile.doc or create a new one called profile.doctest.

Steps for reproducing:

  • clone the repository
  • remove no_compile from doctest
  • run cargo doc

Expected result: cargo compiles optimized version (without call to non-existing function)
What happened: cargo ignored any profile options and attempted to build doctest without optimization.

@carols10cents carols10cents added A-profiles Area: profiles C-bug Category: bug Command-doc labels Aug 30, 2017
@idubrov
Copy link

idubrov commented Apr 2, 2019

Another use case: no_std crates. I have a doctest with #[no_std] attribute and currently it causes test to fail with:

error: language item required, but not found: eh_personality

What I would like is to use panic to disable stack unwinding via:

[profile.doc]
panic = "abort"

Currently I use #[lang = "eh_personality"] fn eh_personality() {}, but that only works on nightly would only work on nightly.

@idubrov
Copy link

idubrov commented Apr 2, 2019

This link #3166 (comment) add some context why it might not be that easy.

In my case, though, doctest is no_run (because it's for embedded library which you cannot run anyways).

paulvt added a commit to paulvt/stm32f4disc-demo that referenced this issue Apr 3, 2019
Set an option for running doctests so that they can be run without
the "`eh_personality` language item not found" popping up.
See also: rust-lang/cargo#4251
@weihanglo
Copy link
Member

By using custom profile, we can have a profile for doctest or whatever, so we probably don't need an extra built-in profile. This solves the first half of the issue.

The other half is that there is a discrepancy between rustc and rustdoc handling (see #6570 (comment)) options, we track it with #6570. For panic behaviour in test/doctest driven by libtest, please follow the progress of -Z panic-abort-tests.

Going to close this now. If you found this is a fault, please comment, and we'll consider re-open.

@Kixunil
Copy link
Author

Kixunil commented Oct 13, 2022

If a custom profile is used it wouldn't be used by cargo doc by default, right? I think being able to use whichever profile cargo doc uses is more important than having a custom profile.

@weihanglo
Copy link
Member

This issue was created before custom profiles got stabilized, so I assumed you didn't know the feature at that time. Now it looks like even with the ability to customize profiles, a built-in one for doc/doctest is still the thing you are looking for, right? I'd like to know more about your opinion on having a built-in versus customizing your own. Could you share more from your side? Thank you!

@Kixunil
Copy link
Author

Kixunil commented Oct 13, 2022

Basically my goal is to get opt-level = 3 whenever someone calls cargo doc. Whether it's achieved by a custom profile that gets auto-set or a built-in profile or even reusing test I don't care (1). Note that cargo doc --profile custom_profile_name is not great. The idea is for it to work correctly out of the box.

1: I guess having a separate built-in profile may be nicer architecture but not sure if it's worth the hassle.

@weihanglo
Copy link
Member

Sorry, I don't really get it. What do you want to achieve with cargo doc? Or did you mean cargo test --doc?

Assumed you meant cargo test --doc. If we do go with a built-in doc profile, why should it be with opt-level = 3 but not 0? Does it inherit all settings from test or release profile? This might become a bikeshedding problem as I see it.

I'd suggest using the combos of config [alias] and custom profile to achieve at this moment. That is,

  1. Customize a doctest profile in Cargo.toml
    [profile.doctest]
    inherits = "test" # or inherit form any profile 
    opt-level = 3
  2. Put an alias in .cargo/config.toml under project root, and run with the custom profile
    [alias]
    doctest = "test --doc --profile doctest"

Unfortunately, Cargo doesn't allow an alias to shadow built-in commands. Instead, people need to run cargo doctest directly.

On the other hand, as aforementioned, rustdoc doesn't handle all compiler settings in the same way as rustc, so we may need to fix that before adding any built-in profile for rustdoc. Otherwise people will get confused by those flags not working the same.

@Kixunil
Copy link
Author

Kixunil commented Oct 13, 2022

Sorry, yes, I meant cargo test --doc.

why should it be with opt-level = 3

it shouldn't by default, only when overridden by Cargo.toml and I want to override it in that crate.

so we may need to fix that before adding any built-in profile for rustdoc

Indeed.

@andylizi
Copy link

From my perspective, I would much prefer building doctests with the same profile as unit tests (i.e. test) 1. This is because cargo test will build both unit tests and doctests in one command, which can lead to strange behavior like this:

#[cfg(test)]
compile_error!("cfg(test)");

#[cfg(not(test))]
compile_error!("cfg(not(test))");

Running cargo test for the above code will result in confusing error messages:

error: cfg(not(test))
 --> src\lib.rs:5:1
  |
5 | compile_error!("cfg(not(test))");
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: cfg(test)
 --> src\lib.rs:2:1
  |
2 | compile_error!("cfg(test)");
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: could not compile `playground` due to previous error
warning: build failed, waiting for other jobs to finish...
error: could not compile `playground` due to previous error

How can test be both enabled and not at the same time? Of course, we know the reason is that doctests are not build with test, but I'd imagine most user wouldn't know this detail, which isn't even documented anywhere (AFAIK) and is technically contradictory to what the cargo reference claims:

The test profile is the default profile used by cargo test. The test profile inherits the settings from the dev profile.

(By the way, cargo test --profile test doesn't solve this issue, for some reason.)

True, the above situation rarely happens in real life—I believe this would come up only when compiler settings (like cfg(test) or opt-level in OP's case) actually have an effect on whether the code is able to compile. In my case, I discovered this when I was writing a no_std crate, where conditionals like #![cfg_attr(not(test), no_std)] are commonly used. I remember spending a lot of time trying to figure out why cfg(test) just won't take effect, and ended up disabling all doctests.

Footnotes

  1. although this would sadly be a breaking change……

@weihanglo
Copy link
Member

weihanglo commented Oct 13, 2022

Thank @andylizi for pointing out one concern I have. Currently, profile selection is on a command-basis. All rustc invocations of a Cargo command invocation should receive the same set of flags (theoretically, see #6570). If we decide to differentiate doctests from unit tests, it would probably complicate Cargo internals, and also confuses users as they now need to remember the special case of doctest since it will be a feature.

Personally I lean toward not doing so. cargo test --doc --profile <custom-profile> should be sufficient, and I am sorry you cannot shadow built-in command with an alias to minimize characters typed.


@andylizi said:

which isn't even documented anywhere (AFAIK) and is technically contradictory to what the cargo reference claims:

From my understanding, Cargo compiles doctests and unittests under the same profiles when executing cargo test. [cfg(test)] is nothing related to profiles. Rustc knows nothing about profiles. The compile failure is because lib target need to be built so doctests can use it, whereas unit tests don't need that.

If you run cargo test --verbose, you should observe two rustc invocations from cargo. One with --test rustc flag, the other without. And then the follow up rustdoc call compiles doctests with the lib artifact from the previous rustc invocation. You can read this old comment #10361 (comment) to get more info of the execution model, but this should be a separate issue if we want to proceed.

@Kixunil
Copy link
Author

Kixunil commented Oct 13, 2022

Agree with not making it a separate profile, test is good. So once [profile.test] is picked up by doc tests my issue is resolved.

@andylizi
Copy link

[cfg(test)] is nothing related to profiles. Rustc knows nothing about profiles. The compile failure is because lib target need to be built so doctests can use it, whereas unit tests don't need that.

Ah I see. I had thought it was profile-related because panic = "abort" didn't seem to be working (it keep asking for eh_personality), and I just assumed it must be related to that... Thanks for clarifying!

The problem I encountered was a long time ago and I've forgot the exact details (maybe something to do with #[cfg(test)] extern crate std;), although I'm pretty sure in the end I only workarounded it by setting doctest = false—but that's not really a proper solution. And for what it's worth, making cargo test play nice with no_std have always been a huge pain, and some setups seem outright impossible. But that probably belongs to another issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-profiles Area: profiles C-bug Category: bug Command-doc
Projects
None yet
Development

No branches or pull requests

5 participants