Content addressing and adding to store cleanup by Ericson2314 · Pull Request #9325 · NixOS/nix

Ericson2314 · 2023-11-09T03:29:54Z

Motivation

Organize content addressing

We now better distinguish ways to:

Content address a store object (to CA store path):
- Text
- Flat
- NAR
- (someday soon) Git
Content-address a file system object (to hash):
- Text
- Flat
- NAR
- (someday soon) Git

The former has a new underlying method to give the method it uses for the latter.

Use `SourceAccessor` with `Store::addToStore`

This helps promote SourceAccessor --- eventually (I think) we want just about all file system access to go directly through it and not use native operations directly. This will help us with in-memory story (good for unit tests!) and dovetails with #9278.

Remove now-redundant text-hashing store methods

addTextToStore and computeStorePathFromDump are now redundant because the other methods are upgraded to take a ContentAddressMethod not FileIngestionMethod --- we have a single suite of methods that works with all types store object content adressing, and we don't need any additional methods for the Text case.

Context

I was reminded how much duplicated code we had working on #8918. This is a long standing issue and it's high time we clean it up. Cleaning it up first will also make it easier to review git hashing support to libstore.

Priorities

Add 👍 to pull requests you find important.

edolstra · 2023-11-09T17:11:01Z

src/libfetchers/input-accessor.cc


 namespace nix {

-StorePath InputAccessor::fetchToStore(


Just to note but some of this code is here to minimize the diff with lazy-trees. So if we start changing this, it will increases the lazy-trees diff again.

@edolstra happy to fix that merge conflict :)

Right now there is a conflict in lazy trees from #8848 that is very domain-specific on both side. I can't help with that so much, but I am happy to do all the other conflicts!

fetchToStore stays non-virtual on lazy-trees so I think there is no issue here. This PR makes Store able to consume SourceAccessors directly so we just use that. Anything in an InputAcessor beyond the underlying SourceAccessor Store::addToStore won't care about anyways.

For the fingerprinting, Sure that method could go back, but we could also flip around the logic so the SourceAccesor itself has a maybeLstat-like method to try to cheaply content-address file system object, and the InputAccesor implementations do the getCache() to look this up.

On lazy-trees, InputAccessor::fetchToStore() implements a SQLite cache for SourcePath -> StorePath mappings that I want to backport to master. It optimizes stuff like src = <some big tree>. That's why it's important not to get rid of fetchToStore().

OK I'll put it back

It is now put back (not sure why this thread isn't marked outdated).

Not on a red or green line, is probably why.

Ericson2314 · 2023-11-09T22:21:36Z

Fixed the bug!

roberth

Just one suggestion.
Great stuff: less code, more consistent use of fewer interfaces, progress towards in-memory store.

Ok to merge after the suggestion and resolving the work priority inversion. It seems that you can't help @edolstra with lazy-trees until the existing conflict is resolved.

src/libstore/content-address.cc

Ericson2314 · 2023-11-16T03:24:14Z

@amjoseph-nixpkgs BTW this addresses some duplication you brought up a while back :)

nixos-discourse · 2023-11-20T14:47:35Z

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

https://discourse.nixos.org/t/2023-11-17-nix-team-meeting-minutes-104/35753/1

Ericson2314 · 2023-11-24T05:11:41Z

Just rebased this fixing conflicts. it would be very good to get it reviewed @edolstra so I can continue with the rest of the git hashing stuff!

nixos-discourse · 2023-11-27T16:08:48Z

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

https://discourse.nixos.org/t/2023-11-27-nix-team-meeting-minutes-107/36112/1

…tore` Co-authored-by: Robert Hensing <[email protected]>

`addTextToStore` and `computeStorePathFromDump` are now redundant. Co-authored-by: Robert Hensing <[email protected]>

Ericson2314 changed the title ~~content addressing and adding to store cleanup~~ Content addressing and adding to store cleanup Nov 9, 2023

github-actions bot added new-cli Relating to the "nix" command store Issues and pull requests concerning the Nix store fetching Networking with the outside (non-Nix) world, input locking labels Nov 9, 2023

edolstra reviewed Nov 9, 2023

View reviewed changes

Ericson2314 force-pushed the accessor-add-to-store branch from a1f39f5 to 2e3711a Compare November 9, 2023 22:21

Ericson2314 marked this pull request as ready for review November 9, 2023 22:21

Ericson2314 requested a review from thufschmitt as a code owner November 9, 2023 22:21

Ericson2314 force-pushed the accessor-add-to-store branch 2 times, most recently from 23b70f2 to 395e3c3 Compare November 10, 2023 13:15

roberth reviewed Nov 10, 2023

View reviewed changes

src/libstore/content-address.cc Outdated Show resolved Hide resolved

Ericson2314 mentioned this pull request Nov 10, 2023

Git object hashing in libstore #8918

Merged

8 tasks

Ericson2314 force-pushed the accessor-add-to-store branch from 54d85ce to 26b254b Compare November 13, 2023 05:35

Ericson2314 mentioned this pull request Nov 15, 2023

Add a new nix store add command #9357

Merged

thufschmitt assigned edolstra Nov 17, 2023

Ericson2314 force-pushed the accessor-add-to-store branch from 26b254b to fdc03a1 Compare November 20, 2023 16:12

Ericson2314 mentioned this pull request Nov 20, 2023

Tracking issue for RFC 133: Git hashing and Git-hashing-based remote stores #8919

Open

Ericson2314 force-pushed the accessor-add-to-store branch from fdc03a1 to d792a27 Compare November 24, 2023 04:56

Ericson2314 force-pushed the accessor-add-to-store branch from d792a27 to 3bd924a Compare December 3, 2023 21:37

Ericson2314 requested a review from fricklerhandwerk as a code owner December 3, 2023 21:37

roberth assigned roberth and edolstra and unassigned edolstra and roberth Dec 18, 2023

Organize content addressing, use SourceAccessor with `Store::addToS…

dfc8765

…tore` Co-authored-by: Robert Hensing <[email protected]>

Remove now-redundant text-hashing store methods

ed26b18

`addTextToStore` and `computeStorePathFromDump` are now redundant. Co-authored-by: Robert Hensing <[email protected]>

Ericson2314 force-pushed the accessor-add-to-store branch from 3bd924a to ed26b18 Compare December 18, 2023 15:44

edolstra merged commit 7cfd6c0 into master Dec 19, 2023

edolstra deleted the accessor-add-to-store branch December 19, 2023 14:10

Ericson2314 mentioned this pull request Dec 22, 2023

Add a Git-based content-addressed tarball cache #9485

Merged

theoparis mentioned this pull request Jan 21, 2024

Allow plain Nix expressions as flake inputs #5663

Open

Ericson2314 added this to the git-hashing stabilisation milestone Mar 22, 2024

Uh oh!

Comments

Conversation

Ericson2314 commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Organize content addressing

Use SourceAccessor with Store::addToStore

Remove now-redundant text-hashing store methods

Context

Priorities

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ericson2314 commented Nov 9, 2023

Uh oh!

roberth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Ericson2314 commented Nov 16, 2023

Uh oh!

nixos-discourse commented Nov 20, 2023

Uh oh!

Ericson2314 commented Nov 24, 2023

Uh oh!

nixos-discourse commented Nov 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Ericson2314 commented Nov 9, 2023 •

edited

Loading

Use `SourceAccessor` with `Store::addToStore`

Ericson2314 Nov 10, 2023 •

edited

Loading