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:

• Does supporting the rustc wrappers make sense?
  • yes, cargo-miri for example needs a "hack" to workaround the issue
• What environment variable(s) should be read for the rustc wrapper? Should rustdoc use the same names as cargo?
  • None, since rustdoc takes arguments
• What name should be used for a rustdoc CLI option?
  • --test-builder-wrapper
• 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
  • Added doc to src/doc/rustdoc/src/command-line-arguments.md per @GuillaumeGomez

[rustbot]

r? @jsha

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

[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 😉

[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 😉 .

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/.

🤦 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?

[RalfJung]

I filed a bug tracking the cargo side of this at rust-lang/cargo#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 d02e66d

[GuillaumeGomez]

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

[GuillaumeGomez]

@bors r+ rollup

[bors]

📌 Commit d02e66d has been approved by GuillaumeGomez

It is now in the queue for this repository.