Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't recommend setting html_root_url #230

Merged
merged 1 commit into from
Aug 5, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Nov 18, 2020

See #229 for discussion.

r? @BurntSushi

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I don't have merge permission on this repo. And it would probably be good to give it a few days for others to chime in.

@dtolnay
Copy link
Member

dtolnay commented Nov 18, 2020

FWIW in my work codebase, in which all Rust projects (and all other languages) are built by Buck, there is no Cargo involved and Buck makes rustc and rustdoc invocations directly. See https://buck.build/rule/rust_library.html. When building documentation of a Rust target (buck build path/to/my/target#doc) third party code without an html_root_url would not be able to be linked.

@jyn514
Copy link
Member Author

jyn514 commented Nov 19, 2020

@dtolnay I'm not very familiar with buck - does it use a shared build directory between dependencies? If so, rustdoc will pick up the documentation from the other dependencies without either --extern-html-root-url or html_root_url.

@dtolnay
Copy link
Member

dtolnay commented Nov 19, 2020

It does not use a shared build directory between dependencies. For example the output of buck build path/to/my/target#doc would always go into buck-out/gen/path/to/my/target#doc. If that target depends on some other target, the other target's docs are correspondingly somewhere else.

@jyn514
Copy link
Member Author

jyn514 commented Nov 19, 2020

Hmm, I see. I'd expect most of the links to be broken then :/ I don't know many libraries setting html_root_url.

On nightly there's --extern-html-root-url, but the docs for buck say 1.33 is supported, so it wouldn't work on stable.

@dtolnay
Copy link
Member

dtolnay commented Nov 19, 2020

We're using the 1.48 prerelease so that's not a problem. We actually do use --extern-html-root-url for cross linking first-party code.

Regarding this:

When rustdoc needs to generate a link to an item in an external crate, it will first check if the extern crate has been documented locally on-disk, and if so link directly to it. Failing that, it will use the URL given by the --extern-html-root-url command-line flag if available. If that is not available, then it will use the html_root_url value in the extern crate if it is available. If that is not available, then the extern items will not be linked.

The reason I don't think this fits our use case is that the priority order is backwards. We can easily make Buck always pass a docs.rs extern-html-root-url for all third-party crates (like you might have been expecting rust-lang/cargo#8296 to wire up in the Cargo case) but that's wrong: Buck doesn't contain a Rust parser so it would have no idea whether the crate it's building already contains an html_root_url attribute. I think we'd be all set if html_root_url took precedence over extern-html-root-url but I'm not sure what other use case that breaks. Maybe this needs some separate flag.

@jyn514
Copy link
Member Author

jyn514 commented Nov 19, 2020

I'm trying to imagine the scenario where html_root_url should take precedence - are you imagining pointing to a site other than docs.rs, like https://api.rocket.rs/ or something? I guess it does make sense in that case.

In general I don't think there's a clear answer here: docs.rs definitely wants --extern-html-root-url to take precedence, so we can't break that use case either. I'd be willing to add a switch toggling the precedence though.

@dtolnay
Copy link
Member

dtolnay commented Nov 19, 2020

Yes exactly. Or third party code that's not in the crates.io / docs.rs ecosystem at all.

@jyn514
Copy link
Member Author

jyn514 commented Nov 19, 2020

