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

Link to dependency of dependency #1588

Closed
djeedai opened this issue Jan 9, 2022 · 8 comments
Closed

Link to dependency of dependency #1588

djeedai opened this issue Jan 9, 2022 · 8 comments

Comments

@djeedai
Copy link

djeedai commented Jan 9, 2022

This is a follow-up issue from a discussion on Discord with @Nemo157 about a bug in a crate's doc trying to link a type in another crate, which is actually a re-export from a third crate (dependency-of-dependency).

Description

The situation is as follow:

  • The Bevy game engine has a top-level "facade"* crate bevy that projects are expected to take a dependency on.
  • Bevy itself is implemented in many crates, bevy_XXX, and the public types are re-exported from the facade.
  • A third-party crate which wants to link a doc to a Bevy type will typically attempt to use the same module path as the Rust code it uses, that is a path starting with the facade bevy::.

*I'm using "facade" in this issue as what I think it means, a "top-level" crate that re-export many other "internal" ones, but maybe I misunderstood the meaning given in rust-lang/rust#22083. If so, sorry for the confusion, and please take "facade" as the meaning I just described.

The result is that unfortunately links on docs.rs are broken, rendering verbatim instead of being actual links to the HTML page of the documentation of the type.

Bug example

Note: To avoid confusion, the example is from my crate bevy_tweening, which is not an official Bevy crate, and not part of the official repo. So in particular that crate has no direct access to internal Bevy crates, and should be seen as "third-party".

See an example of published crate here, where the [Entity] link toward the top of the page doesn't link anywhere (the HTML link contains the verbatim Rust path bevy::ecs::entity::Entity). The source code those docs are generated from is:

https://github.com/djeedai/bevy_extra/blob/f9c8049fb7674ea314a2c1a9c59a2b316d5d0abf/crates/bevy_tweening/src/lib.rs#L32

For reference (as for some reason GitHub refuses to expand the above link into code) the link is:

/// Animate the position ([`Transform::translation`]) of an [`Entity`]:
///
/// ...
///
/// [`Entity`]: bevy::ecs::entity::Entity

The Entity type is defined in the internal crate bevy_ecs, then re-exported in the facade crate bevy under the bevy::ecs::entity::Entity path.

Repro steps

I'm mentioning it here for completeness because @Nemo157 provided the command to run to replicate what docs.rs does, to test this:

  • First, delete old docs, as otherwise rustdoc seem to link them : rm -rf target/doc or similar for other OSes
  • Then run cargo doc -Zrustdoc-map --no-deps

Workaround

A workaround would be to directly link the internal type. Unfortunately since Bevy uses a facade crate, a third-party crate typically does not directly take a dependency to those Bevy internal crates, only to the facade, so simply changing the link makes the build fail. This means in addition of changing all broken links to target internal Bevy crates, a crate maintainer has to add a docs.rs-specific feature to depend on all those internal crates for documentation purpose only.

The html_root_url attribute was also mentioned, and I'm chatting with the Bevy docs team (CC: @alice-i-cecile) to see if we can try it, but we haven't yet.

Related issues

As explained by @Nemo157 on Discord, the scenario detailed above is likely related to rust-lang/rust#22083 and rust-lang/rfcs#3011, although it was under our impression that those issues do not cover entirely the problem at hand, and in particular the dependency-of-dependency linking.

@djeedai
Copy link
Author

djeedai commented Jan 9, 2022

Note: unfortunately the workaround does not work for Bevy, I believe due to some bug in the Component macro derive. I'm not sure this can be fixed, so that makes this issue all the more annoying for Bevy crate maintainers.

@jyn514
Copy link
Member

jyn514 commented Feb 15, 2022

I don't quite understand why this happens (I would expect -Zrustdoc-map to fix any issue with facade crates), but if you can reproduce it locally then it's a rustdoc bug and not a docs.rs bug; you should report it in rust-lang/rust.

@Nemo157
Copy link
Member

Nemo157 commented Feb 17, 2022

The issue is with -Zrustdoc-map being shallow; so technically this is a cargo bug (IIRC there's no upstream issue directly about this, just an unresolved question on the tracking issue). But it's also a docs.rs bug since our use of -Zrustdoc-map should be transparent to users; and we're almost certainly the majority users of the feature currently so it's likely up to us to fix it.

@Nemo157
Copy link
Member

Nemo157 commented Feb 17, 2022

Ah, and I remember what the related issues are about now. If we fix -Zrustdoc-map to support deep linking; then this will not actually support what bevy wants, which is to shallowly link to the facade rather than their internal crates.

@jyn514
Copy link
Member

jyn514 commented Mar 11, 2023

cc rust-lang/cargo#8296 (comment), rust-lang/rust#42301.

Ah, and I remember what the related issues are about now. If we fix -Zrustdoc-map to support deep linking; then this will not actually support what bevy wants, which is to shallowly link to the facade rather than their internal crates.

@dtolnay had an interesting suggestion in #2081:


If rustdoc in --no-deps mode is building the docs of crate A (tracing-opentelemetry) and has an intra doc link to opentelemetry::trace::SpanKind in crate B (opentelemetry) which is a re-export of the type opentelemetry_api::trace::SpanKind from crate C (opentelemetry_api), the following situations could occur:

If rustdoc has --extern-html-root-url=B=... but not --extern-html-root-url=C=..., today it generates a broken link. Is it possible / would it be reasonable to generate a link to the correct path under B's docs which will perform the necessary redirect to C's docs?


I think that would do the correct thing for bevy, without needing rust-lang/rfcs#3011 to be fixed first, but it will do the wrong thing for non-facade crates, so making -Zrustdoc-map include transitive dependencies in the future would be a regression for bevy.

As an aside, this issue is large/complicated enough and spread across sufficiently many projects that I think any change here should involve an RFC.

facebook-github-bot pushed a commit to facebook/fbthrift that referenced this issue Apr 28, 2023
Summary:
the rustdocs for mocks can be a bit confusing because there's no documentation for any of the individual mock fields. this is because the module in which the mock field objects are defined is private.

to expose them in rustdoc, just `pub` that module. each of the mock field objects have private fields, so downstream crates will still be unable to instantiate these mock fields on their own (and this makes `unimplemented` `pub(crate)` to ensure that as well).

the link to some mock fields is still broken, e.g. `FacebookService`, but that's because of rust-lang/docs.rs#1588 (comment).

Reviewed By: thepacketgeek

Differential Revision: D45406779

fbshipit-source-id: 91f14ef2e022857969540c82d2482e81bda1421c
facebook-github-bot pushed a commit to facebook/hhvm that referenced this issue Apr 28, 2023
Summary:
the rustdocs for mocks can be a bit confusing because there's no documentation for any of the individual mock fields. this is because the module in which the mock field objects are defined is private.

to expose them in rustdoc, just `pub` that module. each of the mock field objects have private fields, so downstream crates will still be unable to instantiate these mock fields on their own (and this makes `unimplemented` `pub(crate)` to ensure that as well).

the link to some mock fields is still broken, e.g. `FacebookService`, but that's because of rust-lang/docs.rs#1588 (comment).

Reviewed By: thepacketgeek

Differential Revision: D45406779

fbshipit-source-id: 91f14ef2e022857969540c82d2482e81bda1421c
tguichaoua added a commit to tguichaoua/bevy_cursor that referenced this issue Aug 12, 2023
tguichaoua added a commit to tguichaoua/bevy_cursor that referenced this issue Aug 12, 2023
* readme: fix link to bevy doc

* workaround to link to bevy type

see rust-lang/docs.rs#1588

* readme: move the link to bevy at the end

* add a link to `CursorInfo` doc
rparrett added a commit to rparrett/bevy_simple_text_input that referenced this issue Feb 28, 2024
Links to Bevy types don't work on docs.rs. The workaround is to
use actual url to docs.rs. That seems tedious and fragile, so
we'll just remove the links and inser the word "Bevy" so that
people know where to look.

See rust-lang/docs.rs#1588
@Nemo157
Copy link
Member

Nemo157 commented Mar 11, 2024

Since a few nightlies ago, -Zrustdoc-map has been including transitive dependencies too, (rust-lang/cargo#13481), so links should be working, but will go all the way to the origin crate, not the facade.

@syphar
Copy link
Member

syphar commented Mar 12, 2024

@Nemo157 so this issue can be closed? Or is something is missing?

Or do we just need a rebuild?

@Nemo157
Copy link
Member

Nemo157 commented Mar 12, 2024

Yeah I guess so, the main thing we somewhat controlled about getting the html-root-url passed through has been fixed, having links to the facade instead of the origin requires a new rustdoc feature (tracked in rust-lang/rfcs#3011) and shouldn't require any docs.rs changes.

@Nemo157 Nemo157 closed this as completed Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants