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

migrate the main rustdoc handlers to axum, drop iron #1963

Merged
merged 2 commits into from
Dec 25, 2022

Conversation

syphar
Copy link
Member

@syphar syphar commented Dec 18, 2022

So, I couldn't make out a sensible intermediate step due to the overlapping routes between the rustdoc-redirector and -html-handler.

So this is all the rest of the handlers, and is dropping iron.

Some things to keep in mind:

  • I'm dropping the LegacySharedResourceHandler, replaced with a piece in rustdoc_redirector_handler which serves assets only from the root. From the answers in Docs built during 2018-{10-12} link to the wrong url for static resources #1181 I would say we close that one then.
  • I'm dropping my fix for double slashes ( fixes #1409: internal server error with two slashes at the end of the URL #1410 ), axum doesn't error here but just serves a normal 404 in these cases, which IMO is more correct. Of course I can re-add that functionality if needed.
  • The .ico handling is migrated to a simple redirect to our global favicon, which is what I think was the intention in the original ico_handler (f3848a3 is the original commit)
  • making the RenderingsTimeRecorder available in spawn_blocking when I call storage methods was difficult, so I disable measuring these steps for now, and will re-activate this when I migrate the storage to be async (which would be probably the next good step).

fixes

@github-actions github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Dec 18, 2022
src/web/mod.rs Show resolved Hide resolved
src/web/mod.rs Outdated Show resolved Hide resolved
@syphar syphar requested review from jsha, jyn514 and Nemo157 December 18, 2022 07:23
@syphar
Copy link
Member Author

syphar commented Dec 18, 2022

r? @Nemo157 @jyn514 @jsha

sorry for the mass-ping, but since all of you also touched this endpoint a help with reviewing would be awesome.

Also, even though there is some test coverage, some short manual tests with your local environments would be really helpful.

@syphar syphar force-pushed the axum-all-the-rest branch 3 times, most recently from 42348a7 to 9a59fd3 Compare December 19, 2022 09:55
Copy link
Member

@jyn514 jyn514 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 awesome ❤️ lots of small comments, but no blockers

src/storage/mod.rs Show resolved Hide resolved
src/web/rustdoc.rs Show resolved Hide resolved
src/web/rustdoc.rs Show resolved Hide resolved
src/web/rustdoc.rs Show resolved Hide resolved
src/web/rustdoc.rs Show resolved Hide resolved
src/web/rustdoc.rs Show resolved Hide resolved
src/web/rustdoc.rs Outdated Show resolved Hide resolved
@jyn514 jyn514 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 Dec 25, 2022
@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 Dec 25, 2022
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Just noting the changes in behavior before the conversations get resolved:

  • double slashes are now a 404 instead of a redirect (e.g. docs.rs//tokio)
  • a couple handlers that special cased JS over other files have been removed
  • we are going to lose timings from some of the database routes until we switch the codebase to async

I think it is somewhat likely this will introduce a regression by accident, but we can fix it after the fact, I expect it will be something small like the // routes actually being used.

Thank you for your work on this ❤️❤️❤️

@syphar
Copy link
Member Author

syphar commented Dec 25, 2022

Small correction : the double slash correction previously only handled double slashes at the end

@syphar
Copy link
Member Author

syphar commented Dec 25, 2022

This was merged into master as:

I'm still doing the merge for documentation.

@syphar syphar merged commit 503a808 into rust-lang:master Dec 25, 2022
@syphar syphar deleted the axum-all-the-rest branch December 25, 2022 19:44
@github-actions github-actions bot added 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 Dec 25, 2022
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.

2 participants