Ok, so it sounds like there are two possible paths forward, both of which allow getting rid of html_root_url in the common case when you just want to link to docs.rs.

  1. Have a flag for html_root_url to take precedence over --extern-html-root-url. Buck/Bazel/Cargo/etc. can unconditionally pass --extern-html-root-url without generating broken links, and the documentation will link to the right place automatically. Docs.rs can unconditionally pass the flag, without the override, because it knows all crates from crates.io are documented on docs.rs.
  2. Have rustdoc link to docs.rs whenever html_root_url is unset and --extern-html-root-url is unset. Docs.rs continues passing --extern-html-root-url to override external docs (since it never wants to link to https://api.rocket.rs), buck doesn't have to pass it except when it actually wants to override docs.rs, and Tracking issue for -Zrustdoc-map cargo#8296 is no longer necessary.

I think doing both would be a mistake - do you have a preference for one or the other?

@jyn514
Copy link
Member Author

jyn514 commented Nov 19, 2020

cc @rust-lang/rustdoc, @rust-lang/cargo: for context, we're talking about how to avoid broken links when documenting with --no-deps or a build system other than cargo.

@dtolnay
Copy link
Member

dtolnay commented Nov 19, 2020

Speaking for my own codebase the absolute best for us would actually be a rustc flag, not a rustdoc flag, which specified a "fallback html_root_url" for the current crate i.e. not DEP=URL like for --extern-html-root-url. We'd pass that from Buck during the compilation of third party code, and then when a doc build later depends on it, the links would go to the right place i.e. the crate's actual html_root_url if it provides one, otherwise the docs.rs fallback fed by our Buck config.

@BurntSushi
Copy link
Member

Either way, it seems like we don't need to suggest setting html_root_url in the API guidelines?

@jyn514
Copy link
Member Author

jyn514 commented Nov 19, 2020

@BurntSushi well, neither suggestion has been implemented yet. It depends whether you think the edge cases are worth worrying about.

@ehuss
Copy link
Contributor

ehuss commented Nov 25, 2020

  1. Have rustdoc link to docs.rs whenever html_root_url is unset and --extern-html-root-url is unset.

In this situation, how would rustdoc know this is a crates.io crate? I don't think rustdoc should have any automatic linking to docs.rs without some kind of command-line opt-in (or -Zrustdoc-map, which knows which crates exist on crates.io).

Either way, it seems like we don't need to suggest setting html_root_url in the API guidelines?

I think it would be reasonable to remove it. There are few scenarios where it is useful. One is running cargo doc --no-deps, but you want dependency links to still work. In that case, I think the user should just remove --no-deps, or use -Zrustdoc-map. Only about 4% of crates on crates.io set it, of those only 225 set it to a non-docs.rs site. In practice, I don't think it is in enough widespread use to warrant suggesting everyone set it. I think it only makes sense if you really want to link to a non-docs.rs site, which very few do.

@jyn514
Copy link
Member Author

jyn514 commented Nov 25, 2020

In this situation, how would rustdoc know this is a crates.io crate?

Ahh, good point, I hadn't thought about path dependencies or private registries. I think having an option for html_root_url to override --extern-html-root-url is the way to go, then.

@jyn514
Copy link
Member Author

jyn514 commented Nov 25, 2020

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?).

Maybe the flag addresses this, too? The user can override -Z rustdoc-map with html_root_url by passing the new flag (bikeshed: --html-root-takes-precedence). Or alternatively rustdoc can switch the default, and you can opt-in to having --extern-html-root-url take precedence, I don't feel strongly about what the default should be.

@KodrAus
Copy link
Contributor

KodrAus commented Dec 22, 2020

Kicked off an FCP in #229 for this.

@KodrAus KodrAus added deletion needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Dec 22, 2020
@jyn514
Copy link
Member Author

jyn514 commented Feb 13, 2021

@KodrAus the FCP finished, so I think this can be merged :)

@SergioBenitez
Copy link

All: I am quite confused about the rationale to stop recommending this attribute. The crux of the argument, if I'm reading this right, is that 1) docs.rs overrides the URL when it builds docs anyway, and 2) it's hard to keep the URL up to date. I find the former to be rather problematic with consideration for decentralization, local docs, and development docs, and the latter a non-issue.

On 1: not only does this point entirely ignore local documentation building, but it also ignores crates that host their own documentation. Rocket, for example, hosts its docs at https://api.rocket.rs. Hosting our own docs allows us to provide redundancy, ensuring docs are accessible should something go wrong with docs.rs, while also allowing us to ensure docs are always up to date without waiting or relying on a third-party source. This latter point is especially important when releasing a new version. The delay between publishing a new release and docs.rs building and updating docs can sometimes be considerable.

What's more, Rocket hosts docs for development versions that docs.rs does not and cannot build. See, for instance, https://api.rocket.rs/master. Without something like html_root_url or considerable manual effort to determine and set where every dependency's docs live, these docs, of at least equal importance, will contain broken links and be out of parity with the docs at docs.rs. It should be possible to independently build documentation that is as good as what's on docs.rs; removing the recommendation makes this considerably more difficult.

On 2: crates generally mention the version string in more places than just Cargo.toml, all of which need to be kept in sync as well, and so html_root_url shouldn't pose considerably more burden. For example, a crate's README often contains the TOML needed to add the crate as a dependency, which contains the version string. While this duplication is unfortunate, tools, like the one posted above, can ameliorate the situation. Rocket, for instance, automates changing the version number via a script to ensure they are all synchronized. Thus, adding one more version string to keep up-to-date should not pose considerably more burden, and certainly less so then needing to track down and keep up-to-date every dependency's preferred documentation URL should html_root_url be considered out of vogue entirely.

In summary, we should advocate for being able to build docs as good as those on docs.rs without being docs.rs for a variety of reasons, and the overhead to allowing this is minimal given existing realities. As such, it is my opinion that the recommendation for html_root_url should stay.

@jyn514
Copy link
Member Author

jyn514 commented Mar 4, 2021

@SergioBenitez you can still do this perfectly well by simply not passing --no-deps when you document your crate.

@jyn514
Copy link
Member Author

jyn514 commented Mar 4, 2021

Again, from #229, the only case where html_root_url actually affects things is when a downstream crate is documented with --no-deps and re-exports or links to one of your types. If any of those things aren't the case, then html_root_url does nothing at all.

@SergioBenitez
Copy link

@jyn514 This doesn't change any of the points I've made. I still cannot build docs like docs.rs can without being docs.rs. What's more, building the docs for all dependencies is wasteful, needlessly expensive, and creates clutter in the index.

@jyn514
Copy link
Member Author

jyn514 commented Mar 4, 2021

I still cannot build docs like docs.rs can without being docs.rs.

You can build docs like docs.rs by passing -Z rustdoc-map the way it does: rust-lang/cargo#8296. That does require a nightly toolchain, but it's significantly more reliable than html_root_url, which most crates already don't set. If you want to go even further, you can build things exactly the same way as docs.rs by using the library we provide: https://github.com/rust-lang/docs.rs/blob/master/crates/metadata/lib.rs#L3.

I'm not trying to make a value judgement on whether or not you should use docs.rs. I just think this feature doesn't pull its weight, and I think it would better to stabilize -Z rustdoc-map instead of adding another task for library authors.

@SergioBenitez
Copy link

You can build docs like docs.rs by passing -Z rustdoc-map the way it does: rust-lang/cargo#8296. That does require a nightly toolchain, but it's significantly more reliable than html_root_url, which most crates already don't set. If you want to go even further, you can build things exactly the same way as docs.rs by using the library we provide: https://github.com/rust-lang/docs.rs/blob/master/crates/metadata/lib.rs#L3.

Thank you for bringing rustdoc-map to my attention! I was not aware of it prior. This appears to solve the "considerable manual effort to determine and set where every dependency's docs live" as long as we want to point all crate docs to a centralized source like docs.rs. It still does not provide a mechanism, however, for a crate author to dictate where its docs live and for its dependencies to point to that location without considerable effort. So while I see rustdoc-map as a wonderful fallback when html_root_url is not set, it is not a replacement.

I'm not trying to make a value judgement on whether or not you should use docs.rs. I just think this feature doesn't pull its weight, and I think it would better to stabilize -Z rustdoc-map instead of adding another task for library authors.

I think a nice compromise would be to:

  • Set rustdoc-map = docs.rs by default.
  • Have cargo-doc determine a crate's doc locations via html_root_url, then extern-html-root-url, then html_root_url, in that order of preference.
  • Change the guidelines to suggest setting html_root_url only when the preferred doc location is not docs.rs.

This would resolve all of my concerns while also avoiding the need for html_root_url when a crate author wants docs.rs to host their docs.

@jyn514
Copy link
Member Author

jyn514 commented Apr 21, 2021

