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

Tracking Issue for -Z doctest-in-workspace #9427

Closed
ehuss opened this issue Apr 28, 2021 · 13 comments · Fixed by #12221
Closed

Tracking Issue for -Z doctest-in-workspace #9427

ehuss opened this issue Apr 28, 2021 · 13 comments · Fixed by #12221
Labels
A-doctests Area: rustdoc --test C-tracking-issue Category: A tracking issue for something unstable. disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns. T-cargo Team: Cargo to-announce

Comments

@ehuss
Copy link
Contributor

ehuss commented Apr 28, 2021

Summary

Original issue: #8097 #8993
Implementation: #9105
Documentation: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#doctest-in-workspace

The -Z doctest-in-workspace flag changes the behavior of the current working directory used when running doctests. Historically, Cargo has run rustdoc --test relative to the root of the package, with paths relative from that root. However, this is inconsistent with how rustc and rustdoc are normally run in a workspace, where they are run relative to the workspace root. This inconsistency causes problems in various ways, such as when passing RUSTDOCFLAGS with relative paths, or dealing with diagnostic output.

The -Z doctest-in-workspace flag causes cargo to switch to running rustdoc from the root of the workspace. It also passes the --test-run-directory to rustdoc so that when running the tests, they are run from the root of the package. This preserves backwards compatibility and is consistent with how normal unittests are run.

Unresolved issues

None at this time.

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

cc @Swatinem

@weihanglo
Copy link
Member

I wonder what's the current status of this feature. It seems that there is nothing left but stabilizing rustdoc --test-run-directory flag. Should we propose a stabilization on rustdoc's side?

@Swatinem
Copy link
Contributor

I would love to!

… and even go a step further and maybe make this the default in a future edition?

@weihanglo
Copy link
Member

Out of curious. What's the consequence of making it the default in terms of compatibility from users' perspective. I can see one that all relative paths of test output format switch to workspace-relative.

@Swatinem
Copy link
Contributor

That is the most obvious user visible change indeed. Otherwise it is mostly implementation details of how the doctests are being built, which I believe is mostly a positive change, as the original motivation for this flag is that doctests are built the same way as other tests.

But as there is some kind of change, making that opt-in via an edition is IMO a good way forward.

@Swatinem
Copy link
Contributor

I opened up a PR to stabilize rustdoc --test-run-directory and wrote a stabilization report.

Doing so I realized that the rustdoc flag itself is just an implementation detail of the doctest-in-workspace cargo flag.

The cargo flag itself is used to opt into fixing an inconsistency in how different categories of tests are compiled and executed.

@ehuss would you agree to push for stabilization of the cargo flag? And to eventually change the default from opt-in to opt-out? If there is backwards compatibility concerns, that can be done as an edition change.

I believe the only user visible change here is the command line output when running doctests, otherwise this should only be implementation details.

Also, can you think of any way to stabilize this cargo feature without necessarily having to stabilize the underlying rustdoc feature?

@ehuss
Copy link
Contributor Author

ehuss commented Oct 30, 2022

I would lean towards just changing it without an opt-in/out. I don't expect there to be widespread breakage, and it is clearly not working as it should, and thus I would categorize this as a bug fix.

Unfortunately using new rustdoc flags can be difficult, so I typically wait until they hit stable to switch Cargo over. That means if --test-run-directory can be stabilized in 1.67, then Cargo can start using it in 1.69. That means landing in Cargo's master branch some time late January and reaching stable in late April.

Also, can you think of any way to stabilize this cargo feature without necessarily having to stabilize the underlying rustdoc feature?

I think --test-run-directory is essentially required. An alternate solution could be --persist-doctests and have cargo handle running the tests, but I would prefer to not do that.

@ehuss
Copy link
Contributor Author

ehuss commented Jun 2, 2023

@rust-lang/cargo I'm going to propose stabilizing this feature now that --test-run-directory is available on stable.

I think the risk of this causing breakage is relatively low. With this feature enabled, tests run in the same directory as they did before. It just affects things like relative paths in RUSTDOCFLAGS. There aren't a lot of flags that have relative paths, and it seems unlikely that most of them are being used when running doctests. However, there is a definite risk so I wanted to start this to poll the team to see what you think.

Unfortunately I can't think of a good way to test this, since we don't have a good way to try out various workspaces. I don't see any issues with the rust-lang/rust repository, but more testing there is probably warranted.

If the risk seems too large, then we could make this an option that gets flipped in the next edition, but I'm reluctant to do that for a change that seems like it shouldn't affect too many people.

Note: I did find one thing that is broken, --persist-doctests doesn't work, see rust-lang/rust#112191. I hope that a fix for that should be easy. That flag is nightly-only.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jun 2, 2023

Team member @ehuss 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.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Jun 2, 2023
@Swatinem
Copy link
Contributor

Swatinem commented Jun 2, 2023

Thanks @ehuss for creating the fcp.

The PR to actually stabilize/enable this is in #12221

@weihanglo
Copy link
Member

Is it required that TOOL in --runtool <TOOL> becomes relative to --test-run-directory?

Given that is also a nightly feature, I have no concern right now.

@rfcbot reviewed

@ehuss
Copy link
Contributor Author

ehuss commented Jun 2, 2023

That's a good point about --runtool. I opened rust-lang/rust#112210 since it looks like it is not working correctly. The fix for that also looks like it should be pretty simple.

@rfcbot rfcbot added final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Jun 6, 2023
@rfcbot
Copy link
Collaborator

rfcbot commented Jun 6, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period FCP complete and removed final-comment-period FCP — a period for last comments before action is taken labels Jun 16, 2023
@rfcbot
Copy link
Collaborator

rfcbot commented Jun 16, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-doctests Area: rustdoc --test C-tracking-issue Category: A tracking issue for something unstable. disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns. T-cargo Team: Cargo to-announce
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants