Skip to content

Serve rustdoc static files from /-/rustdoc.static/ #1885

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

Merged
merged 3 commits into from
Oct 21, 2022

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Oct 17, 2022

Fixes #1869

This changes the --static-root-path we pass to rustdoc, so it generates HTML that looks for files in the new location. It also changes where the shared static files are uploaded to in storage. They are uploaded to a matching path (/-/rustdoc.static/).

Files under this path can be served from an ordinary route. After this change, the special logic in SharedResourceHandler (ignore request path, serve file from storage root) will only be needed when serving documentation built prior to this PR being deployed. As such, I've renamed SharedResourceHandler to LegacySharedResourceHandler, and named the new handler SharedResourceHandler.

Note: this does not depend on rust-lang/rust#101702.

@github-actions github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Oct 17, 2022
Copy link
Member

@syphar syphar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for pushing this!

I like renaming the old handler.

@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Oct 17, 2022
@syphar
Copy link
Member

syphar commented Oct 17, 2022

also, if you want to copy/paste some things: master...syphar:docs.rs:static-prefix

@jsha
Copy link
Contributor Author

jsha commented Oct 17, 2022

Thanks for the feedback! Pushed some work-in-progress changes. For some reason with the change to the RUSTDOC_STATIC_STORAGE_PREFIX, I'm getting 404s for the static files. Will debug later, but I figured y'all might want to take a look.

@jsha jsha force-pushed the static-new-path branch 2 times, most recently from 36e0ad8 to bb22a36 Compare October 19, 2022 06:35
@jsha
Copy link
Contributor Author

jsha commented Oct 19, 2022

Figured out the problem, rebased, and pushed a new revision. I think I've addressed all feedback.

@syphar
Copy link
Member

syphar commented Oct 19, 2022

@jsha it looks like the build & clippy is failing, could you check?

@jsha
Copy link
Contributor Author

jsha commented Oct 19, 2022

Fixed!

Copy link
Member

@syphar syphar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're nearly there! Thanks for sticking to this!

I'm missing tests for the new static handler, also checking the canonicalization.

Since my local build-process is still broken I would love to have another manual build& server test (@Nemo157 ?)

@jsha
Copy link
Contributor Author

jsha commented Oct 20, 2022

I wound up removing the code that tried to detect path traversal by canonicalizing paths. The code I copy-pasted from the docs.rs static assets path was using std::fs to canonicalize paths; that technique doesn't apply to S3.

And in fact according to this, S3 doesn't treat ../ specially anyhow, so I believe we don't need to worry about path traversal here.

@syphar
Copy link
Member

syphar commented Oct 20, 2022

I wound up removing the code that tried to detect path traversal by canonicalizing paths. The code I copy-pasted from the docs.rs static assets path was using std::fs to canonicalize paths; that technique doesn't apply to S3.
And in fact according to this, S3 doesn't treat ../ specially anyhow, so I believe we don't need to worry about path traversal here.

TIL, I didn't know that. Then you're right removing the piece.

@syphar syphar added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Oct 20, 2022
Copy link
Member

@syphar syphar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine from my side,

IMO this needs a second manual test ( build a crate, check if the docs can access their static files, probably even disabling the legacy handler so we're sure the new handler is used).

( I can't test builds right now on my machine, need to fix that at.. )

@Nemo157
Copy link
Member

Nemo157 commented Oct 20, 2022

I just tested https://docs.rs.dev.nemo157.com/cbor-diag/0.1.11/cbor_diag/ locally and confirmed that main-20221019-1.66.0-nightly-4b8f43199.js which it loads isn't available in the minio root, only under docsrs/rust-docs-rs/rustdoc-static/.

@jsha
Copy link
Contributor Author

jsha commented Oct 21, 2022

( I can't test builds right now on my machine, need to fix that at.. )

Any chance you're running into #1881 ?

@syphar
Copy link
Member

syphar commented Oct 21, 2022

( I can't test builds right now on my machine, need to fix that at.. )

Any chance you're running into #1881 ?

Thanks for the hint, but that's not it. It's because my computer is an M1 Mac, and the "run the build in the web container " doesn't work any more.

@syphar syphar merged commit 1702933 into rust-lang:master Oct 21, 2022
@github-actions github-actions bot added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Oct 21, 2022
@jsha jsha deleted the static-new-path branch October 24, 2022 08:23
@syphar syphar removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Nov 5, 2022
@syphar
Copy link
Member

syphar commented Nov 5, 2022

@jsha I just deployed the change and the first build uses the new path:
https://docs.rs/cozo/0.1.3/cozo/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change static root path
3 participants