rustdoc: add `--test-builder-wrapper` arg to support wrappers such as RUSTC_WRAPPER when building doctests

[tmfink]

Currently, rustdoc builds test crates with rustc directly instead of using RUSTC_WRAPPER (if any is set).

This causes build issues in build systems that use cargo but tweak linking flags by setting the RUSTC_WRAPPER environment variable.

This change is not meant to be final--it's only a minimal proof of concept. Please advise on the best way to proceed.

Open questions:

• [x] Does supporting the rustc wrappers make sense?
  • yes, cargo-miri for example needs a "hack" to workaround the issue
• [X] What environment variable(s) should be read for the rustc wrapper? Should rustdoc use the same names as cargo?
  • None, since rustdoc takes arguments
• [X] What name should be used for a rustdoc CLI option?
  • --test-builder-wrapper
• [X] Should a separate workspace wrapper (like RUSTC_WORKSPACE_WRAPPER) be supported?
  • --test-builder-wrapper can be passed multiple times to get multiple wrappers passed
• [ ] How/where should this be documented? It's not obvious to all users that cargo doc actually causes rustdoc to compile tests with rust

[rustbot]

r? @jsha

(rustbot has picked a reviewer for you, use r? to override)

[rust-log-analyzer]

The job mingw-check-tidy failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v3' (SHA:c85c95e3d7251135ab7dc9ce3241c5835cc595a9)
Download action repository 'actions/upload-artifact@v3' (SHA:0b7f8abb1508181956e8e162db84b466c27e18ce)
Complete job name: PR - mingw-check-tidy
git config --global core.autocrlf false
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
---
GITHUB_ACTION=__run_7
GITHUB_ACTIONS=true
GITHUB_ACTION_REF=
GITHUB_ACTION_REPOSITORY=
GITHUB_ACTOR=tmfink
GITHUB_API_URL=https://api.github.com
GITHUB_BASE_REF=master
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_71538451-5298-47ee-8aab-a7c0ae5c3628
GITHUB_EVENT_NAME=pull_request
---
GITHUB_SERVER_URL=https://github.com
GITHUB_SHA=6e718088266f06e7f9100aec6814677c14f6ed3c
GITHUB_STATE=/home/runner/work/_temp/_runner_file_commands/save_state_71538451-5298-47ee-8aab-a7c0ae5c3628
GITHUB_STEP_SUMMARY=/home/runner/work/_temp/_runner_file_commands/step_summary_71538451-5298-47ee-8aab-a7c0ae5c3628
GITHUB_TRIGGERING_ACTOR=tmfink
GITHUB_WORKFLOW_REF=rust-lang/rust/.github/workflows/ci.yml@refs/pull/114651/merge
GITHUB_WORKFLOW_SHA=6e718088266f06e7f9100aec6814677c14f6ed3c
GITHUB_WORKSPACE=/home/runner/work/rust/rust
GOROOT_1_18_X64=/opt/hostedtoolcache/go/1.18.10/x64
---
#11 3.715 Building wheels for collected packages: reuse
#11 3.716   Building wheel for reuse (pyproject.toml): started
#11 4.025   Building wheel for reuse (pyproject.toml): finished with status 'done'
#11 4.026   Created wheel for reuse: filename=reuse-1.1.0-cp310-cp310-manylinux_2_35_x86_64.whl size=180116 sha256=351235b2326fb4db7a18e257e13ce7896c5f77339521e2c2612e71e154800a19
#11 4.026   Stored in directory: /tmp/pip-ephem-wheel-cache-i0c_73re/wheels/c2/3c/b9/1120c2ab4bd82694f7e6f0537dc5b9a085c13e2c69a8d0c76d
#11 4.028 Installing collected packages: boolean-py, binaryornot, setuptools, reuse, python-debian, markupsafe, license-expression, jinja2, chardet
#11 4.053   Attempting uninstall: setuptools
#11 4.054     Found existing installation: setuptools 59.6.0
#11 4.055     Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
---
##[endgroup]
Built container sha256:cb2677fcd591084bab6e134ab7d14117326d1f9ded6da1d807b050f62c40ec2f
Uploading finished image to https://ci-caches.rust-lang.org/docker/a94f38f4e3d1f273ae476fab43c648e9503bc14021c4d4cea86fe04214617f37a9707c59d7db02b9e252db7d352cc71b37a361a5c724754d20ed2e13d846b3d0

<botocore.awsrequest.AWSRequest object at 0x7f7865b05d90>
gzip: stdout: Broken pipe
xargs: docker: terminated by signal 13
[CI_JOB_NAME=mingw-check-tidy]
[CI_JOB_NAME=mingw-check-tidy]
---
##[endgroup]
fmt check
tidy check
tidy: Skipping binary file check, read-only filesystem
##[error]tidy error: /checkout/src/librustdoc/config.rs:60: Instead of XXX use FIXME
##[error]tidy error: /checkout/src/librustdoc/doctest.rs:348: Instead of XXX use FIXME
some tidy checks failed
  local time: Wed Aug  9 08:08:59 UTC 2023
  network time: Wed, 09 Aug 2023 08:08:59 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

[RalfJung]

To work around this when cargo miri test runs doctests, cargo-miri sets the RUSTDOC variable to itself, and then adjusts the --test-builder and --runtool flags. (--runtool is needed since rustdoc also ignores cargo's target.runner configuration.)

Would be very nice if those hacks were not needed any more. :)

[RalfJung]

What I wonder is... RUSTC_WRAPPER is applied by cargo, not rustc. So shouldn't it be cargo's job to also apply it when invoking rustdoc? cargo could supply appropriate values for --test-builder and --runtool.

[tmfink]

To work around this when cargo miri test runs doctests, cargo-miri sets the RUSTDOC variable to itself, and then adjusts the --test-builder and --runtool flags. (--runtool is needed since rustdoc also ignores cargo's target.runner configuration.)

Would be very nice if those hacks were not needed any more. :)

That's a clever hack, but I don't want to implement it myself :wink:

[tmfink]

What I wonder is... RUSTC_WRAPPER is applied by cargo, not rustc. So shouldn't it be cargo's job to also apply it when invoking rustdoc? cargo could supply appropriate values for --test-builder and --runtool.

Yes, cargo actually applies RUSTC_WRAPPER. Of course, we need to add options to rustdoc that cargo can run.

In my next iteration, I'll add the equivalent rustdoc options for:

• rustc-wrapper
• runner

(Edited to fix mistakenly typing rustc instead of cargo)

[RalfJung]

Yes, rustc actually applies RUSTC_WRAPPER.

No it doesn't. If you grep the compiler sources for RUSTC_WRAPPER you won't find any hit.

As I said, it's cargo applying RUSTC_WRAPPER.

I'll add the equivalent rustdoc options for:

rustdoc already has the necessary options: --test-builder and --runtool. Cargo just needs to use them.

I think this PR is on the wrong repo, it should be in https://github.com/rust-lang/cargo/.

[tmfink]

Yes, rustc actually applies RUSTC_WRAPPER.

No it doesn't. If you grep the compiler sources for RUSTC_WRAPPER you won't find any hit.

I accidentally typed rustc instead of cargo--that's why my comment was not self consistent. I edited my comment comment--be careful when writing responses late at night :wink: .

I'll add the equivalent rustdoc options for:

rustdoc already has the necessary options: --test-builder and --runtool. Cargo just needs to use them.

I think this PR is on the wrong repo, it should be in https://github.com/rust-lang/cargo/.

:facepalm: I missed this point--it should be easier to just edit cargo.

[tmfink]

Actually, after some more investigation, I don't think a cargo change alone is enough (without some very hacky changes) since the expectations of cargo's RUSTC_WRAPPER and rustdoc's --test-builder are different.

For RUSTC_WRAPPER:

The first argument passed to the wrapper is the path to the actual executable to use

But rustdoc treats the --test-builder option as if it were rustc itself--the actual path to rustc is not passed as the first argument. I think it would be desirable for existing rustc wrappers to "just work"^TM without modification, so we can't simply use the existing --test-builder option.

I can think of a few ways to resolve the issue:

1. Ship an additional program (such as rustdoc-rustc-wrapper-binary ) that understands how find RUSTC_WRAPPER. This binary could be passed to rustdoc's --test-builder. In turn, rustdoc-rustc-wrapper-binary would call RUSTC_WRAPPER with the path to rustc and other arguments.
   • Pretty hacky
   • It would be a big pain to ship an additional binary to support a "feature" that we should have already supported.
2. Equivalently to the previous option, another existing toolchain program (e.g. cargo) could be taught to act like a rustdoc-style wrapper around the user's RUSTC_WRAPPER. It sounds like this is similar approach cargo-miri currently uses.
   • Pretty hacky. We would need to pass several environment variables (to indicate whether we are acting as a rustdoc-wrapper and the path to rustc).
   • This would have the benefit of not requiring a rustc change.
