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

Run rustdoc doctests relative to the workspace #9105

Merged
merged 1 commit into from
Feb 23, 2021

Conversation

Swatinem
Copy link
Contributor

By doing so, rustdoc will also emit workspace-relative filenames for the doctests.

This was first landed in #8954 but later backed out in #8996 because it changed the CWD of rustdoc test invocations.

The second try relies on the new --test-run-directory rustdoc option which was added in rust-lang/rust#81264 to explicitly control the rustdoc test cwd.

fixes #8993

@rust-highfive
Copy link

r? @Eh2406

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 26, 2021
@Swatinem
Copy link
Contributor Author

r? @alexcrichton

CC @jyn514

This is currently blocked by having a rustdoc that supports that new flag. I did a "rustup update nightly" locally, but either its not in there yet, or cargo does not run the tests with nightly? Not sure… How can I control this?

@rust-highfive rust-highfive assigned alexcrichton and unassigned Eh2406 Jan 26, 2021
@alexcrichton
Copy link
Member

Thanks! Unfortunately due to the use of -Z we won't be able to unconditionally enable this in Cargo. One option is to add a similar -Z flag to Cargo, or we could wait for the rustdoc option to stabilize to land this.

@bors
Copy link
Collaborator

bors commented Feb 3, 2021

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

@ehuss ehuss added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 9, 2021
@Swatinem Swatinem force-pushed the rustdoc-run-cwd branch 4 times, most recently from 58cc481 to c4667b2 Compare February 13, 2021 09:27
@Swatinem Swatinem marked this pull request as ready for review February 13, 2021 10:06
@Swatinem
Copy link
Contributor Author

I updated the code to be compatible with both stable and nightly Rust but adding a few more conditions to the testsuite.

I remember @jyn514 said there is no problem with stabilizing this option in rustdoc immediately as long as we validate that it actually does its job well.

Due to the fact that cargo compiles/runs its testsuite on stable as well, I do think this needs to ride the trains to be turned on in cargo?

@jyn514
Copy link
Member

jyn514 commented Feb 13, 2021

Due to the fact that cargo compiles/runs its testsuite on stable as well, I do think this needs to ride the trains to be turned on in cargo?

Normally nightly tests are skipped when running with a stable toolchain I think. It seems strange to delay this just for the test suite.

p.arg("--crate-name").arg(&unit.target.crate_name());
p.arg("--test");

if nightly_features_allowed() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☝️ @jyn514 This is what I mean; removing this, every test that does a doctest will fail when run against stable rustc (up until it rides the trains)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the problem; why do you want to remove if nightly_features_allowed? That seems like the right check.

If it helps, cargo always runs against the same toolchain version of rustc; if cargo is nightly, rustc will be nightly too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, this should get into stable at some point…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh - that has to wait for rustdoc to stabilize the option. The plan is

  • add the nightly feature in rustdoc
  • pass it automatically in nightly cargo (this PR)
  • stabilize the feature in rustdoc
  • pass the flag unconditionally in cargo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this should probably be gated by a specific -Z flag in Cargo rather than simply unconditionally on the nightly channel

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That'll also make the impact in the tests less because the new changes will have to be opted-into instead of automatically happening on nightly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 fair enough! Rebased and added an unstable option that controls this behavior. That should be well enough for my purposes.

@bors
Copy link
Collaborator

bors commented Feb 13, 2021

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

By doing so, rustdoc will also emit workspace-relative filenames for the doctests.

This was first landed in rust-lang#8954 but later backed out in rust-lang#8996 because it changed the CWD of rustdoc test invocations.

The second try relies on the new `--test-run-directory` rustdoc option which was added in rust-lang/rust#81264 to explicitly control the rustdoc test cwd.

fixes rust-lang#8993
@alexcrichton
Copy link
Member

@bors: r+

Looks good to me, thanks!

@bors
Copy link
Collaborator

bors commented Feb 23, 2021

📌 Commit b4c4028 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Feb 23, 2021
@bors
Copy link
Collaborator

bors commented Feb 23, 2021

⌛ Testing commit b4c4028 with merge 6243e8e...

@bors
Copy link
Collaborator

bors commented Feb 23, 2021

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 6243e8e to master...

@bors bors merged commit 6243e8e into rust-lang:master Feb 23, 2021
@Swatinem Swatinem deleted the rustdoc-run-cwd branch February 24, 2021 08:09
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 27, 2021
Update cargo

11 commits in bf5a5d5e5d3ae842a63bfce6d070dfd438cf6070..572e201536dc2e4920346e28037b63c0f4d88b3c
2021-02-18 15:49:14 +0000 to 2021-02-24 16:51:20 +0000
- Pass the error message format to rustdoc (rust-lang/cargo#9128)
- Fix test target_in_environment_contains_lower_case (rust-lang/cargo#9203)
- Fix hang on broken stderr. (rust-lang/cargo#9201)
- Make it more clear which module is being tested when running cargo test (rust-lang/cargo#9195)
- Updates to edition handling. (rust-lang/cargo#9184)
- Add --cfg and --rustc-cfg flags to output compiler configuration (rust-lang/cargo#9002)
- Run rustdoc doctests relative to the workspace (rust-lang/cargo#9105)
- Add support for [env] section in .cargo/config.toml (rust-lang/cargo#9175)
- Add schema field and `features2` to the index. (rust-lang/cargo#9161)
- Document the default location where cargo install emitting build artifacts (rust-lang/cargo#9189)
- Do not exit prematurely if anything failed installing. (rust-lang/cargo#9185)
@ehuss ehuss added this to the 1.52.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make doctest compile / run cwd consistent with other test kinds
7 participants