rustdoc: Add support for local resources #107640

GuillaumeGomez · 2023-02-03T16:22:55Z

This PR adds the support for "local resources". What it means exactly: if a local item is referenced in the markdown, rustdoc will copy it into {LOCAL_RESOURCES_FOLDER_NAME}/{crate_name} and then refer to it.

To do so, I originally tried to do it directly when we generated the markdown, but I ended with a pretty bad code. You can see this (unfinished) version here.

So how is it implemented? First, we go through a new pass which will gather the local resources from the markdown and store them into two maps: one map to keep the correspondance between the item and its "local location", and the other contains all the stored items to prevent duplications. This second is also used to copy the files into the static folder once everything has been generated.

Then, when we generate the markdown content, we replace the links to these items if we can find them in the correspondance map.

Best review it commit by commit.

RFC: rust-lang/rfcs#3397
r? @notriddle

GuillaumeGomez · 2023-02-03T17:05:48Z

Forgot to update librustdoc inner tests. Should be solved now.

notriddle

This should also go through an RFC, since it's adding a new API to rustdoc (like intra-doc links did).

notriddle · 2023-02-03T19:31:12Z

src/librustdoc/passes/collect_local_resources.rs

+                    self.cx.tcx.sess.struct_span_err(
+                        item.attr_span(self.cx.tcx),
+                        &format!("`{ori_path}`: No such file"),
+                    ).emit();


This should probably emit a warning, at least for awhile, because someone could've made a home-grown build system that already did the resource copying.

Just a warning forever should be enough indeed.

notriddle · 2023-02-03T19:31:41Z

src/librustdoc/passes/collect_local_resources.rs

+            if !path.is_file() {
+                self.cx.tcx.sess.struct_span_err(
+                    item.attr_span(self.cx.tcx),
+                    &format!("`{ori_path}`: No such file (expanded into `{}`)", path.display()),


This should probably emit a warning, at least for awhile, because someone could've made a home-grown build system that already did the resource copying.

notriddle · 2023-02-03T19:32:53Z

src/librustdoc/html/markdown.rs

+    pub(crate) error_codes: ErrorCodes,
+    pub(crate) edition: Edition,
+    pub(crate) playground: &'a Option<Playground>,
+}


Because I'm afraid that this PR might take awhile to approve, can you please separate 62e7207 into its own PR?

notriddle · 2023-02-03T19:45:16Z

tests/rustdoc/local-resources.rs

+
+// @has local_resources/foo/0.svg
+// @has foo/struct.Enum.html
+// @has - '//img[@src="../local_resources/foo/0.svg"]' ''


Can we please change this so that it attempts to preserve, at least partially, the original filename?

Pure numbers are annoying for anyone who wants to download the file (right-click, Save As) or image search engines that use the filename to guess at the subject matter.

Even if everything is sorted so that the numbering is technically deterministic, it still means minor changes to the docs can cause images to swap filenames. This will be very confusing when these URLs are shared elsewhere.

It would be better if this filename had a file hash in it, and local_resources ought to be named local.files, for consistency with static files and so that the CDN can be configured the same way.

The problem with keeping the original filename is that we can potentially end up with multiple files with the same name in the static files folder. I suppose it'll be debated in the RFC.

Add a hash to the file name. If two files have the same name and the same hash, then that’s not actually a problem.

Currently the key is the absolute file path, which is how we know if a file was already linked to.

notriddle · 2023-02-03T19:54:44Z

tests/rustdoc/local-resources.rs

+    // @has - '//img[@src="../../local_resources/foo/0.svg"]' ''
+    /// test!
+    ///
+    /// ![yep](../../src/librustdoc/html/static/images/rust-logo.svg)


Please also add a test case that combines this feature with cross-crate inlining?

…c-into-struct, r=notriddle Turn MarkdownWithToc into a struct with named fields Extracted the commit from rust-lang#107640. r? `@notriddle`

bors · 2023-02-07T00:14:20Z

☔ The latest upstream changes (presumably #107738) made this pull request unmergeable. Please resolve the merge conflicts.

jsha · 2023-02-14T01:38:45Z

src/librustdoc/formats/cache.rs

+    /// This will be used when generating the HTML, once everything is generated, we copy these
+    /// files into the static folder.


We shouldn't put per-crate files into static.files/, which is used for toolchain-specific static files. If we go this route, there should be a separate directory for invocation-specific files.

Absolutely. The RFC mentions that there won't be a crate folder. ;)

GuillaumeGomez · 2023-03-16T12:57:17Z

Opened RFC: rust-lang/rfcs#3397

anden3 · 2023-05-14T14:24:06Z

Hello @GuillaumeGomez! Noticed this PR has some merge conflicts, other than that it looks like it's waiting on the RFC :)

GuillaumeGomez · 2023-05-14T19:19:18Z

I added the blocked label.

GuillaumeGomez added 6 commits February 3, 2023 17:14

Copy local resources into doc generated folder

d23042d

Turn MarkdownWithToc into a struct with named fields

62e7207

Integrate LocalResources into markdown generation

3d28a2d

Add test for local resources

618edd1

Update rustdoc-ui test output

6713e68

Add rustdoc UI test for not found local resources

15f3fc6

rustbot assigned notriddle Feb 3, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Feb 3, 2023

Update librustdoc inner tests

bd298a3

notriddle reviewed Feb 3, 2023

View reviewed changes

GuillaumeGomez mentioned this pull request Feb 6, 2023

Turn MarkdownWithToc into a struct with named fields #107725

Merged

notriddle mentioned this pull request Feb 6, 2023

Create RFC for bundling local images in rustdoc output GuillaumeGomez/rfcs#3

Closed

jsha reviewed Feb 14, 2023

View reviewed changes

GuillaumeGomez added the needs-rfc This change is large or controversial enough that it should have an RFC accepted before doing it. label Feb 14, 2023

anden3 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 May 14, 2023

GuillaumeGomez added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label May 14, 2023

Dylan-DPC removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: Add support for local resources #107640

rustdoc: Add support for local resources #107640

GuillaumeGomez commented Feb 3, 2023 •

edited by Dylan-DPC

Loading

GuillaumeGomez commented Feb 3, 2023

notriddle left a comment

notriddle Feb 3, 2023

GuillaumeGomez Feb 6, 2023

notriddle Feb 3, 2023

notriddle Feb 3, 2023

GuillaumeGomez Feb 6, 2023

notriddle Feb 3, 2023

GuillaumeGomez Feb 6, 2023

notriddle Feb 6, 2023

GuillaumeGomez Feb 6, 2023

notriddle Feb 3, 2023

bors commented Feb 7, 2023

jsha Feb 14, 2023

GuillaumeGomez Feb 14, 2023

GuillaumeGomez commented Mar 16, 2023

anden3 commented May 14, 2023

GuillaumeGomez commented May 14, 2023

		/// This will be used when generating the HTML, once everything is generated, we copy these
		/// files into the static folder.

rustdoc: Add support for local resources #107640

Are you sure you want to change the base?

rustdoc: Add support for local resources #107640

Conversation

GuillaumeGomez commented Feb 3, 2023 • edited by Dylan-DPC Loading

GuillaumeGomez commented Feb 3, 2023

notriddle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Feb 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Mar 16, 2023

anden3 commented May 14, 2023

GuillaumeGomez commented May 14, 2023

GuillaumeGomez commented Feb 3, 2023 •

edited by Dylan-DPC

Loading