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.

Sign up for GitHub

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

Jump to bottom

Run rustdoc doctests relative to the workspace #8954

Merged
merged 2 commits into from
Dec 7, 2020
Merged

Run rustdoc doctests relative to the workspace #8954

merged 2 commits into from
Dec 7, 2020

Conversation

Swatinem
Copy link
Contributor

@Swatinem Swatinem commented Dec 6, 2020

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

fixes #8097

@Swatinem
Run rustdoc doctests relative to the workspace
2cc2ae8 
By doing so, rustdoc will also emit workspace-relative filenames for
the doctests.

fixes rust-lang#8097
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 6, 2020
@Swatinem Swatinem mentioned this pull request Dec 6, 2020
Open
@alexcrichton
Copy link
Member

@bors: r+

Looks great to me, thanks!

@bors
Copy link
Contributor

bors commented Dec 7, 2020

📌 Commit e729880 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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 7, 2020
@bors
Copy link
Contributor

bors commented Dec 7, 2020

⌛ Testing commit e729880 with merge 8ff53f6...

@bors
Copy link
Contributor

bors commented Dec 7, 2020

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 8ff53f6 to master...

@bors bors merged commit 8ff53f6 into rust-lang:master Dec 7, 2020
This was referenced Dec 7, 2020
Closed
Merged
@ehuss ehuss mentioned this pull request Dec 8, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 8, 2020
@bors
Auto merge of rust-lang#79820 - ehuss:update-cargo, r=ehuss
7913250 
Update cargo

10 commits in 63d0fe43449adcb316d34d98a982b597faca4178..d274fcf862b89264fa2c6b917b15230705257317
2020-12-02 01:44:30 +0000 to 2020-12-07 23:08:44 +0000
- Clarify cargo manifest edition field docs (rust-lang/cargo#8953)
- Run rustdoc doctests relative to the workspace (rust-lang/cargo#8954)
- Workaround fs issue in `cargo publish`. (rust-lang/cargo#8950)
- Fix panic with -Zbuild-std and no roots. (rust-lang/cargo#8942)
- Slightly optimize `cargo vendor` (rust-lang/cargo#8937)
- Fixes rust-lang/cargo#8783 , cargo new fails without a author name or email (rust-lang/cargo#8912)
- Fix test escaping __CARGO_TEST_ROOT (rust-lang/cargo#8929)
- Add period to allowed feature name characters. (rust-lang/cargo#8932)
- faq: small fixes (rust-lang/cargo#8931)
- Fix semver documentation tests. (rust-lang/cargo#8930)
phil-opp added a commit to rust-osdev/bootimage that referenced this pull request Dec 10, 2020
@phil-opp
Fix nightly breakage of doctests in workspaces
54492a9 
In rust-lang/cargo#8954 the working directory for the runner executable changed for doctests. Before, the working directory was the crate dir, now it is the workspace dir. This lead to a bug in bootimage because it now looked for the `test-args` and other config option in the workpace root `Cargo.toml`, instead of the crate-specific `Cargo.toml`.

This commit fixes this by using the `CARGO_MANIFEST_DIR` environment variable for determining the `Cargo.toml` path, instead of directly using `cargo locate-project` as before.
@phil-opp phil-opp mentioned this pull request Dec 10, 2020
Merged
Herschel added a commit to Herschel/ruffle that referenced this pull request Dec 10, 2020
@Herschel
chore: Fix swf doctests working directory
ec3f262 
Rust 12-09 nightly changed the working dir of doctests to be the
workspace root instead of the subcrate root. This broke the
doctests for swf. Now do some setup to ensure the same working
directory on both stable and nightly.

cc rust-lang/cargo#8954
@Herschel Herschel mentioned this pull request Dec 10, 2020
Merged
Herschel added a commit to ruffle-rs/ruffle that referenced this pull request Dec 10, 2020
@Herschel
chore: Fix swf doctests working directory
cec0ca7 
Rust 12-09 nightly changed the working dir of doctests to be the
workspace root instead of the subcrate root. This broke the
doctests for swf. Now do some setup to ensure the same working
directory on both stable and nightly.

cc rust-lang/cargo#8954
@Swatinem Swatinem mentioned this pull request Dec 11, 2020
Merged
@jan-auer
Copy link

The workspace-relative file names are great, but changing the cwd comes with a set of disadvantages:

  • It's no longer consistent with the behavior of tests from tests/ and #[test], which always use the member crate's root as cwd.
  • It changes whether tests are invoked with --workspace or with --package <crate-name>. In the latter case, the cwd is still in the member crate.
  • It breaks existing tests. Repositories currently testing with both nightly and stable, you have to manually implement workarounds, like changing the cwd.

@jan-auer jan-auer mentioned this pull request Dec 17, 2020
Merged
@luser
Copy link
Contributor

luser commented Dec 17, 2020

It's no longer consistent with the behavior of tests from tests/ and #[test], which always use the member crate's root as cwd.

This seems like it will be the most confusing part, although given that doctests and unit tests are already inconsistent, perhaps the better solution would be to make them all use the same CWD?

@jan-auer
Copy link

perhaps the better solution would be to make them all use the same CWD

Agreed. I am not sure how intentional this behavior is, but both integration tests and unit tests already run with the crate root (workspace member) as the CWD. The same used to be true for doc tests, and only changed with this PR for doc tests when run with --workspace.

@alexcrichton
Copy link
Member

Oh dear, sorry for the breakage, that's unintentional!

@jan-auer can you open an issue for the breakage? We may need to revert this PR and reevaluate

@jan-auer jan-auer mentioned this pull request Dec 17, 2020
Closed
@Swatinem Swatinem mentioned this pull request Dec 17, 2020
Closed
@alexcrichton alexcrichton mentioned this pull request Dec 18, 2020
Merged
bors added a commit that referenced this pull request Dec 18, 2020
@bors
Auto merge of #8996 - alexcrichton:revert, r=ehuss
8abc050 
Revert #8954 - changing rustdoc's cwd

This PR reverts #8954 in reference to #8993 and #8992 where there's still definitely a bug to be fixed but we should probably avoid regressing in the meantime.

Closes #8992
@ehuss ehuss mentioned this pull request Dec 23, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 24, 2020
@bors
Auto merge of rust-lang#80322 - ehuss:update-cargo, r=ehuss
0edce6f 
Update cargo

10 commits in a3c2627fbc2f5391c65ba45ab53b81bf71fa323c..75d5d8cffe3464631f82dcd3c470b78dc1dda8bb
2020-12-14 17:21:26 +0000 to 2020-12-22 18:10:56 +0000
- Update git2 (rust-lang/cargo#9009)
- Stabilize RUSTC_WORKSPACE_WRAPPER (rust-lang/cargo#8976)
- Make cargo metadata and tree respect target (rust-lang/cargo#8987)
- Update git2 (rust-lang/cargo#8998)
- Revert rust-lang/cargo#8954 - changing rustdoc's cwd (rust-lang/cargo#8996)
- With debug HTTP mode log curl's version (rust-lang/cargo#8991)
- Reject ambiguous git dependency declaration. (rust-lang/cargo#8984)
- Fix tests not working with a different CARGO_TARGET_DIR. (rust-lang/cargo#8982)
- Add version to credential dependencies. (rust-lang/cargo#8983)
- Clarify FAQ entry wording about lockfiles (rust-lang/cargo#8978)
@Swatinem Swatinem mentioned this pull request Jan 26, 2021
Merged
Swatinem added a commit to Swatinem/cargo that referenced this pull request Feb 22, 2021
@Swatinem
Run rustdoc doctests relative to the workspace
b4c4028 
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
bors added a commit that referenced this pull request Feb 23, 2021
@bors
Auto merge of #9105 - Swatinem:rustdoc-run-cwd, r=alexcrichton
6243e8e 
Run rustdoc doctests relative to the workspace

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
@ehuss ehuss added this to the 1.50.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.50.0
Development

Successfully merging this pull request may close these issues.

cargo doc and cargo test --doc execute rustdoc in different directories.
7 participants
@Swatinem @rust-highfive @alexcrichton @bors @jan-auer @luser @ehuss