feat(libstore): curl-based s3#13752
Conversation
|
Would it be conceivable to just outright replace aws-sdk-cpp with this? I don't think we'd want to support it if we land this eventually. |
|
I was unsure about just removing the existing implementation outright, but I can do that if folks are OK with it? |
Can't speak for everyone, but I feel ripping out aws-sdk-cpp would be very welcome if the new thing is more lightweight and reuses the curl infrastructure with feature parity. That would definitely be better than supporting both implementations. |
|
@xokdvium I'm going to get this in a fully working state, and after I'm happy with it I'll tack on a final commit removing the old impl, so if there's tension we can discuss that point separately, or I can make it a separate PR, etc. I'm with you that if this works well, we're better off just tossing the old impl, though that would mean folks building with old curl would lose the ability to talk to s3, but idk if we care to support people building non-standard configurations like that. |
cd41165 to
1046367
Compare
2030275 to
86873ed
Compare
|
I want to definitely test this with our hydra staging instance to ensure it still can upload stuff to s3 buckets before merging. |
|
AFAICT this is ready for (very) preliminary testing; at least I was able to pull and push to an S3 store and auth just worked, like magic :) |
|
Nevermind, I messed something up, buckets outside of us-east-1 don't work :P |
aefcffd to
3433232
Compare
2dd60ad to
80e3327
Compare
xokdvium
left a comment
There was a problem hiding this comment.
Some preliminary comments
80e3327 to
3333622
Compare
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This commit replaces the AWS C++ SDK with a lighter curl-based approach for S3 binary cache operations. - Removed dependency on the heavy aws-cpp-sdk-s3 and aws-cpp-sdk-transfer - Added lightweight aws-crt-cpp for credential resolution only - Leverages curl's native AWS SigV4 authentication (requires curl >= 7.75.0) - S3BinaryCacheStore now delegates to HttpBinaryCacheStore - Function s3ToHttpsUrl converts ParsedS3URL to ParsedURL - Multipart uploads are no longer supported (may be reimplemented later) - Build now requires curl >= 7.75.0 for AWS SigV4 support Fixes: NixOS#13084, NixOS#12671, NixOS#11748, NixOS#12403, NixOS#5947
Now that the legacy S3 implementation is gone, we can go back to calling things `NIX_WITH_S3_SUPPORT`.
Move S3 URL parsing, store configuration, and public bucket support outside of NIX_WITH_S3_SUPPORT guards. Only AWS credential resolution remains gated, allowing builds with withAWS = false to: - Parse s3:// URLs - Register S3 store types - Access public S3 buckets (via HTTPS conversion) - Use S3-compatible services without authentication The setupForS3() function now always performs URL conversion, with authentication code conditionally compiled based on NIX_WITH_S3_SUPPORT. The aws-creds.cc file (only code using AWS CRT SDK) is now conditionally compiled by meson.
The macro now accurately reflects its purpose: gating only AWS authentication code, not all S3 functionality. S3 URL parsing, store configuration, and public bucket access work regardless of this flag. This rename clarifies that: - S3 support is always available (URL parsing, store registration) - Only AWS credential resolution requires the flag - The flag controls AWS CRT SDK dependency, not S3 protocol support
|
|
|
@lovesegfault the prose doesn't seem to appear in the manual anymore. Could you look into that? |
Motivation
#13084
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.