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

[rustdoc] stabilize cfg(doctest) #63803

Open
wants to merge 1 commit into
base: master
from

Conversation

@GuillaumeGomez
Copy link
Member

commented Aug 22, 2019

Fixes #62210.

Since we removed rustdoc from providing cfg(test) on test runs, it's been replaced by cfg(doctest). It'd be nice to have it in not too far in the future.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:stabilize-doctest branch from 8946805 to 2e000c0 Aug 22, 2019

@GuillaumeGomez GuillaumeGomez changed the title stabilize cfg(doctest) [rustdoc] stabilize cfg(doctest) Aug 22, 2019

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 22, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-08-22T09:30:48.5798182Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-22T09:30:48.6018608Z ##[command]git config gc.auto 0
2019-08-22T09:30:48.6095962Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-22T09:30:48.6145030Z ##[command]git config --get-all http.proxy
2019-08-22T09:30:48.6296509Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/63803/merge:refs/remotes/pull/63803/merge
2019-08-22T09:30:50.6930153Z remote:                                                                                         
---
2019-08-22T09:31:24.0770250Z do so (now or later) by using -b with the checkout command again. Example:
2019-08-22T09:31:24.0770518Z 
2019-08-22T09:31:24.0771003Z   git checkout -b <new-branch-name>
2019-08-22T09:31:24.0771256Z 
2019-08-22T09:31:24.0771495Z HEAD is now at 9096e1a21 Merge 2e000c03bb60d32e49cb3ece7a36d7194c74ee90 into 42dcd4b7c5fb7b61bc2f4c0842f66e5ad40057e4
2019-08-22T09:31:24.0931973Z ##[section]Starting: Collect CPU-usage statistics in the background
2019-08-22T09:31:24.0935108Z ==============================================================================
2019-08-22T09:31:24.0935183Z Task         : Bash
2019-08-22T09:31:24.0935226Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-08-22T09:37:55.2917135Z     Finished release [optimized] target(s) in 1m 29s
2019-08-22T09:37:55.2992930Z tidy check
2019-08-22T09:37:56.2276996Z * 578 error codes
2019-08-22T09:37:56.2290719Z * highest error code: E0733
2019-08-22T09:37:56.5981484Z tidy error: Found 1 features without a gate test.
2019-08-22T09:37:56.5982197Z Expected a gate test for the feature 'cfg_doctest'.
2019-08-22T09:37:56.5982472Z Hint: create a failing test file named 'feature-gate-cfg_doctest.rs'
2019-08-22T09:37:56.5982715Z       in the 'ui' test suite, with its failures due to
2019-08-22T09:37:56.5982767Z       missing usage of `#![feature(cfg_doctest)]`.
2019-08-22T09:37:56.5982994Z Hint: If you already have such a test and don't want to rename it,
2019-08-22T09:37:56.5983222Z       you can also add a // gate-test-cfg_doctest line to the test file.
2019-08-22T09:37:57.2811687Z some tidy checks failed
2019-08-22T09:37:57.2812652Z 
2019-08-22T09:37:57.2812652Z 
2019-08-22T09:37:57.2813762Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-08-22T09:37:57.2814237Z 
2019-08-22T09:37:57.2814275Z 
2019-08-22T09:37:57.2821073Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-08-22T09:37:57.2821346Z Build completed unsuccessfully in 0:01:33
2019-08-22T09:37:57.2821346Z Build completed unsuccessfully in 0:01:33
2019-08-22T09:37:57.2873477Z == clock drift check ==
2019-08-22T09:37:57.2883640Z   local time: Thu Aug 22 09:37:57 UTC 2019
2019-08-22T09:37:57.3729628Z   network time: Thu, 22 Aug 2019 09:37:57 GMT
2019-08-22T09:37:57.3729920Z == end clock drift check ==
2019-08-22T09:37:58.7510558Z ##[error]Bash exited with code '1'.
2019-08-22T09:37:58.7558292Z ##[section]Starting: Checkout
2019-08-22T09:37:58.7560117Z ==============================================================================
2019-08-22T09:37:58.7560187Z Task         : Get sources
2019-08-22T09:37:58.7560232Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@RalfJung

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

Does it make sense to open a PR before the discussion mentioned at #62210 (comment) has been had?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2019

@RalfJung We already validated to stabilize cfg(test) in rustdoc before removing it because of issues. Therefore, we also said that the new feature should be cfg(doctest). From this point, I guess we already had the discussion?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2019

Oh and I fixed the failure, just waiting for github to update the display I guess...

@RalfJung

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

@GuillaumeGomez do you have pointers to that discussion?

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:stabilize-doctest branch from 9472a47 to 0f91e98 Aug 22, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2019

Isn't the issue description enough? And the fact that we stabilized the cfg(test) too?

src/libsyntax/feature_gate.rs Outdated Show resolved Hide resolved
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2019

☔️ The latest upstream changes (presumably #63824) made this pull request unmergeable. Please resolve the merge conflicts.

@Kixiron

This comment has been minimized.

Copy link

commented Aug 26, 2019

Ping from triage. @GuillaumeGomez any progress?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2019

Need to fix conflict (again) but beside that I'm waiting for someone to approve.

@Dylan-DPC Dylan-DPC removed the S-blocked label Aug 29, 2019

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:stabilize-doctest branch from b859350 to 116185e Sep 3, 2019

collecting doctests, so you can utilize doctest functionality without forcing the test to appear in
docs, or to find an arbitrary private item to include it on.

If you add `#![feature(cfg_doctest)]` to your crate, Rustdoc will set `cfg(doctest)` when collecting

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Sep 4, 2019

Member

Since we're stabilizing this, I think the note here about features should be removed, right?


If you add `#![feature(cfg_doctest)]` to your crate, Rustdoc will set `cfg(doctest)` when collecting
doctests. Note that they will still link against only the public items of your crate; if you need to
test private items, unit tests are still the way to go.

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Sep 4, 2019

Member

Maybe something like "either make items conditionally public via cfg(doctest) or write a unit test"?

collecting doctests, so you can utilize doctest functionality without forcing the test to appear in
docs, or to find an arbitrary private item to include it on.

If you add `#![feature(cfg_doctest)]` to your crate, Rustdoc will set `cfg(doctest)` when collecting

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Sep 4, 2019

Member

I would avoid the "collecting" language I think -- maybe "when compiling a crate for use in doctests, rustdoc will set cfg(doctest).

/// let x = my_crate::MyStruct(-5);
/// ```
#[cfg(doctest)]
pub struct MyStructOnlyTakesUsize;

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Sep 4, 2019

Member

It would be good to add a note somewhere that this struct isn't actually exposed in anyway, i.e., it won't be in documentation or in the public API of this crate.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

@GuillaumeGomez Let's also initiate FCP (should be @rfcbot fcp merge), since this is a stabilization.

I think modulo FCP this PR is good to go from a stabilization/documentation standpoint; I would like to think some more about it but I can do that async with FCP and such.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:stabilize-doctest branch from 116185e to 1174308 Sep 4, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2019

I made the suggested changes. Thanks for reviewing!

@GuillaumeGomez Let's also initiate FCP (should be @rfcbot fcp merge), since this is a stabilization.

I think modulo FCP this PR is good to go from a stabilization/documentation standpoint; I would like to think some more about it but I can do that async with FCP and such.

Completely fine by me. Let's go for it!

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

commented Sep 4, 2019

Team member @GuillaumeGomez has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

In thinking about this a bit more I think it may make sense to point users at https://crates.io/crates/compiletest_rs as an alternative to doctests when using cfg(doctest), as it's essentially the same functionality (but perhaps packaged more nicely). I'd personally suggest that we not add this feature in favor of compiletest, but I may be biased given that I usually work in the compiler tree where compiletest is already well integrated (and, indeed, the primary mode of testing).

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2019

That is not the same thing at all: compiletext extracts test and compile them outside. If it runs like most crates doing the same thing, it means you'll have to rebuild your crate every time you want to test it. Also, why adding an external dependency on something that can be done directly through rustdoc?

And last point (minor but still very important): a few crates are using the cfg(doctest) feature and are stuck to nightly because of it since a while.

We need to stabilize. It should have been done through adding cfg(test) in rustdoc a few versions back but it got reverted and updated to this and now it's stuck here. I'd really like for this to move forward.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

We discussed this some on Discord with @GuillaumeGomez and I wanted to write up a quick summary. Essentially, the primary use case for this feature in the wild today is to include a README or similar file only during doctests for the purpose of checking its doctests.

I raised the point that this would ideally be done by Cargo, not via a snippet in everyone's lib.rs or similar, but I think the important point here that was raised is that this feature is useful regardless for other reasons, and that Cargo can always implement this functionality anyway.

Overall, I feel that this feature is indeed small enough that we should stabilize it at this point since it's so low-cost.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

In thinking about this a bit more I think it may make sense to point users at https://crates.io/crates/compiletest_rs as an alternative to doctests when using cfg(doctest)

Unfortunately, compiletest_rs is currently not very maintained, and severely lacking behind the in-tree compiletest, see e.g. laumann/compiletest-rs#123 and laumann/compiletest-rs#182.

@abonander

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

I ran into a use for cfg(doctest) or similar to enable a test_util module (which is marked cfg(test)) for doctests; a utility module for (doc)tests might sound like an antipattern but it comes in handy with async APIs which need a lot more supporting code in tests and examples.

@abonander

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

Unfortunately, compiletest_rs is currently not very maintained, and severely lacking behind the in-tree compiletest, see e.g. laumann/compiletest-rs#123 and laumann/compiletest-rs#182.

Trybuild is the alternative that comes up a lot and it works quite well from my experience: https://github.com/dtolnay/trybuild

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2019

☔️ The latest upstream changes (presumably #64264) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:stabilize-doctest branch from 1174308 to 47f2308 Sep 8, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

☔️ The latest upstream changes (presumably #63118) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.