crates.io: Add support for download URLs without placeholders #365
Conversation
|
side note: I've added a basic test case for the Fastly implementation, but the WASI target is making it somewhat difficult to test. if the target declarations in |
| let path = url.path(); | ||
|
|
||
| if let Some(captures) = RE.captures(path) { | ||
| let krate = captures.name("crate").unwrap().as_str(); |
There was a problem hiding this comment.
Unwrapping here seems dangerous. I'm not sure how much debugging information we'll get if we just fail hard. Instead, I think it is better to change the return type of the function to Result<(), Error> and handle the error gracefully. And if we log an error with error!, we'll get to see that in the service logs.
There was a problem hiding this comment.
hmm, can you think of a scenario where the unwrap would panic? the outer if let ensures that the regex matches and both of the captures are non-optional so I think unwrap is safe here for this regex 🤔
There was a problem hiding this comment.
That's a good argument. I don't think this can panic in its current form, but I'm also somewhat paranoid about changes in the future and other people assuming, based on this example, that it's okay to unwrap in this binary.
Turbo87
force-pushed
the
downloads-without-placeholders
branch
from
c35d876 to
f1c91c2
Compare
| let path = url.path(); | ||
|
|
||
| if let Some(captures) = RE.captures(path) { | ||
| let krate = captures.name("crate").unwrap().as_str(); |
There was a problem hiding this comment.
That's a good argument. I don't think this can panic in its current form, but I'm also somewhat paranoid about changes in the future and other people assuming, based on this example, that it's okay to unwrap in this binary.
|
This is now live in staging. 🚀 |
|
looks like it's working as intended. e.g. https://static.staging.crates.io/crates/crates-staging-test-tb/0.69.86/download |
|
Seems to work on staging, so I'll merge this and deploy to production. |
Turbo87
force-pushed
the
downloads-without-placeholders
branch
from
f1c91c2 to
456e3c7
Compare
Turbo87
force-pushed
the
downloads-without-placeholders
branch
from
456e3c7 to
2c6252e
Compare
|
We fixed a last issue with the use of |
In https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/placeholders.20in.20index.20config.2Ejson I asked what cargo version added support for placeholders in the
dlconfig field of the package index and apparently the answer is Rust 1.24.In the future the crates.io team would like to move crate file downloads directly to the CDN, instead of them having to go through our own backend, since that has proven to be a somewhat problematic bottleneck. To be able to support earlier cargo versions, this PR introduces URL rewriting for
/crates/:crate/:version/downloadrequests for the CloudFront and Fastly CDNs.This should allow us to change https://github.com/rust-lang/crates.io-index/blob/27423df2d3aecc0642d64eee2b321ffa1e6860ba/config.json#L2 to
"dl": "https://static.crates.io/crates"in the future.