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

cargo doc no longer generates a nav list of all dependencies in the sidebar #85301

Closed
Evrey opened this issue May 14, 2021 · 11 comments · Fixed by #85304
Closed

cargo doc no longer generates a nav list of all dependencies in the sidebar #85301

Evrey opened this issue May 14, 2021 · 11 comments · Fixed by #85304
Labels
A-rustdoc-js Area: Rustdoc's JS front-end C-bug Category: This is a bug. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@Evrey
Copy link

Evrey commented May 14, 2021

Since i don't know which version, when running cargo doc on a project, the left sidebar of the docs no longer has a nav for all crates in the dependency graph. However, the documentation for all dependencies is still compiled.

Steps to reproduce

  1. Pick a Rust project with a few dependencies.
  2. Run cargo doc --open
  3. Expected: Left sidebar has a list of all dependencies, linking to each crate's local docs.
  4. Got: No such thing.

Version it worked on

It most recently worked on: I honestly don't remember. I update my nightly before every dev session. Allegedly that bug is 27 days old.

Version with regression

rustc --version --verbose:

rustc 1.54.0-nightly (ca82264ec 2021-05-09)
binary: rustc
commit-hash: ca82264ec7556a6011b9d3f1b2fd4c7cd0bc8ae2
commit-date: 2021-05-09
host: x86_64-unknown-linux-gnu
release: 1.54.0-nightly
LLVM version: 12.0.1
@Evrey Evrey added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels May 14, 2021
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-untriaged Untriaged performance or correctness regression. labels May 14, 2021
@Nemo157 Nemo157 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-js Area: Rustdoc's JS front-end labels May 14, 2021
@Nemo157
Copy link
Member

Nemo157 commented May 14, 2021

Appears to be a regression from #84150, the call to addSidebarCrates was moved into initSidebarItems, but this function is not called on the crate-root doc page. I tested locally that it fails on nightly-2021-04-18 and works on nightly-2021-04-17.

@Nemo157 Nemo157 added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label May 14, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 14, 2021
@Nemo157
Copy link
Member

Nemo157 commented May 14, 2021

Checking https://doc.rust-lang.org/beta/std/ it looks like this has made it onto the 1.53 beta.

@jyn514
Copy link
Member

jyn514 commented May 14, 2021

cc @jsha

@jsha
Copy link
Contributor

jsha commented May 14, 2021

I think the ideal fix is around

if parentlen == 0 {
// There is no sidebar-items.js beyond the crate root path
// FIXME maybe dynamic crate loading can be merged here
} else {
write!(buffer, "<script defer src=\"{path}sidebar-items.js\"></script>", path = relpath);
}
. We should emit a sidebar-items.js for the top page that just calls initSidebarItems({}). And remove that comment, since we now have dynamic crate loading.

A quicker fix is to just move the call to addSidebarCrates so it gets called independently from initSidebarItems. I like that less since the crates really are a sidebar item and should be treated as such.

@Stupremee
Copy link
Member

I can do that if no one else wants to do it!

@jsha
Copy link
Contributor

jsha commented May 14, 2021

Go for it! I'm happy to review.

@LeSeulArtichaut LeSeulArtichaut added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels May 15, 2021
@LeSeulArtichaut
Copy link
Contributor

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented May 15, 2021

It's actually something I fixed in #84834 (still waiting for approval/review).

@jsha
Copy link
Contributor

jsha commented May 15, 2021

I propose we plan on landing and backporting #85304 to fix the regression: it's much smaller and limited to the specific fix. Then we can work on landing #84834 in nightly.

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented May 15, 2021

No, the approach taken in #85304 is problematic. The information is already there and we don't need this new file.

EDIT: also, #84834 is ready, there is a debate for the other part of the PR (the change on module's sidebar) which I can move to another PR.

@jsha
Copy link
Contributor

jsha commented May 16, 2021

See #85304 (comment) - after some discussion, @GuillaumeGomez and @Nemo157 concluded to merge #85304 as our quick fix.

To explain why I proposed to add a tiny sidebar-item.js: We have one main.js that's loaded by a variety of different pages. It also potentially has many different entry points. For instance, the primary entry point is that when main.js itself is loaded, it runs various functions, adding event handlers and so on. Another entry point is that when sidebar-items.js loads, it calls initSidebarItems. That's the root cause of this bug - code in main.js expected initSidebarItems would get called, and for a subset of pages it wasn't.

It's a bit inelegant to have a small file whose purpose is only to call a known function. But I think that inelegance is balanced against the improved maintainability of having most doc pages follow closer to the same code path through main.js. The more code paths, the more chances for bugs and the more complexity in testing.

Another way to address this longer-term would be to eliminate the sidebar-items.js entrypoint entirely. Instead, when main.js runs, it would trigger the load of sidebar-items and run any callbacks based on that load.

Yet another way to address this would be to separate out addSidebarCrates from initSidebarItems, so that addSidebarCrates is called unconditionally for all pages.

@bors bors closed this as completed in 94ecdfd May 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-js Area: Rustdoc's JS front-end C-bug Category: This is a bug. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants