-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Rustdoc copy local img #68734
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
Rustdoc copy local img #68734
Conversation
@GuillaumeGomez While the approach of hashing the URL is cheap, it's also a bit risky. Consider a library which has some image it uses from a lot of places to illustrate things. Those places may all have subtly different paths to the same object (different numbers of |
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 |
@kinnison This is a very good point. Maybe storing a checksum instead maybe and use it as name? What do you think? |
30e1937
to
39659a1
Compare
I realized that we could get the full path from a path and therefore generate a same hash every time. |
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 |
If you've resolved the filename to a canonical path before hashing then that sounds fine to me. It will make it harder to match exact hashes in your test though. |
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.
I'm good with the approach, but I think the test will need rethinking.
// @has static/8a40d4987fbb905 | ||
// @has foo/struct.Enum.html | ||
// @has - '//img[@src="../static/8a40d4987fbb905"]' '' |
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.
I fear these hashes won't be consistent between systems now that the filename is canonicalised. Is there a cut-down test which will still work here?
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.
Oh, didn't think about that, the hash will fail indeed. Great catch! I can add something to count files in the given folder and add a "start with" thing. Unless you have another idea?
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.
One option could indeed be that. Another -- hash the file as part of the deduplication effort, but then simply number them as you add them to the static/
dir?
There will need to be some way to explicitly opt-in to this feature. As it stands this could break the docs for people already using relative paths to images. Additionally missing files should be errors. The main issue I see with this implementation is that it resolves paths to the images relative to rustdoc's current directory. Even if that were changed to be relative to the source file, it can't be rustdoc which actually loads the file because rustdoc doesn't have access to the source files of external crates so cross-crate inlining won't work. I think this will have to be implemented similarly to |
Triaged |
☔ The latest upstream changes (presumably #70499) made this pull request unmergeable. Please resolve the merge conflicts. |
Closing this due to inactivity. |
Fixes #32104.
r? @kinnison