Skip to content

Commit d2b6499

Browse files
authored
Merge pull request #13752 from lovesegfault/curl-based-s3-v2
feat(libstore): curl-based s3
2 parents e3232af + e069c98 commit d2b6499

26 files changed

+91
-1082
lines changed

.github/workflows/ci.yml

Lines changed: 2 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,6 @@ jobs:
6868
primary: true
6969
stdenv: stdenv
7070
withAWS: true
71-
withCurlS3: false
7271
# TODO: remove once curl-based-s3 fully lands
7372
- scenario: on ubuntu (no s3)
7473
runs-on: ubuntu-24.04
@@ -77,32 +76,20 @@ jobs:
7776
primary: false
7877
stdenv: stdenv
7978
withAWS: false
80-
withCurlS3: false
81-
# TODO: remove once curl-based-s3 fully lands
82-
- scenario: on ubuntu (curl s3)
83-
runs-on: ubuntu-24.04
84-
os: linux
85-
instrumented: false
86-
primary: false
87-
stdenv: stdenv
88-
withAWS: false
89-
withCurlS3: true
9079
- scenario: on macos
9180
runs-on: macos-14
9281
os: darwin
9382
instrumented: false
9483
primary: true
9584
stdenv: stdenv
9685
withAWS: true
97-
withCurlS3: false
9886
- scenario: on ubuntu (with sanitizers / coverage)
9987
runs-on: ubuntu-24.04
10088
os: linux
10189
instrumented: true
10290
primary: false
10391
stdenv: clangStdenv
10492
withAWS: true
105-
withCurlS3: false
10693
name: tests ${{ matrix.scenario }}
10794
runs-on: ${{ matrix.runs-on }}
10895
timeout-minutes: 60
@@ -126,15 +113,13 @@ jobs:
126113
nix build --file ci/gha/tests/wrapper.nix componentTests -L \
127114
--arg withInstrumentation ${{ matrix.instrumented }} \
128115
--argstr stdenv "${{ matrix.stdenv }}" \
129-
${{ format('--arg withAWS {0}', matrix.withAWS) }} \
130-
${{ format('--arg withCurlS3 {0}', matrix.withCurlS3) }}
116+
${{ format('--arg withAWS {0}', matrix.withAWS) }}
131117
- name: Run VM tests
132118
run: |
133119
nix build --file ci/gha/tests/wrapper.nix vmTests -L \
134120
--arg withInstrumentation ${{ matrix.instrumented }} \
135121
--argstr stdenv "${{ matrix.stdenv }}" \
136-
${{ format('--arg withAWS {0}', matrix.withAWS) }} \
137-
${{ format('--arg withCurlS3 {0}', matrix.withCurlS3) }}
122+
${{ format('--arg withAWS {0}', matrix.withAWS) }}
138123
if: ${{ matrix.os == 'linux' }}
139124
- name: Run flake checks and prepare the installer tarball
140125
run: |
@@ -147,7 +132,6 @@ jobs:
147132
--arg withInstrumentation ${{ matrix.instrumented }} \
148133
--argstr stdenv "${{ matrix.stdenv }}" \
149134
${{ format('--arg withAWS {0}', matrix.withAWS) }} \
150-
${{ format('--arg withCurlS3 {0}', matrix.withCurlS3) }} \
151135
--out-link coverage-reports
152136
cat coverage-reports/index.txt >> $GITHUB_STEP_SUMMARY
153137
if: ${{ matrix.instrumented }}

ci/gha/tests/default.nix

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313
withSanitizers ? false,
1414
withCoverage ? false,
1515
withAWS ? null,
16-
withCurlS3 ? null,
1716
...
1817
}:
1918

@@ -59,10 +58,7 @@ rec {
5958
nix-expr = prev.nix-expr.override { enableGC = !withSanitizers; };
6059

6160
# Override AWS configuration if specified
62-
nix-store = prev.nix-store.override (
63-
lib.optionalAttrs (withAWS != null) { inherit withAWS; }
64-
// lib.optionalAttrs (withCurlS3 != null) { inherit withCurlS3; }
65-
);
61+
nix-store = prev.nix-store.override (lib.optionalAttrs (withAWS != null) { inherit withAWS; });
6662

6763
mesonComponentOverrides = lib.composeManyExtensions componentOverrides;
6864
# Unclear how to make Perl bindings work with a dynamically linked ASAN.
@@ -231,12 +227,7 @@ rec {
231227

232228
vmTests = {
233229
}
234-
# FIXME: when the curlS3 implementation is complete, it should also enable these tests.
235230
// lib.optionalAttrs (withAWS == true) {
236-
# S3 binary cache store test only runs when S3 support is enabled
237-
inherit (nixosTests) s3-binary-cache-store;
238-
}
239-
// lib.optionalAttrs (withCurlS3 == true) {
240231
# S3 binary cache store test using curl implementation
241232
inherit (nixosTests) curl-s3-binary-cache-store;
242233
}

ci/gha/tests/wrapper.nix

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,13 @@
66
componentTestsPrefix ? "",
77
withInstrumentation ? false,
88
withAWS ? null,
9-
withCurlS3 ? null,
109
}@args:
1110
import ./. (
1211
args
1312
// {
1413
getStdenv = p: p.${stdenv};
1514
withSanitizers = withInstrumentation;
1615
withCoverage = withInstrumentation;
17-
inherit withAWS withCurlS3;
16+
inherit withAWS;
1817
}
1918
)
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
---
2+
synopsis: "Improved S3 binary cache support via HTTP"
3+
prs: [13823, 14026, 14120, 14131, 14135, 14144, 14170, 14190, 14198, 14206, 14209, 14222, 14223, 13752]
4+
issues: [13084, 12671, 11748, 12403]
5+
---
6+
7+
S3 binary cache operations now happen via HTTP, leveraging `libcurl`'s native
8+
AWS SigV4 authentication instead of the AWS C++ SDK, providing significant
9+
improvements:
10+
11+
- **Reduced memory usage**: Eliminates memory buffering issues that caused
12+
segfaults with large files
13+
- **Fixed upload reliability**: Resolves AWS SDK chunking errors
14+
(`InvalidChunkSizeError`)
15+
- **Lighter dependencies**: Uses lightweight `aws-crt-cpp` instead of full
16+
`aws-cpp-sdk`, reducing build complexity
17+
18+
The new implementation requires curl >= 7.75.0 and `aws-crt-cpp` for credential
19+
management.
20+
21+
All existing S3 URL formats and parameters remain supported, with the notable
22+
exception of multi-part uploads, which are no longer supported.
23+
24+
Note that this change also means Nix now supports S3 binary cache stores even
25+
if build without `aws-crt-cpp`, but only for public buckets which do not
26+
require auth.

packaging/components.nix

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -490,7 +490,7 @@ in
490490
491491
Example:
492492
```
493-
overrideScope (finalScope: prevScope: { aws-sdk-cpp = null; })
493+
overrideScope (finalScope: prevScope: { aws-crt-cpp = null; })
494494
```
495495
*/
496496
overrideScope = f: (scope.overrideScope f).nix-everything;

packaging/dependencies.nix

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -16,21 +16,6 @@ in
1616
scope: {
1717
inherit stdenv;
1818

19-
aws-sdk-cpp =
20-
(pkgs.aws-sdk-cpp.override {
21-
apis = [
22-
"identity-management"
23-
"s3"
24-
"transfer"
25-
];
26-
customMemoryManagement = false;
27-
}).overrideAttrs
28-
{
29-
# only a stripped down version is built, which takes a lot less resources
30-
# to build, so we don't need a "big-parallel" machine.
31-
requiredSystemFeatures = [ ];
32-
};
33-
3419
boehmgc =
3520
(pkgs.boehmgc.override {
3621
enableLargeConfig = true;

src/libstore-tests/s3-binary-cache-store.cc

Lines changed: 4 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,9 @@
11
#include "nix/store/s3-binary-cache-store.hh"
2+
#include "nix/store/http-binary-cache-store.hh"
3+
#include "nix/store/filetransfer.hh"
4+
#include "nix/store/s3-url.hh"
25

3-
#if NIX_WITH_S3_SUPPORT
4-
5-
# include <gtest/gtest.h>
6-
7-
namespace nix {
8-
9-
TEST(S3BinaryCacheStore, constructConfig)
10-
{
11-
S3BinaryCacheStoreConfig config{"s3", "foobar", {}};
12-
13-
EXPECT_EQ(config.bucketName, "foobar");
14-
}
15-
16-
} // namespace nix
17-
18-
#elif NIX_WITH_CURL_S3
19-
20-
# include "nix/store/http-binary-cache-store.hh"
21-
# include "nix/store/filetransfer.hh"
22-
# include "nix/store/s3-url.hh"
23-
24-
# include <gtest/gtest.h>
6+
#include <gtest/gtest.h>
257

268
namespace nix {
279

@@ -141,5 +123,3 @@ TEST(S3BinaryCacheStore, parameterFiltering)
141123
}
142124

143125
} // namespace nix
144-
145-
#endif

src/libstore-tests/s3-url.cc

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,8 @@
11
#include "nix/store/s3-url.hh"
22
#include "nix/util/tests/gmock-matchers.hh"
33

4-
#if NIX_WITH_S3_SUPPORT || NIX_WITH_CURL_S3
5-
6-
# include <gtest/gtest.h>
7-
# include <gmock/gmock.h>
4+
#include <gtest/gtest.h>
5+
#include <gmock/gmock.h>
86

97
namespace nix {
108

@@ -228,5 +226,3 @@ INSTANTIATE_TEST_SUITE_P(
228226
[](const ::testing::TestParamInfo<S3ToHttpsConversionTestCase> & info) { return info.param.description; });
229227

230228
} // namespace nix
231-
232-
#endif

src/libstore/aws-creds.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
#include "nix/store/aws-creds.hh"
22

3-
#if NIX_WITH_CURL_S3
3+
#if NIX_WITH_AWS_AUTH
44

55
# include <aws/crt/Types.h>
66
# include "nix/store/s3-url.hh"

src/libstore/builtins/fetchurl.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ static void builtinFetchurl(const BuiltinBuilderContext & ctx)
4141
FileTransferRequest request(VerbatimURL{url});
4242
request.decompress = false;
4343

44-
#if NIX_WITH_CURL_S3
44+
#if NIX_WITH_AWS_AUTH
4545
// Use pre-resolved credentials if available
4646
if (ctx.awsCredentials && request.uri.scheme() == "s3") {
4747
debug("[pid=%d] Using pre-resolved AWS credentials from parent process", getpid());

0 commit comments

Comments
 (0)