Skip to content

Use the Git cache for tarball flakes#10038

Merged
Ericson2314 merged 5 commits intoNixOS:masterfrom
edolstra:tarball-git-cache
Feb 21, 2024
Merged

Use the Git cache for tarball flakes#10038
Ericson2314 merged 5 commits intoNixOS:masterfrom
edolstra:tarball-git-cache

Conversation

@edolstra
Copy link
Member

Motivation

This makes Nix use the same Git cache for tarball flakes as GitHub flakes.

Backported from the lazy-trees branch.

Context

Priorities and Process

Add 👍 to pull requests you find important.

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

@github-actions github-actions bot added with-tests Issues related to testing. PRs with tests have some priority fetching Networking with the outside (non-Nix) world, input locking labels Feb 19, 2024
@Ericson2314
Copy link
Member

I'm happy to review this one.

We have to support this for `fetchTree { type = "file" }` (and
probably other types of trees that can have a non-directory at the
root, like NARs).
Backported from the lazy-trees branch.
Comment on lines 167 to 168
if (res->cached) {
infoAttrs = cached->infoAttrs;
Copy link
Member

Choose a reason for hiding this comment

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

When does this happen?

Copy link
Member Author

Choose a reason for hiding this comment

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

It happens when the tarball metadata is cached in fetcher-cache-v1.sqlite and the tree is still in the Git cache:

    auto cached = getCache()->lookupExpired(inAttrs);
    ...
    if (cached && !getTarballCache()->hasObject(getRevAttr(cached->infoAttrs, "treeHash")))
        cached.reset();

Copy link
Member

Choose a reason for hiding this comment

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

How's that different from caching logic above?

Copy link
Member

Choose a reason for hiding this comment

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

Or rather, how is that different from

if (cached && !cached->expired)
        return attrsToResult(cached->infoAttrs);

?

I guess the answer is expired vs non-expired for impure caching?

Copy link
Member Author

Choose a reason for hiding this comment

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

The first one (!cache->expired) is before we do an HTTP request (i.e. it's when the fetcher cache has a cached entry that hasn't exceeded the tarballTTL yet). The second (res->cached) is when the HTTP request returns a cached (304) response.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added some comments about this.

Copy link
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.

Just a few minor things, basically asking for more comments.

Overall idea definitely looks good!

Copy link
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.

OK thanks for the comments, looks good now!

@infinisil
Copy link
Member

This seems to have caused #10395

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-08-12-nix-team-meeting-minutes-168/50561/1

@roberth
Copy link
Member

roberth commented Aug 14, 2024

Maybe this should use packbuilder?
It seems that this could significantly reduce the amount of small system I/O operations.
It can also be configured to use threads. (for I/O?, compression? - unclear)

@roberth
Copy link
Member

roberth commented Aug 23, 2024

I've confused this for

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fetching Networking with the outside (non-Nix) world, input locking with-tests Issues related to testing. PRs with tests have some priority

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants