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

Add support for rustdoc root URL mappings. #8287

Merged
merged 5 commits into from
May 30, 2020
Merged

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented May 26, 2020

This adds an experimental configuration setting to allow Cargo to pass the --extern-html-root-url flag to rustdoc. This flag allows rustdoc to link to other locations when a dependency is not locally documented. See the documentation in unstable.md for more details.

There are some known issues with this implementation:

  • Rustdoc doesn't seem to know much about renamed dependencies. The links it generates are to the package name, not the renamed name. The code is written to pass in package names, but if there are multiple dependencies to the same package, it won't work properly.

  • Similarly, if there are multiple versions of the same package within the dep graph, rustdoc will only link to one of them. To fix this, Cargo would need to pass metadata info into rustdoc (such as the package version).

  • If a dependency is built with different features than what is on docs.rs, some links may break.

  • This explodes the command-line length significantly. Before stabilizing, we may want to consider addressing that. I'm not sure if it would make sense to change rustdoc's interface, or to use response files?

  • This does not pass mappings for transitive dependencies. This normally isn't an issue, but can arise for re-exports (see the alt_registry test for an example). I'm not sure if this is a bug in rustdoc or not (there is a large number of issues regarding reexports and rustdoc). Cargo could include these, but this would make the command-line length even longer. Not sure what to do here.

  • The config value does not support environment variables. This would be very difficult to support, because Cargo doesn't retain the registry name in SourceId. I looked into fixing that, but it is very difficult, and hard to make it reliable.

I have tried to consider future changes in this design, to ensure it doesn't make them more difficult:

  • Single-tab browsing. This would be a mode where the std docs are merged with the local crate's docs so that the std docs are shown in the same place (and included in the index). This could be expressed with something like doc.extern-map.std = "include" or something like that. (Or maybe just use build-std?)

  • Direct-dependencies only. Often transitive dependencies aren't that interesting, and take up a lot of space in the output, and clog the search index. Some users want the ability to (locally) document their package + direct dependencies only. I think this could be implemented with some kind of command-line flag, perhaps with a config setting in the [doc] table. --extern-html-root-url flag will automatically handle second-level dependencies.

  • Manual-exclusions. Sometimes there are specific dependencies that are very expensive to document locally, but you still want everything else. I think this could be implemented with a command-line flag (--exclude winapi?), and the rustdoc-map feature would automatically link those excluded crates' items to docs.rs. This could also be added to the [doc] table.

We can also consider at any time to change the defaults (such as making crates-io = "https://docs.rs" the default). It could also potentially auto-detect std = "local", although rustdoc could do the same internally.

Closes #6279

@rust-highfive
Copy link

r? @Eh2406

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 26, 2020
@ehuss
Copy link
Contributor Author

ehuss commented May 26, 2020

cc @yaahc @GuillaumeGomez

@GuillaumeGomez
Copy link
Member

👍

@alexcrichton
Copy link
Member

This seems like a neat feature! Reading over this it looks like the command line length is on the order of the size of --extern, so I'm curious if I'm missing something about where info is coming from?

Otherwise the issues about renaming/multiple deps sound like good issues for upstream to fix before stabilizing this, but otherwise this seems like a pretty reasonable feature to me.

To confirm, the main use case here is cargo doc --no-deps, right? If so should we consider perhaps defaulting to --no-deps?

One thing I'd also wonder, this ignores the package's listed documentation URL, which often may be more accurate than a blanket "everything in this registry is documented here" scheme?

@ehuss
Copy link
Contributor Author

ehuss commented May 27, 2020

I'm missing something about where info is coming from?

Right, it is a separate --extern-html-root-url foo=https://docs.rs/foo/1.2.3/ flag for each direct dependency. The command-line length increase isn't bad, it's just a concern that is always in the back of my mind when adding a bunch of options. On my system, documenting Cargo goes from 5200 bytes to 8400.

IIRC, windows API is limited to 32767 (and the cmd prompt itself is limited to 8191, which means I wouldn't be able to copy and paste it in a terminal).

To confirm, the main use case here is cargo doc --no-deps, right?

Correct.

If so should we consider perhaps defaulting to --no-deps?

I'm uncertain about that. There would need to be a new flag to do the opposite (--deps?). And I think there is value to have the dependencies in the local rustdoc search index, and to be locally available (as opposed to accessing docs.rs).

The change I would probably be more confident about is to make the default be "direct dependencies only", since transitive dependencies usually aren't interesting. A variant of that would be "direct dependencies + publicly accessible dependencies" when pub/priv dependencies are a thing, but that's a long way off, and seems complicated and maybe unnecessary.

And maybe some config option would also be a possibility.

One thing I'd also wonder, this ignores the package's listed documentation URL, which often may be more accurate than a blanket "everything in this registry is documented here" scheme?

Hm, yea, I hadn't thought about that. Unfortunately the documentation URL may be difficult to integrate. It doesn't differentiate on version. And, for example, serde's link is https://docs.serde.rs/serde/, but rustdoc wants the links without the crate name (like https://docs.serde.rs/). Also, I notice those links sometimes point to "user guides" instead of API docs, or to a GitHub repo, or whatever.

I was going to say that the #![doc(html_root_url = "...")] attribute would be better, but unfortunately the --extern-html-root-url flag takes precedence over the attribute.

I suspect in practice there aren't many packages where it matters. I had thought about allowing individual overrides (like winapi = "https://example.com/"), but that seemed like overkill. You can still override with RUSTDOCFLAGS. Specific package settings could be added later in a separate table (like [doc.extern-map.packages]).

@yaahc
Copy link
Member

yaahc commented May 27, 2020

This all looks good to me, excited about the prospect of single tab browsing!

@alexcrichton
Copy link
Member

I don't think there's really any harm in landing this at this time. I also think though there's quite a few questions, some very fundamental, which may prevent this from being stabilized. I think some issues are:

  • Dealing with renames/duplicate crates (hopefully an easy-enough rustdoc PR)
  • The documentation URL isn't consulted, nor can it really be (somewhat fundamental)
  • Unclear how this will affect default builds (switching to --no-deps seems bad, switching to just immediate deps is less bad but still not great, etc)

I think I'm basically not entirely clear on how we expect this to be used. For example is everyone using this expected to write some form of config file? Is there any other registry for which a config file can actually be written? (etc, etc)

Do you think it's good to go ahead and land this in the meantime though @ehuss?

@ehuss
Copy link
Contributor Author

ehuss commented May 29, 2020

This is much closer to "experimental" than "ready to stabilize", definitely some issues to resolve, that was kinda the point of getting it started — to identify exactly what's broken, and how to make the UX better.

I'm thinking I'd like to land this as-is for now. We can then experiment with different defaults. One possibility is to:

  1. Default to crates-io = "https://docs.rs/". I think this is a relatively safe change, since it only enhances the current behavior (people who use --no-deps will now have links instead of no links in their docs).
    1a. I'm less confident about changing the default of std, so I'd probably leave that as-is for now, and see if there is much feedback about preferring local docs over remote ones.
  2. Consider adding a config to control the default deps behavior for plain cargo doc. This would also need a new command-line flag to override the config.
  3. Consider setting the default of the deps behavior to "direct dependencies only". This is a tougher decision, since it is a big change. Personally I think it makes more sense, but it could cause problems, or maybe I'm not seeing some use case. Maybe solicit community feedback for that?

So assuming we do step 1 and 2, I'd expect if a user wants this new behavior, they would add this to ~/.cargo/config:

[doc]
default-deps = "direct"  # or whatever syntax we come up with

And if we do step 3, there would be no need for the typical user to set anything in their config, and only advanced users would need to override anything.

@alexcrichton
Copy link
Member

Ok sounds reasonable to me! Do you think this is all worth writing up in a tracking issue on the Cargo side? Or perhaps directly into the unstable documentation?

In any case r=me with a tracking issue filled in

@ehuss ehuss mentioned this pull request May 30, 2020
11 tasks
@ehuss
Copy link
Contributor Author

ehuss commented May 30, 2020

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented May 30, 2020

📌 Commit f2d32ac has been approved by alexcrichton

@bors 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 May 30, 2020
@bors
Copy link
Contributor

bors commented May 30, 2020

⌛ Testing commit f2d32ac with merge 00fe8a5...

@bors
Copy link
Contributor

bors commented May 30, 2020

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing 00fe8a5 to master...

@bors bors merged commit 00fe8a5 into rust-lang:master May 30, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2020
Update cargo

9 commits in 9fcb8c1d20c17f51054f7aa4e08ff28d381fe096..40ebd52206e25c7a576ee42c137cc06a745a167a
2020-05-25 16:25:36 +0000 to 2020-06-01 22:35:00 +0000
- Warn if using hash in git URL, Fixes rust-lang/cargo#8241 (rust-lang/cargo#8297)
- reset lockfile information between resolutions (rust-lang/cargo#8274)
- Disable strip_works test on macos. (rust-lang/cargo#8301)
- Fix typo in impl Display for Strip (rust-lang/cargo#8299)
- Add support for rustdoc root URL mappings. (rust-lang/cargo#8287)
- Fix tests with enoent error message on non-english systems. (rust-lang/cargo#8295)
- Fix fingerprinting for lld on Windows with dylib. (rust-lang/cargo#8290)
- Fix a typo (rust-lang/cargo#8289)
- Fix several issues with close_output test. (rust-lang/cargo#8286)
@ehuss ehuss added this to the 1.46.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support rustdoc root-url feature
7 participants