Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Content addressing and adding to store cleanup #9325

Merged
merged 2 commits into from
Dec 19, 2023
Merged

Content addressing and adding to store cleanup #9325

merged 2 commits into from
Dec 19, 2023

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Nov 9, 2023 •
edited
Loading

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.

@Ericson2314 Ericson2314 changed the title content addressing and adding to store cleanup Content addressing and adding to store cleanup Nov 9, 2023
@github-actions 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
edolstra reviewed Nov 9, 2023
View reviewed changes
src/libfetchers/input-accessor.cc
@@ -3,31 +3,6 @@

namespace nix {

StorePath InputAccessor::fetchToStore(
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@edolstra happy to fix that merge conflict :)

Copy link
Member Author

Choose a reason for hiding this comment

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

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!

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@Ericson2314 Ericson2314 Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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().

Copy link
Member Author

Choose a reason for hiding this comment

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

OK I'll put it back

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 is now put back (not sure why this thread isn't marked outdated).

Copy link
Member

Choose a reason for hiding this comment

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

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

@Ericson2314 Ericson2314 force-pushed the accessor-add-to-store branch from a1f39f5 to 2e3711a Compare November 9, 2023 22:21
@Ericson2314 Ericson2314 marked this pull request as ready for review November 9, 2023 22:21
@Ericson2314
Copy link
Member Author

Fixed the bug!

@Ericson2314 Ericson2314 force-pushed the accessor-add-to-store branch 2 times, most recently from 23b70f2 to 395e3c3 Compare November 10, 2023 13:15
roberth
roberth reviewed Nov 10, 2023
View reviewed changes
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@Ericson2314 Ericson2314 mentioned this pull request Nov 10, 2023
Merged
8 tasks
@Ericson2314 Ericson2314 force-pushed the accessor-add-to-store branch from 54d85ce to 26b254b Compare November 13, 2023 05:35
@Ericson2314 Ericson2314 mentioned this pull request Nov 15, 2023
Merged
@Ericson2314
Copy link
Member Author

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

@nixos-discourse
Copy link

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 Ericson2314 force-pushed the accessor-add-to-store branch from 26b254b to fdc03a1 Compare November 20, 2023 16:12
@Ericson2314 Ericson2314 mentioned this pull request Nov 20, 2023
Open
@Ericson2314 Ericson2314 force-pushed the accessor-add-to-store branch from fdc03a1 to d792a27 Compare November 24, 2023 04:56
@Ericson2314
Copy link
Member Author

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

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

@Ericson2314 Ericson2314 force-pushed the accessor-add-to-store branch from d792a27 to 3bd924a Compare December 3, 2023 21:37
@roberth roberth assigned roberth and edolstra and unassigned edolstra and roberth Dec 18, 2023
@Ericson2314 @roberth
Organize content addressing, use SourceAccessor with `Store::addToS…
dfc8765 
…tore`

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
@Ericson2314 @roberth
Remove now-redundant text-hashing store methods
ed26b18 
`addTextToStore` and `computeStorePathFromDump` are now redundant.

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
@Ericson2314 Ericson2314 force-pushed the accessor-add-to-store branch from 3bd924a to ed26b18 Compare December 18, 2023 15:44
@edolstra edolstra merged commit 7cfd6c0 into master Dec 19, 2023
13 of 14 checks passed
@edolstra edolstra deleted the accessor-add-to-store branch December 19, 2023 14:10
@Ericson2314 Ericson2314 mentioned this pull request Dec 22, 2023
Merged
@theoparis theoparis mentioned this pull request Jan 21, 2024
Open
@Ericson2314 Ericson2314 added this to the git-hashing stabilisation milestone Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edolstra edolstra edolstra left review comments

@roberth roberth roberth left review comments

@thufschmitt thufschmitt Awaiting requested review from thufschmitt

@fricklerhandwerk fricklerhandwerk Awaiting requested review from fricklerhandwerk fricklerhandwerk is a code owner

Assignees

@edolstra edolstra

Labels
fetching Networking with the outside (non-Nix) world, input locking new-cli Relating to the "nix" command store Issues and pull requests concerning the Nix store
Projects
Nix team
Archived in project
Milestone
git-hashing stabilisation
Development

Successfully merging this pull request may close these issues.
4 participants
@Ericson2314 @nixos-discourse @roberth @edolstra