3. Add an additional "wrap" arguments to rustdoc. For example, we could call the new option --test-builder-wrapper which would take RUSTC_WRAPPER style wrappers. In fact, this arg could be passed multiple times to support nested wrappers (like cargo supports already with RUSTC_WRAPPER and RUSTC_WORKSPACE_WRAPPER.
   • Seems to be the "proper" solution.

Folks have thoughts on the best way to proceed?

[RalfJung]

--test-builder-wrapper makes most sense to me (and have cargo set that and/or --test-builder as controlled by env vars -- note that it is totally possible to have both set, in which case the wrapper should be passed the binary given by --test-builder), but this is up to @rust-lang/rustdoc

[Nemo157]

Another idea could be to have a --test-builder-args that get prepended before rustdoc's generated args, that'd also be helpful for my cargo-rustdoc-clippy script to be able to not have to reinvoke itself and pass args through the environment. A difficulty there is how to encode multiple args, maybe multiple separate --test-builder-arg that each take a single argument to prepend in order 🤔.

[tmfink]

Another idea could be to have a --test-builder-args that get prepended before rustdoc's generated args, that'd also be helpful for my cargo-rustdoc-clippy script to be able to not have to reinvoke itself and pass args through the environment. A difficulty there is how to encode multiple args, maybe multiple separate --test-builder-arg that each take a single argument to prepend in order 🤔.

My opinion is that since we would already support a wrapper, there's no need to support a separate rustdoc argument to pass rustc options. Anything that could be done with something like --test-builder-args could be handled by a wrapper program. Also, there could be cases where you need to modify the rustc arguments in a more complex way, such as appending arguments, modifying arguments, etc. I don't think it makes sense to add all these variants such as:

• --test-builder-args-prepend
• --test-builder-args-append

Of course, this is up to the rustdoc maintainers.

[tmfink]

I updated the PR to add the --test-builder-wrapper argument as discussed--it's ready for review.

Is there some documentation that should also be updated?

[rust-log-analyzer]

The job x86_64-gnu-llvm-15 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

GITHUB_ACTION=__run_7
GITHUB_ACTIONS=true
GITHUB_ACTION_REF=
GITHUB_ACTION_REPOSITORY=
GITHUB_ACTOR=tmfink
GITHUB_API_URL=https://api.github.com
GITHUB_BASE_REF=master
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_1f127899-237c-4e3b-81e1-0c13f4303b1e
GITHUB_EVENT_NAME=pull_request
---
GITHUB_SERVER_URL=https://github.com
GITHUB_SHA=f196b260808165611ce6c71d1355e1d0d34474ce
GITHUB_STATE=/home/runner/work/_temp/_runner_file_commands/save_state_1f127899-237c-4e3b-81e1-0c13f4303b1e
GITHUB_STEP_SUMMARY=/home/runner/work/_temp/_runner_file_commands/step_summary_1f127899-237c-4e3b-81e1-0c13f4303b1e
GITHUB_TRIGGERING_ACTOR=tmfink
GITHUB_WORKFLOW_REF=rust-lang/rust/.github/workflows/ci.yml@refs/pull/114651/merge
GITHUB_WORKFLOW_SHA=f196b260808165611ce6c71d1355e1d0d34474ce
GITHUB_WORKSPACE=/home/runner/work/rust/rust
GOROOT_1_18_X64=/opt/hostedtoolcache/go/1.18.10/x64
---
##[group]Testing stage2 compiletest suite=run-make mode=run-make (x86_64-unknown-linux-gnu)

running 341 tests
................i....i....ii............................................................  88/341
..................................i...i..iFi..i......................ii.....i........... 176/341
..............................ii.i.......i...iiiiiiii.iii.ii.i...............

failures:


---- [run-make] tests/run-make/issue-88756-default-output stdout ----

error: make failed
status: exit status: 2
command: cd "/checkout/tests/run-make/issue-88756-default-output" && AR="ar" CC="cc -ffunction-sections -fdata-sections -fPIC -m64" CXX="c++ -ffunction-sections -fdata-sections -fPIC -m64" HOST_RPATH_DIR="/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" LD_LIB_PATH_ENVVAR="LD_LIBRARY_PATH" LLVM_BIN_DIR="/usr/lib/llvm-15/bin" LLVM_COMPONENTS="aarch64 aarch64asmparser aarch64codegen aarch64desc aarch64disassembler aarch64info aarch64utils aggressiveinstcombine all all-targets amdgpu amdgpuasmparser amdgpucodegen amdgpudesc amdgpudisassembler amdgpuinfo amdgputargetmca amdgpuutils analysis arm armasmparser armcodegen armdesc armdisassembler arminfo armutils asmparser asmprinter avr avrasmparser avrcodegen avrdesc avrdisassembler avrinfo binaryformat bitreader bitstreamreader bitwriter bpf bpfasmparser bpfcodegen bpfdesc bpfdisassembler bpfinfo cfguard codegen core coroutines coverage debuginfocodeview debuginfodwarf debuginfogsym debuginfomsf debuginfopdb demangle dlltooldriver dwarflinker dwp engine executionengine extensions filecheck frontendopenacc frontendopenmp fuzzercli fuzzmutate globalisel hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo instcombine instrumentation interfacestub interpreter ipo irreader jitlink lanai lanaiasmparser lanaicodegen lanaidesc lanaidisassembler lanaiinfo libdriver lineeditor linker lto m68k m68kasmparser m68kcodegen m68kdesc m68kdisassembler m68kinfo mc mca mcdisassembler mcjit mcparser mips mipsasmparser mipscodegen mipsdesc mipsdisassembler mipsinfo mirparser msp430 msp430asmparser msp430codegen msp430desc msp430disassembler msp430info native nativecodegen nvptx nvptxcodegen nvptxdesc nvptxinfo objcarcopts objcopy object objectyaml option orcjit orcshared orctargetprocess passes perfjitevents powerpc powerpcasmparser powerpccodegen powerpcdesc powerpcdisassembler powerpcinfo profiledata remarks riscv riscvasmparser riscvcodegen riscvdesc riscvdisassembler riscvinfo runtimedyld scalaropts selectiondag sparc sparcasmparser sparccodegen sparcdesc sparcdisassembler sparcinfo support symbolize systemz systemzasmparser systemzcodegen systemzdesc systemzdisassembler systemzinfo tablegen target textapi transformutils ve veasmparser vecodegen vectorize vedesc vedisassembler veinfo webassembly webassemblyasmparser webassemblycodegen webassemblydesc webassemblydisassembler webassemblyinfo webassemblyutils windowsdriver windowsmanifest x86 x86asmparser x86codegen x86desc x86disassembler x86info x86targetmca xcore xcorecodegen xcoredesc xcoredisassembler xcoreinfo xray" LLVM_FILECHECK="/usr/lib/llvm-15/bin/FileCheck" NODE="/usr/bin/node" PYTHON="/usr/bin/python3" RUSTC="/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" RUSTDOC="/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustdoc" RUST_BUILD_STAGE="stage2-x86_64-unknown-linux-gnu" RUST_DEMANGLER="/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools-bin/rust-demangler" S="/checkout" TARGET="x86_64-unknown-linux-gnu" TARGET_RPATH_DIR="/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" TMPDIR="/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/issue-88756-default-output/issue-88756-default-output" "make"
--- stdout -------------------------------
LD_LIBRARY_PATH="/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/issue-88756-default-output/issue-88756-default-output:/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib:/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu/release/deps:/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/lib" '/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustdoc' 2>&1 | sed -E 's@/nightly/|/beta/|/stable/|/1\.[0-9]+\.[0-9]+/@/$CHANNEL/@g' | diff - output-default.stdout
<         --test-builder-wrapper WRAPPER
Build completed unsuccessfully in 0:25:45
<                         The wrapper program for running rustc
------------------------------------------

[RalfJung]

I filed a bug tracking the cargo side of this at https://github.com/rust-lang/cargo/issues/12532.

[GuillaumeGomez]

Hi, sorry for the delay! The rustdoc team just talked about your PR and approved it. Just one thing missing: documentation about this new command option. You can add it in src/doc/rustdoc/src/command-line-arguments.md. Once done, I'll approve it.

[tmfink]

@GuillaumeGomez I add docs for --test-builder/--test-builder-wrapper with d02e66ddf0f1b0d436b3a8374479ec8efbe4b1db

[GuillaumeGomez]

Thanks! I'll approve it when the CI is green.

[GuillaumeGomez]

@bors r+ rollup

[bors]

:pushpin: Commit d02e66ddf0f1b0d436b3a8374479ec8efbe4b1db has been approved by GuillaumeGomez

It is now in the queue for this repository.