Have cargo-doc determine a crate's doc locations via html_root_url, then extern-html-root-url, then html_root_url, in that order of preference.

You said html_root_url twice - was there meant to be a third option?

@SergioBenitez
Copy link

Have cargo-doc determine a crate's doc locations via html_root_url, then extern-html-root-url, then html_root_url, in that order of preference.

You said html_root_url twice - was there meant to be a third option?

Indeed! Good catch. I meant to write rustdoc-map as the third option.

@jyn514
Copy link
Member Author

jyn514 commented Apr 21, 2021

@SergioBenitez rustdoc-map is implemented in terms of extern-html-root-url, they can't have different precedence.

@jyn514
Copy link
Member Author

jyn514 commented Aug 5, 2021

@KodrAus could you merge this? The FCP finished several months ago.

@Manishearth Manishearth merged commit ac63584 into rust-lang:master Aug 5, 2021
@jyn514 jyn514 deleted the html-root-url branch August 5, 2021 04:01
davidpdrsn pushed a commit to tokio-rs/axum that referenced this pull request Aug 7, 2021
* Feature-gate test that depends on non-default features

Makes `cargo check` work without extra flags.

* Don't set doc(html_root_url)

It is no longer recommended:
rust-lang/api-guidelines#230

* Remove documentation URL from Cargo.toml

crates.io will link to the right version on docs.rs automatically.

* Ensure toolchains installed by actions-rs/toolchain are actually used

* Fix missing rustup component in check job

* Raise MSRV to 1.51

Older versions weren't actually working before.

* Only run clippy & rustfmt on stable toolchain

MSRV is checked in test-versions.

* Allow cargo doc to succeed without headers and multipart features

CI will still ensure that intra-doc links that rely on these are not broken.
jyn514 added a commit to jyn514/rust that referenced this pull request Aug 19, 2021
…efault, but add a way to opt-in to the previous behavior

 ## 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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 21, 2021
…meGomez

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

## 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.
jplatte added a commit to jplatte/tower that referenced this pull request Oct 15, 2021
It is no longer recommended to set it:
rust-lang/api-guidelines#230
jplatte added a commit to jplatte/tower that referenced this pull request Oct 15, 2021
It is no longer recommended to set it:
rust-lang/api-guidelines#230
eldruin added a commit to eldruin/embedded-hal that referenced this pull request Nov 19, 2021
jplatte added a commit to jplatte/tower that referenced this pull request Jan 17, 2022
It is no longer recommended to set it:
rust-lang/api-guidelines#230
jplatte added a commit to jplatte/tower that referenced this pull request Jan 17, 2022
It is no longer recommended to set it:
rust-lang/api-guidelines#230
olix0r pushed a commit to tower-rs/tower that referenced this pull request Jan 17, 2022
`tower` currently has required dependencies that may not be used
unless certain features are enabled.

This change updates `tower` to make these dependencies optional.

Furthermore, this change removes use of `html_root_url`, which is no
longer recommended (rust-lang/api-guidelines#230),
and updates the documented release instructions.
joshtriplett added a commit to rust-lang/libz-sys that referenced this pull request May 26, 2022
Per rust-lang/api-guidelines#230 .

This also avoids having something that needs to be kept updated.
tarcieri added a commit to dalek-cryptography/curve25519-dalek that referenced this pull request Dec 26, 2022
The recommendation to set this has been removed from the Rust API
guidelines:

rust-lang/api-guidelines#230

It used to be used by docs.rs, but docs.rs now unconditionally sets the
`--extern-html-root-url` parameter of rustdoc which overrides it, making
it no longer needed and superfluous.
rozbb pushed a commit to dalek-cryptography/curve25519-dalek that referenced this pull request Dec 27, 2022
The recommendation to set this has been removed from the Rust API
guidelines:

rust-lang/api-guidelines#230

It used to be used by docs.rs, but docs.rs now unconditionally sets the
`--extern-html-root-url` parameter of rustdoc which overrides it, making
it no longer needed and superfluous.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deletion needs-fcp This change is insta-stable, so needs a completed FCP to proceed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants