Skip to content

refactor(libstore): pass headers into upload methods#14477

Merged
Ericson2314 merged 1 commit into
NixOS:masterfrom
lovesegfault:http-upload-headers
Nov 7, 2025
Merged

refactor(libstore): pass headers into upload methods#14477
Ericson2314 merged 1 commit into
NixOS:masterfrom
lovesegfault:http-upload-headers

Conversation

@lovesegfault
Copy link
Copy Markdown
Member

Motivation

This will make it easier to attach additional headers (e.g. storage
class) on the s3 side of things and makes Content-Encoding less
special.

I also got rid of the override for CompressedSource, since I don't think it was helping much.

Context

This should simplify #14464 a bit.


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

This will make it easier to attach additional headers (e.g. storage
class) on the s3 side of things and makes `Content-Encoding` less
special.
@github-actions github-actions Bot added the store Issues and pull requests concerning the Nix store label Nov 4, 2025
Comment thread src/libstore/s3-binary-cache-store.cc Outdated
@Ericson2314
Copy link
Copy Markdown
Member

So after this change CompressedSource::getCompressionMethod() is no longer used. Can we drop it and make CompressedSource into OwningStringSource instead?

I think that is more honest because CompressedSource has the wrong signature to become what we want it to become (it wouldn't own the underlying source, but just borrow it) anyways.

We can still keep a TODO about constant-space compression on upload, it just wouldn't be associated with OwningStringSource.

Copy link
Copy Markdown
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Otherwise this looks good. I like having fewer upload methods again. This does feel better.

@Ericson2314 Ericson2314 added this pull request to the merge queue Nov 7, 2025
@Ericson2314
Copy link
Copy Markdown
Member

Actually my comment can just a follow-up cleanup.

Merged via the queue into NixOS:master with commit 3c2dcf4 Nov 7, 2025
32 checks passed
@lovesegfault lovesegfault deleted the http-upload-headers branch November 8, 2025 02:44
@edolstra edolstra mentioned this pull request Dec 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

store Issues and pull requests concerning the Nix store

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants