Skip to content

rustdoc: Deprecate option --test-args in favor of new insta-stable option --test-arg #139869

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fmease
Copy link
Member

@fmease fmease commented Apr 15, 2025

@rustbot
Copy link
Collaborator

rustbot commented Apr 15, 2025

r? @notriddle

rustbot has assigned @notriddle.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Apr 15, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 15, 2025

This PR modifies run-make tests.

cc @jieyouxu

@fmease fmease removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 15, 2025
@fmease
Copy link
Member Author

fmease commented Apr 15, 2025

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 15, 2025

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

Concerns:

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 Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 15, 2025
@fmease
Copy link
Member Author

fmease commented Apr 15, 2025

@rfcbot concern insta-stable

Should the new flag be insta-stable? If we keep the new flag unstable, we can't recommend a replacement for the newly deprecated flag. Should we just soft-deprecate the old flag first?

Potential solution: Only deprecate the flag under --edition=future (-Zunstable-options) and make the new flag unstable (requiring -Zunstable-options).

@rfcbot concern insuppressible-warning

We currently emit an insuppressible deprecation warning (IIRC it's infeasible or even impossible to make this an early buffered lint warning). However, that'll probably lead to a lot of terminal spam for Cargo users? Idk, does Cargo use this flag under the hood or do users usually pass these explicitly via env var RUSTDOCFLAGS / config opt rustdocflags?

@GuillaumeGomez
Copy link
Member

Should we tie this change to a new edition?

@fmease
Copy link
Member Author

fmease commented Apr 15, 2025

Should we tie this change to a new edition?

We could. We could deprecate this in all editions (as it's usually done for rustc/rustdoc's flags) and make --test-args a hard-error in the next edition. However, I think that's orthogonal to the actual deprecation.

@GuillaumeGomez
Copy link
Member

Then let's go through deprecation and make the removal in the next edition.

@fmease
Copy link
Member Author

fmease commented Apr 15, 2025

However, I think that's orthogonal to the actual deprecation.

Well, on a second thought, we could delay the deprecation by only rejecting --test-args if --edition=future -Zunstable-options is passed and by gating the new --test-arg flag behind --edition=future -Zunstable-options. This would be a solution to the "insta-stable" concern.

Re. --edition=future: #137606

@rust-log-analyzer

This comment has been minimized.

@Manishearth
Copy link
Member

Should the new flag be insta-stable? If we keep the new flag unstable, we can't recommend a replacement for the newly deprecated flag. Should we just soft-deprecate the old flag first?

I'm not opposed to this provided we have a nice principled way of doing these flags across the board. So if we spend some time making sure this is what we want, I think we're fine.

@ehuss
Copy link
Contributor

ehuss commented Apr 15, 2025

Just FYI, I don't think you'll be able to land this with a warning as-is since it will cause cargo's tests to fail.
(I personally would recommend not warning for at least a few releases.)

@fmease
Copy link
Member Author

fmease commented Apr 15, 2025

Yeah, I noticed that when looking at the CI failure

@fmease fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 15, 2025
@fmease fmease force-pushed the replace-test-args-opt branch from 7a91871 to 1afe243 Compare April 16, 2025 13:42
@fmease
Copy link
Member Author

fmease commented Apr 16, 2025

--test-args is now only soft-deprecated (i.e., the docs and the CLI help output mention it's deprecated but we don't emit any warnings yet). We can re-upgrade to a (hard) deprecation warning once at least Cargo has had the change to migrate to the new flag.

I'll track this in a tracking issue if this FCP goes through successfully.

@rfcbot resolve insuppressible-warning

@fmease fmease added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 16, 2025
@fmease
Copy link
Member Author

fmease commented Apr 16, 2025

For the upgrade path, the new flag needs to be (insta-) stable. We could meddle with performing the deprecation in the Future Edition only (--edition=future -Zunstable-options) but that seems unnecessarily roundabout and complex. Also: #139869 (comment).

@rfcbot resolve insta-stable

@rust-log-analyzer

This comment has been minimized.

@fmease fmease force-pushed the replace-test-args-opt branch 2 times, most recently from 4462bde to 4c3b4f4 Compare April 16, 2025 13:53
@rust-log-analyzer

This comment has been minimized.

@fmease fmease force-pushed the replace-test-args-opt branch from 4c3b4f4 to c262603 Compare April 16, 2025 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants