Skip to content

Stabilize flags for doctest cross compilation #137096

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

Merged
merged 7 commits into from
Apr 25, 2025

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Feb 15, 2025

This makes the following changes in preparation for supporting doctest cross-compiling in cargo:

  • Renames --runtool and --runtool-arg to --test-runtool and --test-runtool-arg to maintain consistency with other --test-* arguments.
  • Stabilizes the --test-runtool and --test-runtool-arg. These are needed in order to support cargo's target.runner option which specifies a runner to execute a cross-compiled doctest (for example, qemu).
  • Stabilizes the --enable-per-target-ignores flag by removing it and making it unconditionally enabled. This makes it possible to disable a doctest on a per-target basis, which I think will be helpful for rolling out this feature.

These changes were suggested in https://rust-lang.zulipchat.com/#narrow/channel/266220-t-rustdoc/topic/stabilizing.20doctest.20xcompile/near/409281127

The intent is to stabilize the doctest-xcompile feature in cargo. This will help ensure that for projects that do cross-compile testing that their doctests are also covered. Currently there is a somewhat surprising behavior that they are ignored.

Closes #64245

try-job: x86_64-msvc-1

@rustbot
Copy link
Collaborator

rustbot commented Feb 15, 2025

r? @fmease

rustbot has assigned @fmease.
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 S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels Feb 15, 2025
@fmease fmease added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Feb 15, 2025
@rust-log-analyzer

This comment has been minimized.

@ehuss ehuss force-pushed the stabilize-doctest-xcompile branch from ab9d4c8 to a676b04 Compare February 15, 2025 20:58
@rustbot rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Feb 15, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 15, 2025

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

The Miri subtree was changed

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

@ehuss ehuss force-pushed the stabilize-doctest-xcompile branch from a676b04 to 8aaa90b Compare February 15, 2025 22:01
@bors
Copy link
Collaborator

bors commented Mar 22, 2025

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

@ehuss ehuss force-pushed the stabilize-doctest-xcompile branch 2 times, most recently from a0799de to ddc6525 Compare March 22, 2025 23:04
@ehuss
Copy link
Contributor Author

ehuss commented Mar 23, 2025

@fmease @GuillaumeGomez Just checking if this can be put on the team's radar?

@GuillaumeGomez
Copy link
Member

On my review list now. :)

@GuillaumeGomez
Copy link
Member

I think they've been around for enough time. Let's start an FCP then.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 23, 2025

Team member @GuillaumeGomez 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 Mar 23, 2025
@ehuss
Copy link
Contributor Author

ehuss commented Mar 28, 2025

In playing around with this, I noticed a slight annoyance when using --edition=2024 and mergeable tests. It always prints:

WARNING: No rustdoc doctest environment variable provided so doctests will be run in the same process

I'm wondering if it would make sense to somehow silence that warning when using --test-runtool?

I'm also wondering if it is the right choice to run in the same process. I could see arguments either way. Let me know if you have any opinion on that.

@rust-log-analyzer

This comment has been minimized.

ehuss added 4 commits March 27, 2025 19:04
This renames `--runtool` and `--runtool-arg` to `--test-runtool` and
`--test-runtool-arg` to maintain consistency with other `--test-*`
arguments.
This removes the `--enable-per-target-ignores` and enables it
unconditionally.
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 24, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 24, 2025

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.

@fmease
Copy link
Member

fmease commented Apr 24, 2025

Thanks everyone! @bors r+ rollup

@bors
Copy link
Collaborator

bors commented Apr 24, 2025

📌 Commit 115341f has been approved by fmease

It is now in the queue for this repository.

@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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Apr 24, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 25, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#137096 (Stabilize flags for doctest cross compilation)
 - rust-lang#140148 (CI: use aws codebuild for job dist-arm-linux)
 - rust-lang#140187 ([AIX] Handle AIX dynamic library extensions within c-link-to-rust-dylib run-make test)
 - rust-lang#140196 (Improved diagnostics for non-primitive cast on non-primitive types (`Arc`, `Option`))
 - rust-lang#140210 (Work around cygwin issue on condvar timeout)
 - rust-lang#140213 (mention about `x.py setup` in `INSTALL.md`)
 - rust-lang#140229 (`DelimArgs` tweaks)
 - rust-lang#140248 (Fix impl block items indent)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 25, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#137096 (Stabilize flags for doctest cross compilation)
 - rust-lang#140148 (CI: use aws codebuild for job dist-arm-linux)
 - rust-lang#140187 ([AIX] Handle AIX dynamic library extensions within c-link-to-rust-dylib run-make test)
 - rust-lang#140196 (Improved diagnostics for non-primitive cast on non-primitive types (`Arc`, `Option`))
 - rust-lang#140210 (Work around cygwin issue on condvar timeout)
 - rust-lang#140213 (mention about `x.py setup` in `INSTALL.md`)
 - rust-lang#140229 (`DelimArgs` tweaks)
 - rust-lang#140248 (Fix impl block items indent)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 610ed82 into rust-lang:master Apr 25, 2025
6 checks passed
@rustbot rustbot added this to the 1.88.0 milestone Apr 25, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 25, 2025
Rollup merge of rust-lang#137096 - ehuss:stabilize-doctest-xcompile, r=fmease

Stabilize flags for doctest cross compilation

This makes the following changes in preparation for supporting doctest cross-compiling in cargo:

- Renames `--runtool` and `--runtool-arg` to `--test-runtool` and `--test-runtool-arg` to maintain consistency with other `--test-*` arguments.
- Stabilizes the `--test-runtool` and `--test-runtool-arg`. These are needed in order to support cargo's `target.runner` option which specifies a runner to execute a cross-compiled doctest (for example, qemu).
- Stabilizes the `--enable-per-target-ignores` flag by removing it and making it unconditionally enabled. This makes it possible to disable a doctest on a per-target basis, which I think will be helpful for rolling out this feature.

These changes were suggested in https://rust-lang.zulipchat.com/#narrow/channel/266220-t-rustdoc/topic/stabilizing.20doctest.20xcompile/near/409281127

The intent is to stabilize the doctest-xcompile feature in cargo. This will help ensure that for projects that do cross-compile testing that their doctests are also covered. Currently there is a somewhat surprising behavior that they are ignored.

Closes rust-lang#64245

try-job: x86_64-msvc-1
ehuss added a commit to ehuss/cargo that referenced this pull request Apr 25, 2025
This updates the flags used for doctest xcompile to match the upstream
changes in rust-lang/rust#137096 which renamed
the flag.
ehuss added a commit to ehuss/cargo that referenced this pull request Apr 25, 2025
This updates the flags used for doctest xcompile to match the upstream
changes in rust-lang/rust#137096 which renamed
and stabilized the flags.
ehuss added a commit to ehuss/cargo that referenced this pull request Apr 26, 2025
This updates the flags used for doctest xcompile to match the upstream
changes in rust-lang/rust#137096 which renamed
and stabilized the flags.
github-merge-queue bot pushed a commit to rust-lang/cargo that referenced this pull request Apr 26, 2025
This updates the flags used for doctest xcompile to match the upstream
changes in rust-lang/rust#137096 which renamed
and stabilized the flags.

This cannot be merged until after nightly is published tonight.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 9, 2025
…ing, r=GuillaumeGomez

rustdoc: Replace unstable flag `--doctest-compilation-args` with a simpler one: `--doctest-build-arg`

Tracking issue: rust-lang#134172.
Context: rust-lang#137096 (comment)

Yeets the ad hoc shell-like lexer for 'nested' program arguments.
No FCP necessary since the flag is unstable.

I've chosen to replace `compilation` with `build` because it's shorter (you now need to pass it multiple times in order to pass many arguments to the doctest compiler, so it matters a bit) and since I prefer it esthetically.

**Issue**: Even though we don't process the argument passed to `--doctest-build-arg`, we end up passing it via an argument file (`rustc `@argfile`)` which delimits arguments by line break (LF or CRLF, [via](https://doc.rust-lang.org/rustc/command-line-arguments.html#path-load-command-line-flags-from-a-path)) meaning ultimately the arguments still get split which is unfortunate. Still, I think this change is an improvement over the status quo.

I'll update the tracking issue if/once this PR merges. I'll also add the (CR)LF issue to 'unresolved question'.

r? GuillaumeGomez
r? notriddle
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request May 9, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#137096 (Stabilize flags for doctest cross compilation)
 - rust-lang#140148 (CI: use aws codebuild for job dist-arm-linux)
 - rust-lang#140187 ([AIX] Handle AIX dynamic library extensions within c-link-to-rust-dylib run-make test)
 - rust-lang#140196 (Improved diagnostics for non-primitive cast on non-primitive types (`Arc`, `Option`))
 - rust-lang#140210 (Work around cygwin issue on condvar timeout)
 - rust-lang#140213 (mention about `x.py setup` in `INSTALL.md`)
 - rust-lang#140229 (`DelimArgs` tweaks)
 - rust-lang#140248 (Fix impl block items indent)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 9, 2025
…ing, r=GuillaumeGomez

rustdoc: Replace unstable flag `--doctest-compilation-args` with a simpler one: `--doctest-build-arg`

Tracking issue: rust-lang#134172.
Context: rust-lang#137096 (comment)

Yeets the ad hoc shell-like lexer for 'nested' program arguments.
No FCP necessary since the flag is unstable.

I've chosen to replace `compilation` with `build` because it's shorter (you now need to pass it multiple times in order to pass many arguments to the doctest compiler, so it matters a bit) and since I prefer it esthetically.

**Issue**: Even though we don't process the argument passed to `--doctest-build-arg`, we end up passing it via an argument file (`rustc `@argfile`)` which delimits arguments by line break (LF or CRLF, [via](https://doc.rust-lang.org/rustc/command-line-arguments.html#path-load-command-line-flags-from-a-path)) meaning ultimately the arguments still get split which is unfortunate. Still, I think this change is an improvement over the status quo.

I'll update the tracking issue if/once this PR merges. I'll also add the (CR)LF issue to 'unresolved question'.

r? GuillaumeGomez
r? notriddle
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 9, 2025
…ing, r=GuillaumeGomez

rustdoc: Replace unstable flag `--doctest-compilation-args` with a simpler one: `--doctest-build-arg`

Tracking issue: rust-lang#134172.
Context: rust-lang#137096 (comment)

Yeets the ad hoc shell-like lexer for 'nested' program arguments.
No FCP necessary since the flag is unstable.

I've chosen to replace `compilation` with `build` because it's shorter (you now need to pass it multiple times in order to pass many arguments to the doctest compiler, so it matters a bit) and since I prefer it esthetically.

**Issue**: Even though we don't process the argument passed to `--doctest-build-arg`, we end up passing it via an argument file (`rustc ``@argfile`)`` which delimits arguments by line break (LF or CRLF, [via](https://doc.rust-lang.org/rustc/command-line-arguments.html#path-load-command-line-flags-from-a-path)) meaning ultimately the arguments still get split which is unfortunate. Still, I think this change is an improvement over the status quo.

I'll update the tracking issue if/once this PR merges. I'll also add the (CR)LF issue to 'unresolved question'.

r? GuillaumeGomez
r? notriddle
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 10, 2025
Rollup merge of rust-lang#139863 - fmease:simp-doctest-build-arg-passing, r=GuillaumeGomez

rustdoc: Replace unstable flag `--doctest-compilation-args` with a simpler one: `--doctest-build-arg`

Tracking issue: rust-lang#134172.
Context: rust-lang#137096 (comment)

Yeets the ad hoc shell-like lexer for 'nested' program arguments.
No FCP necessary since the flag is unstable.

I've chosen to replace `compilation` with `build` because it's shorter (you now need to pass it multiple times in order to pass many arguments to the doctest compiler, so it matters a bit) and since I prefer it esthetically.

**Issue**: Even though we don't process the argument passed to `--doctest-build-arg`, we end up passing it via an argument file (`rustc `@argfile`)` which delimits arguments by line break (LF or CRLF, [via](https://doc.rust-lang.org/rustc/command-line-arguments.html#path-load-command-line-flags-from-a-path)) meaning ultimately the arguments still get split which is unfortunate. Still, I think this change is an improvement over the status quo.

I'll update the tracking issue if/once this PR merges. I'll also add the (CR)LF issue to 'unresolved question'.

r? GuillaumeGomez
r? notriddle
github-merge-queue bot pushed a commit to rust-lang/cargo that referenced this pull request May 14, 2025
This stabilizes the doctest-xcompile feature by unconditionally enabling
it.

Closes #7040
Closes #12118

## What is being stabilized?

This changes it so that cargo will run doctests when using the
`--target` flag for a target that is not the host. Previously, cargo
would ignore doctests (and show a note if passing `--verbose`).

A wrapper for running the doctest can be specified with the
[`target.*.runner`](https://doc.rust-lang.org/cargo/reference/config.html#targettriplerunner)
configuration option (which is powered by the `--test-runtool` rustdoc
flag). This would typically be something like qemu to run under
emulation. It is my understanding that this should work just like
running other kinds of tests.

Additionally, the
[`target.*.linker`](https://doc.rust-lang.org/cargo/reference/config.html#targettriplelinker)
config option is honored for using a custom linker.

Already stabilized in rustdoc is the ability to [ignore tests
per-target](https://doc.rust-lang.org/nightly/rustdoc/write-documentation/documentation-tests.html#ignoring-targets).

## Motivation

The lack of doctest cross-compile support has always been simply due to
the lack of functionality in rustdoc to support this. Rustdoc gained the
ability to cross-compile doctests some time ago, but there were
additional flags like the test runner that were not stabilized until
just recently.

This is intended to ensure that projects have full test coverage even
when doing cross-compilation. It can be
[surprising](#12118) to some
that this was not happening, particularly since cargo is silent about
it.

## Risks

The cargo team had several conversations about how to roll out this
feature. Ultimately we decided to enable it unconditionally with the
understanding that most projects will probably want to have their
doctests covered, and that any breakage will be a local concern that can
be resolved by either fixing the test or ignoring the target.

Tests in rust-lang/rust run into this issue, [particularly on
android](rust-lang/rust#119147 (comment)),
and those will need to be fixed before this reaches beta. This is
something I am looking into.

Some cross-compiling scenarios may need codegen flags that are not
supported. It's not clear how common this will be, or if ignoring will
be a solution, or how difficult it would be to update rustdoc and cargo
to support these. Additionally, the split between RUSTFLAGS and
RUSTDOCFLAGS can be cumbersome.

## Implementation history

- rust-lang/rust#60387 -- Support added to
rustdoc to support the `--target` flag and runtool and
per-target-ignores.
- #6892 -- Initial support in
cargo.
- #7391 -- Added unstable
documentation.
- #8094 -- Fix target for doc
test cross compilation
- #8358 -- Fixed regression with
`--target=HOST` not working on stable.
- #10132 -- Added note about
doctests not running (under `--verbose`).
- rust-lang/rust#112751 -- Fixed
`--test-run-directory` interaction with `--test-runtool`.
- rust-lang/rust#137096 -- Stabilization (and
rename) of the rustdoc `--test-runtool` and `--test-runtool-arg` CLI
args, and drops `--enable-per-target-ignores` unconditionally enabling
it.

## Test coverage

Cargo tests:
-
[artifact_dep::cross_doctests_works_with_artifacts](https://github.com/ehuss/cargo/blob/56c08f84e28d3653ae0c842f331a226738108188/tests/testsuite/artifact_dep.rs#L1248-L1326)
-- Checks that doctest has access to the artifact dependencies.
-
[build_script::duplicate_script_with_extra_env](https://github.com/ehuss/cargo/blob/56c08f84e28d3653ae0c842f331a226738108188/tests/testsuite/build_script.rs#L5514-L5614)
-- Checks that build-script env and cfg values are correctly handled on
host versus target when cross running doctests.
-
[cross_compile::cross_tests](https://github.com/ehuss/cargo/blob/56c08f84e28d3653ae0c842f331a226738108188/tests/testsuite/cross_compile.rs#L416-L502)
-- Basic test that cross-compiled tests work.
-
[cross_compile::doctest_xcompile_linker](https://github.com/ehuss/cargo/blob/56c08f84e28d3653ae0c842f331a226738108188/tests/testsuite/cross_compile.rs#L1139-L1182)
-- Checks that the linker config argument works.
-
[custom_target::custom_target_minimal](https://github.com/ehuss/cargo/blob/56c08f84e28d3653ae0c842f331a226738108188/tests/testsuite/custom_target.rs#L39-L71)
-- Checks that `.json` targets work with rustdoc cross tests.
-
[test::cargo_test_doctest_xcompile_ignores](https://github.com/ehuss/cargo/blob/56c08f84e28d3653ae0c842f331a226738108188/tests/testsuite/test.rs#L4743-L4777)
-- Checks the `ignore-*` syntax works.
-
[test::cargo_test_doctest_xcompile_runner](https://github.com/ehuss/cargo/blob/2603268cda3e32565ac27ee642f2b755fa590bac/tests/testsuite/test.rs#L4783-L4863)
-- Checks runner with cross doctests.
-
[test::cargo_test_doctest_xcompile_no_runner](https://github.com/ehuss/cargo/blob/2603268cda3e32565ac27ee642f2b755fa590bac/tests/testsuite/test.rs#L4869-L4907)
-- Checks cross doctests without a runner.

Rustdoc tests:
-
[run-make/doctest-runtool](https://github.com/rust-lang/rust/tree/25cdf1f67463c9365d8d83778c933ec7480e940b/tests/run-make/doctests-runtool)
-- Tests behavior of `--test-run-directory` with relative paths of the
runner.
-
[rustdoc/doctest/doctest-runtool](https://github.com/rust-lang/rust/blob/25cdf1f67463c9365d8d83778c933ec7480e940b/tests/rustdoc/doctest/doctest-runtool.rs)
-- Tests for `--test-runtool` and `--test-runtool-arg`.

## Future concerns

There have been some discussions
(rust-lang/testing-devex-team#5) about
changing how doctests are driven. My understanding is that stabilizing
this should not affect those plans, since if cargo becomes the driver,
it will simply need to build things with `--target` and use the
appropriate runner.

## Change notes

This PR changed tests a little:
- artifact_dep::no_cross_doctests_works_with_artifacts was changed now
that doctests actually work.
- cross_compile::cross_tests was changed to properly check doctests.
- cross_compile::no_cross_doctests dropped since it is no longer
relevant.
- standard_lib::doctest didn't need `-Zdoctest-xcompile` since
`-Zbuild-std` no longer uses a target.
- test::cargo_test_doctest_xcompile was removed since it is a duplicate
of cross_compile::cross_tests

I think this should probably wait until the next release cutoff, moving
this to 1.89 (will update the PR accordingly if that happens).
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. finished-final-comment-period The final comment period is finished for this PR / Issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. to-announce Announce this issue on triage meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking Issue for Cross Compiling Doctests
8 participants