fix: Pass rustdoc flags to final CCI merge step

[camelid]

View all comments

Cargo wasn't passing the --resource-suffix to the final merge step,
breaking the filename for the search index in rust-lang/rust (and
probably many other use cases). In fact, Cargo wasn't passing the
RUSTDOCFLAGS at all. This commit changes Cargo to pass the same flags to
the final merge invocation as to each crate's rustdoc invocation. This
conflates flags for two somewhat different things, but we already share
the same flags across all crates so I think sharing them with the merge
makes sense.

LLM disclaimer: This change was mostly generated using an LLM, though the commit messages are handwritten.

r? @weihanglo

[camelid]

I need to figure out how to test this locally in bootstrap to make sure it fixes the issue in practice.

[Kobzol]

You can build Cargo and then tell bootstrap to use that local Cargo using build.cargo = "<path>" in config.toml.

[weihanglo]

BTW, this is listed as an unresolved question in #16306

• Should all RUSTDOCFLAGS apply to rustdoc --merge=finalize call?
  • Some probably only need to pass to the --merge=finalize invocation, like --extend-css?

Could any rustdoc expert help walk through rustdoc options and figure out whether it is safe to unconditionally pass RUSTDOCFLAGS to both? Is there any flag you only want to pass to individual invocations or merge invocations?

View changes since the review

[notriddle]

It seems like the simplest option is to pass all of the options to both, while having rustdoc ignore the options that are inapplicable. Since we want to leave the option open to defer all of the HTML generation to the finalize step (we don't do that right now), we need to pass all the HTML-related options to the finalize step.

[weihanglo]

This sounds pretty convenient to Cargo.

However, from rustdoc's point of view, what if some of the options we mark as inapplicable today, may be applicable/useful in the future. If we change the behavior for ignore to apply, would it be a breaking change in rustdoc? Would it be better to just reject incompatible options in the first place? We might want to consider what doors gonna be closed if we do the ignoring way.

Random idea: Cargo could possibly have an unstable feature for bootstrap accepting RUSTDOC_FINALIZE_FLAGS (bad name) as a workaround. Eventually we would love to see a better interface of course.

[camelid]

I think like @notriddle said, the finalize step should have full knowledge about how all the crates were documented (i.e. what rustdoc flags they received). Semantically the finalize step is equivalent to the same logic placed at the end of each crate's documentation run, but for performance reasons we want to just do it once.

[notriddle]

There are a lot of flags, like --theme (and --resource-suffix) that need to be the same across all rustdoc runs in the workspace. Otherwise, you get broken output.

We would like to take those steps and freely move them between phases without breaking things. So we don’t just want to reject every flag that we currently ignore, because we might use it later.

Crate-specific flags don’t apply, but they’re crate-specific, so you can’t put them in RUSTDOCFLAGS anyway.

[weihanglo]

Makes sense. Thanks for the clarification!

[weihanglo]

Do you mind cleaning up commits to what you want to present them?

(like following commit structure in https://epage.github.io/dev/pr-style/)

View changes since this review

[camelid]

Sure, just left them like this for review. Will clean up now.

[weihanglo]

Thanks!

View changes since this review