Give precedence to html_root_url over --extern-html-root-url by default, but add a way to opt-in to the previous behavior

[jyn514]

What is an HTML root url?

It tells rustdoc where it should link when documentation for a crate is
not available locally; for example, when a crate is a dependency of a
crate documented with cargo doc --no-deps.

What is the difference between html_root_url and --extern-html-root-url?

Both of these tell rustdoc what the HTML root should be set to.
doc(html_root_url) is set by the crate author, while
--extern-html-root-url is set by the person documenting the crate.
These are often different. For example, docs.rs uses
--extern-html-root-url https://docs.rs/crate-name/version to ensure
all crates have documentation, even if html_root_url is not set.
Conversely, crates such as Rocket set doc(html_root_url = "https://api.rocket.rs"), because they prefer users to view the
documentation on their own site.

Crates also set html_root_url to ensure they have
documentation when building locally when offline. This is unfortunate to
require, because it's more work from the library author. It also makes
it impossible to distinguish between crates that want to be viewed on a
different site (e.g. Rocket) and crates that just want documentation to
be visible offline at all (e.g. Tokio). I have authored a separate
change to the API guidelines to no longer recommend doing this:
rust-lang/api-guidelines#230.

Why change the default?

In the past, docs.rs has been the main user of --extern-html-root-url.
However, it's useful for other projects as well. In particular, Cargo
wants to pass it by default when running --no-deps
(rust-lang/cargo#8296).

Unfortunately, for these other use cases, the priority order is
inverted. They want to give precedence to the URL the crate picks, and
only fall back to the --extern-html-root if no html_root_url is
present. That allows passing --extern-html-root unconditionally,
without having to parse the source code to see what attributes are
present.

For docs.rs, however, we still want to keep the old behavior, so that
all links on docs.rs stay on the site.

[rust-highfive]

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

[Nemo157]

docs.rs will now link some dependencies to external sites instead of docs.rs.

This feels like a bad change to me. A large part of the value I see in docs.rs is that it's a one-stop-location for all rustdoc output of crates.io crates. A couple of the first reasons I can think of:

• While rustdoc's HTML is usable without the JS enhancements, it's not the greatest experience, and I consider docs.rs a more trustworthy site to enable JS on than some random project's site.

• A lot of self-hosted documentation sites disappear over time from bitrot. While documentation can already include external links that go dead, having at the very least the rendered rustdoc available gives a decent baseline of documentation for all crates.

• It ensures a minimum of testing of generating the documentation on machines other than the developers. If the developers hosted documentation was used instead the first place any portability issues with building the documentation might be encountered would be one of their users attempting to build local docs. Currently there's a decent chance that if docs.rs fails to build the documentation the developers will notice this and investigate why. (Though docs.rs is more limited than users in some ways, like the lack of network access, but some might consider that a good thing to try and make the developers question why they are accessing the network in a build script 😁).

[dtolnay]

The rationale mentions Buck but I need to call out that this change is not so great for Buck. 😦

Generally how the Buck-based (and Bazel too) Rust build workflow works is third party dependencies are turned into build targets via a tool like https://github.com/facebookincubator/reindeer or https://github.com/google/cargo-raze, first party targets are handwritten depending on them just like you would write a Cargo.toml, and then the build tool directly invokes rustc and rustdoc according to the information present in the build targets. See e.g. https://github.com/dtolnay/cxx/blob/3dafaede930026394649639ac0798aa39d6b439f/BUCK#L1-L9 for an example of what this stuff looks like.

When it comes to rustdoc and third party open source libraries in the crates.io / docs.rs ecosystem, the behavior we need to accomplish is that third party libraries link to their preferred html_root_url if set, otherwise to docs.rs if not set, and first party code links to our own builds of the documentation hosted on private infrastructure.

The reason this change is not so good for Buck is that it requires targets downstream to heuristically figure out which of their upstream dependencies are third vs first party in order to construct an appropriate rustdoc invocation. Like if I have internal_crate1 which depends on serde + internal_crate2, then the rustdoc invocation corresponding to internal_crate1's docs build must figure out to pass --extern-html-root-url serde=docs.rs/... and --extern-html-root-url internal_crate2=docs.local/.... Reliably distinguishing which targets are first party or third party is complicated in a way that generalizes over the ways different monorepos make use of Buck. For example some heuristic like "if the top-level directory is named third-party" is brittle.

For supporting Buck's use case, a better approach from rustc and rustdoc would be to accept a fallback html-root-url as part of the downstream crate's build, with lower precedence than an html_root_url attribute set by the same crate. Then we'd have reindeer and cargo-raze emit third-party targets that look like (post Buck macro expansion):

rust_library(
    name = "serde",
    version = "1.0.123",
    srcs = [...],
    rustc_flags = ["--html-root-url=https://docs.rs/serde/1.0.123/serde"],
)

and similarly a url for the internal first party docs builds filled in by Buck macros for first party crates.

This accomplishes all of the objectives. Third party crates will link to their html_root_url if set, otherwise to the fallback location filled in by tooling, and first party will go to our own docs builds, and we avoid building any heuristics about identifying third party code into Buck itself.

[jyn514]

This feels like a bad change to me. A large part of the value I see in docs.rs is that it's a one-stop-location for all rustdoc output of crates.io crates.

Ok, that makes sense to me, I'll add a flag opting in to the old behavior.

For supporting Buck's use case, a better approach from rustc and rustdoc would be to accept a fallback html-root-url as part of the downstream crate's build, with lower precedence than an html_root_url attribute set by the same crate. Then we'd have reindeer and cargo-raze emit third-party targets that look like (post Buck macro expansion):

I'm not quite sure I understand - the snippet you pasted means you know at build time that serde is a third-party crate, because you pass docs.rs and not docs.local. Why is harder to tell serde is a third-party crate when building crates that depend on it? Couldn't you add some metadata field to rust_library that propagates the information? It seems weird to push that all the way back into rustdoc itself.

[jyn514]

ping @dtolnay - I think this may have gotten lost in your emails, I'm hoping to hear more about the Buck use case.

[dtolnay]

I'm not quite sure I understand - the snippet you pasted means you know at build time that serde is a third-party crate

Not quite. At build time (i.e. at the time that buck is executed to perform the build), it's just a rustc invocation with an opaque set of command line arguments that are not meaningful to Buck. There is no concept of "third party" baked into Buck so there is no way it would reason about what is or is not a third party crate or what the implications of the distinction are. "Third party" is only emergent from how Buck is used within some individual monorepo and how the developers have chosen to organize the code in the repo.

At the time of authoring the build targets (i.e. when the developer runs reindeer or cargo raze to translate them from upstream's Cargo.toml), definitely we know what is a third party crate, because those tools only ever operate on third party crates, so by definition "third party" == the set of targets imported via those tools. They, too, do not reason about what is or is not third party code; simply everything they operate on is regarded as third party code. The tool's job is importing third party code from the crates.io/docs.rs ecosystem into the Buck based monorepo internal ecosystem.

Why is it harder to tell serde is a third-party crate when building crates that depend on it?

Think of each Buck target (one rust_library invocation for example) as corresponding to a single invocation of a tool like rustc or rustdoc, with some arguments that possibly include output paths of the other targets it declares as dependencies, and produces one or more artifacts as outputs (like a linked executable, or an rlib, or a static lib). Sometimes the same target can be built in various "flavors" to control what the output is, for example #check to produce an rmeta only, #doc to produce rendered html documentation, and #default to produce rlib.

A small piece of Java logic in the Buck implementation of each rule (e.g. the implementation of rust_library) is responsible for rearranging the human-readable arguments from the configuration language, like name = "serde", into command line args, like '--crate-name' 'serde', or deps = ["//path/to:serde", "//path/to:serde_json"] into '--extern' 'serde=$(//path/to:serde)' '--extern' 'serde_json=$(//path/to:serde_json)', or features = ["std"] into '--cfg' 'feature="std"'. It's good when this is obvious and one-to-one, such that all the programmer is doing is writing down a straightforward rustc invocation in Buck syntax. They then get the ability to run that build in #check mode and #doc mode, build for various targets, build in ways that respect a global Buck configuration like what set of unstable features allowed, build at different optimization levels, etc.

Traversing the Buck targets of the dependencies to categorize which ones are third party is probably possible, but as you can tell is more involved than the above examples (going from features = ["std"] to '--cfg' 'feature="std"'). It involves looking at some other target's args, as opposed to its build outputs, which is suspicious.

[jyn514]

@dtolnay does this make things any harder for Buck? I do think this is the right change for the rest of the ecosystem, it would help with stabilizing -Z rustdoc-map and from what @SergioBenitez it sounds like rocket would also like to use it. Buck seems to have very specific requirements that I think should go through an RFC instead of me adding it as a one-off change.

[dtolnay]

It doesn't immediately. I raised it because the PR description discusses Buck but this isn't what Buck needs. Punting that use case seems okay.

Unrelated to Buck, it's slightly concerning that giving precedence to doc(html_root_url) leaves no way to opt out of a crate's chosen html_root_url, which means it's no longer possible for any docs.rs alternative to choose the opposite route as docs.rs on this issue, i.e. keep building everything itself and cross-linking to its own builds, as opposed to out to the internet. It's an indication that docs.rs's set of decisions are being imposed onto all other possible consumers of rustdoc, which is not necessarily wise, and an opportunity for better design that better isolates the two things. So while for my own use case this isn't a blocker, it's a new coupling that we should acknowledge (if I've understood correctly).

In particular, my sense is that docs.rs is a highly unusual consumer of rustdoc, and most other consumers of rustdoc do not have needs and priorities that are aligned with docs.rs.

• For example docs.rs aims to support "everything" that's published to crates.io, including things with transitive dependency on bizarre requirements of the environment they're built in. This is a highly unusual requirement that only docs.rs has. In a corporate environment (Buck-based monorepo or not) we import only a tiny fraction of crates.io, and know we can build all of it, using exactly all the rust flags we use for building production code. That means some reasons for a crate to choose its own html_root_url are less applicable to us than they are to docs.rs.

• Docs.rs is also on public internet, so whoever is accessing docs.rs is probably also able to connect to to a crate's chosen html_root_url website also on public internet. Meanwhile that's not the case for documentation hosted on intranet (firewalled or airgapped environments).

In response to your question whether this makes things harder for Buck, currently it doesn't because so far we are aiming to link to html_root_url whenever a crate has one, but if we ever needed to not do that (a crate's documentation site goes offline? a crate's documentation site does not document the set of features we primarily use?) there is no longer a way after this PR.

[Nemo157]

Looks like it should work fine for docs.rs, I assume you'll manage the sequencing so that docs.rs updates to use the flag on the nightly this comes out on 😜.

[jyn514]

Yup, I'll wait to merge this until I have time to make a docs.rs PR. I think it would be ok in practice for docs.rs not to pass the flag for a day or two though.

[jyn514]

@bors r=GuillaumeGomez

[bors]

📌 Commit 18f0f24 has been approved by GuillaumeGomez

[bors]

⌛ Testing commit 18f0f24 with merge b1928aa...

[bors]

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing b1928aa to master...

[ehuss]

Sorry, I'm a bit confused on the use case here. This broke cargo's use of --extern-html-root-url. I'm skeptical that it is desired to unconditionally do one method or another for all dependencies. For example, Cargo wants to link to std's docs locally, but that uses html_root_url, so Cargo needs to pass --extern-html-root-takes-precedence. Sorry, I didn't catch the comment you left on rust-lang/cargo#8296. I don't think it was decided anywhere that it is better for the crate author to have control over where things link, so I'm not sure who this change is for. I can add --extern-html-root-takes-precedence, but I'm a little confused. Was this specifically for Buck? I'm having trouble following the comments above.

[jyn514]

This broke cargo's use of --extern-html-root-url. I'm skeptical that it is desired to unconditionally do one method or another for all dependencies. For example, Cargo wants to link to std's docs locally, but that uses html_root_url, so Cargo needs to pass --extern-html-root-takes-precedence.

I thought you said in rust-lang/cargo#8296 that cargo needed a way to give html_root_url precedence before stabilizing -Zrustdoc-map :/

Should there be some way for the crate author to control the link destination? The documentation URL set in Cargo.toml is never consulted. I think this is unlikely to work with rustdoc, because it expects a certain structure to the API docs, and documentation URLs often point to user guides, or other things that aren't API docs. The #![doc(html_root_url = "...")] attribute is overridden by the --extern-html-root-url flag. This may be a good or bad thing depending on your perspective (should the user have control, or should the crate author?).

Is there something else that needs to happen? My long term goal is to stabilize -Zrustdoc-map and turn it on by default so that cargo doc --no-deps is no longer as broken.

[ehuss]

Ah, apologies for the confusion. I think in rust-lang/cargo#8296 I was mainly asking a question of what the correct behavior should be. I don't know the answer. I think there are legitimate arguments for both approaches (although this was good to highlight that unconditionally making the attribute take precedence prevents serving std docs locally).

I'll try to think it over and see if there is some kind of compromise. The flag here (--extern-html-root-takes-precedence) might be fine, I was just trying to understand what this was being added for. If it was for cargo, then I think this PR wasn't what we really needed. But if it was added for other users, then that would be good to know those use cases.

[jyn514]

I think it helped Rocket the most: rust-lang/api-guidelines#230 (comment). I would also be ok with just changing the default back but leaving a way for you to opt-in to the other precedence; but if that only helps Rocket it might not be worth the complexity.