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

explicitly link to external ena docs #88813

Merged
merged 1 commit into from
Sep 12, 2021
Merged

explicitly link to external ena docs #88813

merged 1 commit into from
Sep 12, 2021

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Sep 10, 2021

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 Sep 10, 2021
@jyn514
Copy link
Member

jyn514 commented Sep 10, 2021

r? @jyn514

@lcnr is there a reason you did this instead of -Zrustdoc-map? Using --extern-html-root-url directly means you have to pass it yourself for each dependency.

(As discussed on Zulip, this probably works for indexmap because it has a doc(html_root_url) attribute it sets.)

@jyn514 jyn514 added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Sep 10, 2021
src/bootstrap/doc.rs Outdated Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

Hm, maybe we should just remove the no-deps flag in rustc docs generation?

As far as I can tell, there's no real reason for that exclusion, so we can probably just drop that which would probably fix all these links?

src/bootstrap/doc.rs Outdated Show resolved Hide resolved
@lcnr
Copy link
Contributor Author

lcnr commented Sep 11, 2021

I've now tried -Zrustdoc-map but that didn't seem to fix my issue:

diff --git a/library/backtrace b/library/backtrace
--- a/library/backtrace
+++ b/library/backtrace
@@ -1 +1 @@
-Subproject commit 4f925f8d81dfa57067537217e501e1dff7433491
+Subproject commit 4f925f8d81dfa57067537217e501e1dff7433491-dirty
diff --git a/src/bootstrap/doc.rs b/src/bootstrap/doc.rs
index c8714117930..b12bd43ab29 100644
--- a/src/bootstrap/doc.rs
+++ b/src/bootstrap/doc.rs
@@ -590,10 +590,16 @@ fn run(self, builder: &Builder<'_>) {
         cargo.rustdocflag("-Znormalize-docs");
         cargo.rustdocflag("--show-type-layout");
@@ -590,10 +590,15 @@ fn run(self, builder: &Builder<'_>) {
         cargo.rustdocflag("-Znormalize-docs");
         cargo.rustdocflag("--show-type-layout");
         compile::rustc_cargo(builder, &mut cargo, target);
+
+        cargo.arg("-Zunstable-options");
         cargo.arg("-Zskip-rustdoc-fingerprint");
 
         // Only include compiler crates, no dependencies of those, such as `libc`.
+        // Do link to dependencies on `docs.rs` however using `rustdoc-map`.
         cargo.arg("--no-deps");
+        cargo.arg("-Zrustdoc-map");
+        cargo.arg(r#"--config=doc.extern-map.registries.crates-io="https://docs.rs""#);
 
         // Find dependencies for top level crates.
         let mut compiler_crates = HashSet::new();

with this ./x.py doc --open --stage 1 still doesn't link to ena

@jyn514
Copy link
Member

jyn514 commented Sep 11, 2021

Hmm ok - cargo is trying to pass the flags, I see --extern-html-root-url 'smallvec=https://docs.rs/smallvec/1.6.1/' --extern-html-root-url 'tracing=https://docs.rs/tracing/0.1.25/' in the command line. But ena is missing. I think the issue is rustc_infer doesn't depend directly on ena instead, it depends on rustc_data_structures which re-exports an ena module. @ehuss mentioned this in the tracking issue: rust-lang/cargo#8296

Decide if Cargo should pass mappings for transitive dependencies (currently it does not). 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.

@ehuss what do you think about including the transitive dependencies in the command line? Would it help if rustc exposed a list of crates which have re-exported types?

@jyn514 jyn514 closed this Sep 11, 2021
@jyn514 jyn514 reopened this Sep 11, 2021
@jyn514
Copy link
Member

jyn514 commented Sep 11, 2021

In the meantime, @lcnr I agree with Mark that just removing --no-deps is fine and should fix the links :)

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 11, 2021
@lcnr
Copy link
Contributor Author

lcnr commented Sep 11, 2021

tried out the documentation with --no-deps removed and now understand why it was added 😅

we depend on enough crates that searching for things like hir, Expr or literal has a lot of non-rustc results which is pretty annoying 😅 I prefer either using --rustdoc-map - which seems to still be a bit incomplete - or explicitly whitelisting ena for now, which is what this PR is currently doing.
Screenshot from 2021-09-11 18-25-14

@jyn514
Copy link
Member

jyn514 commented Sep 11, 2021

Hmm, ok - I still am not in favor of special casing ena though, since there are probably a lot of other broken links we just haven't noticed yet. Could you add both -Zrustdoc-map and the --extern-html-root-url=ena=... that you have now? That should take care of the vast majority of these issues, and then we can add more special cases as necessary.

src/bootstrap/doc.rs Outdated Show resolved Hide resolved
src/bootstrap/doc.rs Outdated Show resolved Hide resolved
src/bootstrap/doc.rs Outdated Show resolved Hide resolved
@jyn514
Copy link
Member

jyn514 commented Sep 12, 2021

Looks great, thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 12, 2021

📌 Commit 03f9fe2 has been approved by jyn514

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 12, 2021
Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 12, 2021
Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 12, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2021
…arth

Rollup of 7 pull requests

Successful merges:

 - rust-lang#88336 ( Detect stricter constraints on gats where clauses in impls vs trait)
 - rust-lang#88677 (rustc: Remove local variable IDs from `Export`s)
 - rust-lang#88699 (Remove extra unshallow from cherry-pick checker)
 - rust-lang#88709 (generic_const_exprs: use thir for abstract consts instead of mir)
 - rust-lang#88711 (Rework DepthFirstSearch API)
 - rust-lang#88810 (rustdoc: Cleanup `clean` part 1)
 - rust-lang#88813 (explicitly link to external `ena` docs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 146aee6 into rust-lang:master Sep 12, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 12, 2021
@lcnr lcnr deleted the ena-docs branch September 13, 2021 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants