-
Notifications
You must be signed in to change notification settings - Fork 210
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
Don't build default target twice #534
Conversation
56e9a4b
to
18b57e5
Compare
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 still need to test building crates locally, but this looks great!
The only change I'd like to see is on the dropdown menu allowing you to choose the target: with this PR, if you click the default target you'll be sent first to /:crate/:version/:default-target
, and then redirected to /:crate/:version
. I'd be nice to remove that redirect.
Build code also seem to work fine! |
I'm not sure how to fix this. Currently we set those links like this in
We'd have to compare each |
The code as of the previous commit would let you navigate away from the default, but not navigate back. This adds the default target to `successful_targets` to add a link to the dropdown in 'Platform'. After adding that, the link would give a 404 (because we treat paths literally when sending them to the database), so the metadata for every crate now has to include the default target, which allows redirecting to /:crate/:version/:module/ when visiting /:crate/:version/:module/:default-target
this required a bit of a rearchitecture
74848d7
to
e2d5a0f
Compare
also uses a constant instead of hardcoding the default target
Only the default build should be in target/doc.
I'd generate a |
I don't see a way to send a data structure to a Page, we'd have to make it part of CrateDetails. Is it ok if we add this at a later point? It's not hurting usability, you still get sent to the right page. |
Yep.
Ok, can you open an issue about that? |
Opened: #549 |
This is the non-breaking changes from #532. I also kept the additional documentation for existing metadata.
Copy/pasted from that PR for convenience:
Architecture Changes
This removes the duplicated 'default' build which is run twice.
--target
to Cargo for the common case wheredefault-target
is not set. This will fix Cfg missing during build script build #440. As a side effect, it removes most of the hacking around I had to do to get proc macros to work.x86_64-unknown-linux-gnu
and can be overriden withdefault-target = ...
in Cargo.toml/:crate/:version/:default-target
to/:crate/:version/
, keeping the subpage if it exists.This has been tested both by building with the new code and by building with the old code and running
cratesfyi database migrate
. Note that ifcratesfyi database migrate
is not run, the web server will panic whenever viewing a crate built with the old code.r? @pietroalbini