-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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 index page argument #54543
Add index page argument #54543
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
741050c
to
e585b25
Compare
@Mark-Simulacrum Need help when you have a bit of time. I tried to pass the |
Yes, I plan to take a look at this tomorrow I hope, probably won't get a chance to tonight. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
f3d07a9
to
f30b6ea
Compare
It's now ready! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a screenshot of what the index page looks like if you don't provide the --index-page
argument?
Can you update the Rustdoc Book with the new CLI flags you added?
Yes and yes. |
@QuietMisdreavus What remains to be done here btw? The books are generated with the |
I don't see an update to either of these? EDIT: When i say "update the Rustdoc Book", i mean "write new docs for the book talking about the new flags". |
Now I get it! What remains to be done (reminder for me):
|
28c6582
to
a80d613
Compare
Updated. |
@QuietMisdreavus Anything else? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the CLI flag enable-index-page
, but the Context field disable_index_page
? It creates confusion when looking through the code if the CLI flag is the opposite of how the code looks at it.
(GitHub just released the ability for us to suggest code changes as a potential patch, so i'm going to test it out here, haha!)
src/librustdoc/html/render.rs
Outdated
@@ -106,6 +109,8 @@ struct Context { | |||
/// The map used to ensure all generated 'id=' attributes are unique. | |||
id_map: Rc<RefCell<IdMap>>, | |||
pub shared: Arc<SharedContext>, | |||
pub disable_index_page: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub disable_index_page: bool, | |
pub enable_index_page: bool, |
src/librustdoc/html/render.rs
Outdated
@@ -501,7 +506,12 @@ pub fn run(mut krate: clean::Crate, | |||
sort_modules_alphabetically: bool, | |||
themes: Vec<PathBuf>, | |||
enable_minification: bool, | |||
id_map: IdMap) -> Result<(), Error> { | |||
id_map: IdMap, | |||
disable_index_page: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disable_index_page: bool, | |
enable_index_page: bool, |
src/librustdoc/html/render.rs
Outdated
@@ -572,6 +582,8 @@ pub fn run(mut krate: clean::Crate, | |||
codes: ErrorCodes::from(UnstableFeatures::from_environment().is_nightly_build()), | |||
id_map: Rc::new(RefCell::new(id_map)), | |||
shared: Arc::new(scx), | |||
disable_index_page, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disable_index_page, | |
enable_index_page, |
src/librustdoc/html/render.rs
Outdated
@@ -969,6 +990,42 @@ themePicker.onblur = handleThemeButtonsBlur; | |||
} | |||
try_err!(writeln!(&mut w, "initSearch(searchIndex);"), &dst); | |||
|
|||
if cx.disable_index_page == false { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if cx.disable_index_page == false { | |
if cx.enable_index_page { |
src/librustdoc/lib.rs
Outdated
@@ -580,7 +599,10 @@ fn main_args(args: &[String]) -> isize { | |||
renderinfo, | |||
sort_modules_alphabetically, | |||
themes, | |||
enable_minification, id_map) | |||
enable_minification, id_map, | |||
!enable_index_page, index_page, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!enable_index_page, index_page, | |
enable_index_page, index_page, |
Oh one more thing: Can you add a test? It should just work with the regular rustdoc tests if you add an auxiliary crate to the build. |
a80d613
to
91bba19
Compare
Updated. |
@@ -389,3 +389,16 @@ pub struct BigX; | |||
|
|||
Then, when looking for it through the `rustdoc` search, if you enter "x" or | |||
"big", search will show the `BigX` struct first. | |||
|
|||
### `index-page` feature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these are CLI flags, they should follow the convention of the other flags' headings:
### `--index-page`: provide a top-level landing page for docs
### `--enable-index-page`: generate a default index page for docs
(The section about doc_alias
is also in the wrong place, but i failed to catch that when you wrote it in the first place... >_>
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. I'll fix all that at once then.
91bba19
to
2b64605
Compare
Updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much!
@bors r+ |
📌 Commit 2b64605 has been approved by |
Add index page argument @Mark-Simulacrum: I might need some help from you: in bootstrap, I want to add an argument (a new flag added into `rustdoc`) in order to generate the current index directly when `rustdoc` is documenting the `std` lib. However, my change in `bootstrap` didn't do it and I assume it must be moved inside the `Std` struct. But there, I don't see how to pass it to `rustdoc` through `cargo`. Did I miss anything? r? @QuietMisdreavus
☀️ Test successful - status-appveyor, status-travis |
…acrum Use local links in the alloc docs. Links to other crates (like core) from the alloc crate were incorrectly using the `https://doc.rust-lang.org/nightly/` absolute (remote) links, instead of relative (local) links. For example, the link to `Result` at https://doc.rust-lang.org/1.44.1/alloc/vec/struct.Vec.html#method.try_reserve goes to /nightly/. This is because alloc was being documented before core, and rustdoc relies on the existence of the local directory to know if it should use a local or remote link. There was code that tried to compensate for this (`create_dir_all`), but in rust-lang#54543 it was broken because instead of running `cargo doc` once for all the crates, it was changed to run `cargo rustdoc` for each crate individually. This means that `create_dir_all` was no longer doing what it was supposed to be doing (creating all the directories before starting). The solution here is to just build in the correct order (from the dependency leaves towards the root). An alternate solution would be to switch back to running `cargo doc` once (and use RUSTDOCFLAGS for passing in flags). Another alternate solution would be to iterate over the list twice, creating the directories during the first pass. I also did a little cleanup to remove the "crate-docs" directory. This was added in the past because different crates were built in different directories. Over time, things have been unified (and rustc docs no longer include std), so it is no longer necessary.
…acrum Use local links in the alloc docs. Links to other crates (like core) from the alloc crate were incorrectly using the `https://doc.rust-lang.org/nightly/` absolute (remote) links, instead of relative (local) links. For example, the link to `Result` at https://doc.rust-lang.org/1.44.1/alloc/vec/struct.Vec.html#method.try_reserve goes to /nightly/. This is because alloc was being documented before core, and rustdoc relies on the existence of the local directory to know if it should use a local or remote link. There was code that tried to compensate for this (`create_dir_all`), but in rust-lang#54543 it was broken because instead of running `cargo doc` once for all the crates, it was changed to run `cargo rustdoc` for each crate individually. This means that `create_dir_all` was no longer doing what it was supposed to be doing (creating all the directories before starting). The solution here is to just build in the correct order (from the dependency leaves towards the root). An alternate solution would be to switch back to running `cargo doc` once (and use RUSTDOCFLAGS for passing in flags). Another alternate solution would be to iterate over the list twice, creating the directories during the first pass. I also did a little cleanup to remove the "crate-docs" directory. This was added in the past because different crates were built in different directories. Over time, things have been unified (and rustc docs no longer include std), so it is no longer necessary.
@Mark-Simulacrum: I might need some help from you: in bootstrap, I want to add an argument (a new flag added into
rustdoc
) in order to generate the current index directly whenrustdoc
is documenting thestd
lib. However, my change inbootstrap
didn't do it and I assume it must be moved inside theStd
struct. But there, I don't see how to pass it torustdoc
throughcargo
. Did I miss anything?r? @QuietMisdreavus