Add `--doctest-compilation-args` option to add compilation flags to doctest compilation #128780

GuillaumeGomez · 2024-08-07T12:57:33Z

Fixes #67533.
Tracking issue: #134172

It's been something I meant to take a look at for a long time and actually completely forgot... The idea is to allow to give more control over how doctests are compiled to users. To do so, this PR adds a new --doctest-compilation-args option which provides extra compilation flags.

r? @notriddle

GuillaumeGomez · 2024-08-07T13:58:09Z

Forgot to normalize doctests output in the test...

GuillaumeGomez · 2024-08-07T15:04:17Z

And this time I normalized the time... Hopefully I didn't forget anything again.

tests/rustdoc-ui/doctest/rustflags.rs

notriddle · 2024-08-07T15:38:34Z

src/librustdoc/doctest.rs

+    // the doctest.
+    if let Ok(rustflags) = std::env::var("RUSTFLAGS") {
+        // Very simple parsing implementation. Might be a good idea to correctly handle strings.
+        for flag in rustflags.split(' ').map(|flag| flag.trim()).filter(|flag| !flag.is_empty()) {


Suggested change

for flag in rustflags.split(' ').map(|flag| flag.trim()).filter(|flag| !flag.is_empty()) {

for flag in rustflags.split_whitespace().map(|flag| flag.trim()).filter(|flag| !flag.is_empty()) {

I went hunting through cargo's source code for info on this, and I hope someone can correct me on this, but it looks like this is the function that does the splitting:

https://github.com/rust-lang/cargo/blob/495d02cb4731e31d54c0127ef8f0b7b4b219a3a3/src/cargo/util/context/mod.rs#L946-L983

I saw it but I'm not a big fan of this approach. Parsing a shell command is not too complicated. Just not sure it's worth it here.

I'll update with your suggestion though.

Yeah, I understand. I brought it up because I figured, if both tools are going to consult the same env variable, then they should both parse it the same way.

The TOML wrapping is behind an unstable flag anyway.

Might be worth doing a small rewrite of their code. Putting that in my todo list.

(compiletest has the same problems of splitting rustflags and env vars and such naively on whitespace)

This definitely must parse the variable the same way cargo does, everything else is going to cause nasty surprises.

Here is what Miri does, which we took from cargo originally:

https://github.com/rust-lang/miri/blob/ec037c63430e4f563992d89047c725ce8ce1ed57/cargo-miri/src/util.rs#L121-L124

That does seem to be equal to what this PR does.

notriddle · 2024-08-07T15:53:19Z

@rfcbot fcp merge

The env vars are an API, so covered by stability promise.

rfcbot · 2024-08-07T15:53:23Z

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

src/librustdoc/doctest.rs

Nemo157 · 2024-08-07T16:55:28Z

This won't take into account the dozen other ways to specify rustc flags to cargo, I don't think rustdoc should be trying to replicate cargo's logic but instead have the flags passed down to it explicitly.

GuillaumeGomez · 2024-08-07T17:45:03Z

This won't take into account the dozen other ways to specify rustc flags to cargo, I don't think rustdoc should be trying to replicate cargo's logic but instead have the flags passed down to it explicitly.

I wasn't sure adding a new command line flag was a good idea, but thinking about it some more, I think you're right.

What do you think about --compile-doctest-args?

GuillaumeGomez · 2024-08-19T13:39:42Z

I replaced the usage of RUSTFLAGS by a new option called --doctest-compilation-args as per @Nemo157's suggestion. I think it's much better this way as it won't accidentally overlap. I updated the first comment as well.

aDotInTheVoid · 2024-08-19T14:27:45Z

src/librustdoc/doctest.rs

+            .filter(|flag| !flag.is_empty())
+        {
+            // Very simple parsing implementation. Might be a good idea to correctly handle strings.
+            content.push(flag.to_string());


This has been brought up a load of times, but I'm not sure this is the right thing to do here? Is there any prior art for passing flags down from a flag (and not an env-var).

(The good news is that this isn't insta-stable, so we can change the edge-behaviour later before stabilization)

There's two correct options I know of:

Pass one flag at a time --doctest-compilation-arg -Dwarnings --doctest-compilation-arg -Adead-code

Pass the flags as a single string in a proper encoding, such as toml

Pass one flag at a time --doctest-compilation-arg -Dwarnings --doctest-compilation-arg -Adead-code.

You can currently do it with the option. :)

Pass the flags as a single string in a proper encoding, such as toml

That could work too, but I'm not sure if it's something common enough to justify it here. Parsing shell quotes would be much simpler.

This has been brought up a load of times, but I'm not sure this is the right thing to do here? Is there any prior art for passing flags down from a flag (and not an env-var).

(The good news is that this isn't insta-stable, so we can change the edge-behaviour later before stabilization)

We have --test-args in rustdoc which allows it.

You can currently do it with the option. :)

No, the current implementation would mishandle trying to pass a space containing arg.

Yes, I meant passing the same option flag multiple times.

Code has evolved quite a lot since then. It's now how you would expect a shell environment to parse the command.

rfcbot · 2024-12-19T20:57:29Z

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.

RalfJung · 2024-12-20T11:43:04Z

src/doc/rustdoc/src/unstable-features.md

+
+The parsing of arguments works as follows: if it encounters a `"` or a `'`, it will continue
+until it finds the character unescaped (without a prepending `\`). If not inside a string, a
+whitespace character will also split arguments. Example:


That does not match shell -- in shell, you cannot do any escaping inside '.

Interesting! Gonna change this behaviour then.

I'm not a fan of us having a re-implementation of shell word splitting in the repo. We'll inevitably get it wrong in subtle ways. I think we should, if at all possible, avoid having to do word splitting.

If some word splitting is needed, then given that RUSTFLAGS can get by without any quote handling, it might be better to just remain consistent with that. Or alternatively, if the flag is only meant to be set by machines, we could use the format used by CARGO_ENCODED_RUSTFLAGS.

Actually I'm gonna keep the current behaviour. It's not exactly like a shell would do, but it's fully described in the book and in any case it's unstable until further notice, so if users are unhappy with this format, nothing prevents us from changing it in the future.

If you land this on nightly with completely non-standard behavior (as seems to be the case right now), there's a chance that it's going to be hard to change later. We don't have to make the final call now, but the implementation should at least do something that plausibly could be the final version, and I would say right now that is not the case.

Also, please make sure this is explicitly listed in the tracking issue so that this is discussed before stabilization. Where is that tracking issue, anyway?

It's #134172. Adding it into the first comment as well.

… doctest compilation

…s work

GuillaumeGomez · 2024-12-20T23:12:24Z

Seems like I updated all tests and added missing docs so ready to go.

@bors r=rustdoc

bors · 2024-12-20T23:12:27Z

📌 Commit 2d914be has been approved by rustdoc

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#123604 (Abstract `ProcThreadAttributeList` into its own struct) - rust-lang#128780 (Add `--doctest-compilation-args` option to add compilation flags to doctest compilation) - rust-lang#133782 (Precedence improvements: closures and jumps) - rust-lang#134509 (Arbitrary self types v2: niche deshadowing test) - rust-lang#134524 (Arbitrary self types v2: no deshadow pre feature.) - rust-lang#134539 (Restrict `#[non_exaustive]` on structs with default field values) - rust-lang#134586 (Also lint on option of function pointer comparisons) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#128780 - GuillaumeGomez:rustflags-doctests, r=rustdoc Add `--doctest-compilation-args` option to add compilation flags to doctest compilation Fixes rust-lang#67533. Tracking issue: rust-lang#134172 It's been something I meant to take a look at for a long time and actually completely forgot... The idea is to allow to give more control over how doctests are compiled to users. To do so, this PR adds a new `--doctest-compilation-args` option which provides extra compilation flags. r? `@notriddle`

rustbot assigned notriddle Aug 7, 2024

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. labels Aug 7, 2024

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the rustflags-doctests branch from 7d5d19d to 50f8136 Compare August 7, 2024 13:57

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the rustflags-doctests branch from 50f8136 to d906824 Compare August 7, 2024 15:03

notriddle reviewed Aug 7, 2024

View reviewed changes

tests/rustdoc-ui/doctest/rustflags.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the rustflags-doctests branch from d906824 to 4a7337c Compare August 7, 2024 15:15

notriddle reviewed Aug 7, 2024

View reviewed changes

GuillaumeGomez force-pushed the rustflags-doctests branch from 4a7337c to 9d90942 Compare August 7, 2024 15:49

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 Aug 7, 2024

notriddle approved these changes Aug 7, 2024

View reviewed changes

aDotInTheVoid added the relnotes Marks issues that should be documented in the release notes of the next release. label Aug 7, 2024

fmease reviewed Aug 7, 2024

View reviewed changes

src/librustdoc/doctest.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the rustflags-doctests branch from 9d90942 to b8ee921 Compare August 19, 2024 13:37

GuillaumeGomez changed the title ~~Use RUSTFLAGS environment variable when compiling doctests~~ Add --doctest-compilation-args option to add compilation flags to doctest compilation Aug 19, 2024

GuillaumeGomez force-pushed the rustflags-doctests branch from b8ee921 to babc9ef Compare August 19, 2024 13:40

aDotInTheVoid reviewed Aug 19, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Aug 19, 2024

This comment has been minimized.

Sign in to view

rfcbot added the to-announce Announce this issue on triage meeting label Dec 19, 2024

rustbot mentioned this pull request Dec 19, 2024

Tracking issue for release notes of #128780: Add --doctest-compilation-args option to add compilation flags to doctest compilation #134535

Closed

3 tasks

GuillaumeGomez force-pushed the rustflags-doctests branch 2 times, most recently from a5162c6 to 16b85df Compare December 20, 2024 11:31

RalfJung reviewed Dec 20, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

GuillaumeGomez added 3 commits December 20, 2024 15:17

Add --doctest-compilation-args option to allow passing arguments to…

05731af

… doctest compilation

Add regression test for --doctest-compilation-args

2bd8690

Add explanations about --doctest-compilation-args in rustdoc book

d3c970a

GuillaumeGomez force-pushed the rustflags-doctests branch from 82417a4 to a4411e0 Compare December 20, 2024 14:40

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the rustflags-doctests branch from a4411e0 to de4890b Compare December 20, 2024 15:39

This comment has been minimized.

Sign in to view

GuillaumeGomez and others added 5 commits December 20, 2024 22:35

Update run-make/rustdoc-default-output test

24fafe7

Split arguments from --doctest-compilation-args like a shell would

cbb3df4

Add explanations on how arguments are split

55653a5

Move test into the tests.rs file

bc03e40

Add test to ensure passing --doctest_compilation_args multiple time…

2d914be

…s work

GuillaumeGomez force-pushed the rustflags-doctests branch from de4890b to 2d914be Compare December 20, 2024 21:35

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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 20, 2024

matthiaskrgr mentioned this pull request Dec 21, 2024

Rollup of 7 pull requests #134590

Merged

bors merged commit 472bbb9 into rust-lang:master Dec 21, 2024
6 checks passed

rustbot added this to the 1.85.0 milestone Dec 21, 2024

GuillaumeGomez deleted the rustflags-doctests branch December 21, 2024 14:03

	for flag in rustflags.split(' ').map(\|flag\| flag.trim()).filter(\|flag\| !flag.is_empty()) {
	for flag in rustflags.split_whitespace().map(\|flag\| flag.trim()).filter(\|flag\| !flag.is_empty()) {

Add --doctest-compilation-args option to add compilation flags to doctest compilation #128780

Add --doctest-compilation-args option to add compilation flags to doctest compilation #128780

Conversation

GuillaumeGomez commented Aug 7, 2024 • edited Loading

This comment has been minimized.

GuillaumeGomez commented Aug 7, 2024

This comment has been minimized.

GuillaumeGomez commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

notriddle commented Aug 7, 2024

rfcbot commented Aug 7, 2024 • edited by jsha Loading

Nemo157 commented Aug 7, 2024 • edited Loading

GuillaumeGomez commented Aug 7, 2024

GuillaumeGomez commented Aug 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

rfcbot commented Dec 19, 2024

RalfJung Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

GuillaumeGomez commented Dec 20, 2024

bors commented Dec 20, 2024

Add `--doctest-compilation-args` option to add compilation flags to doctest compilation #128780

Add `--doctest-compilation-args` option to add compilation flags to doctest compilation #128780

GuillaumeGomez commented Aug 7, 2024 •

edited

Loading

RalfJung Dec 12, 2024 •

edited

Loading

rfcbot commented Aug 7, 2024 •

edited by jsha

Loading

Nemo157 commented Aug 7, 2024 •

edited

Loading

RalfJung Dec 20, 2024 •

edited

Loading