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.

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

Git object hashing in libstore #8918

Merged
merged 3 commits into from
Feb 28, 2024
Merged

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Sep 4, 2023

Motivation

Part of RFC 133, tracking issue #8919

Method

  1. The libnixutil interfaces for working with NARs have been resused for a new implementation that works with Git tree+blob Merkle DAGs.

  2. libnixstore's ContentAddress and friends have been augmented to also express git hashes. There are new store paths accordingly.

  3. libfetcher's git fetcher has been agumented to also support this. Any fetcher could create git-hashed objects in principle, but the git fetcher is the best candidate since the source-side objects are also hashed this way; this allows us to fetch by tree hash and thus only need a single hash while still being pure.

The most notable detail is that when transferring git-hashed files over the wire, we create a store object for every subdir and file. This is the easiest way to make building the Merkle Dag safe and easy.

Context

Extracted from our old IPFS branches.

depends on #9294
depends on #9325
depends on #9815

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • documentation in the internal API docs
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

@github-actions github-actions bot added new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority store Issues and pull requests concerning the Nix store fetching Networking with the outside (non-Nix) world, input locking labels Sep 4, 2023
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/obsidian-systems-is-excited-to-bring-ipfs-support-to-nix/7375/64

@Ericson2314 Ericson2314 added the RFC Related to an accepted RFC label Sep 4, 2023
@Ericson2314 Ericson2314 force-pushed the git-objects branch 2 times, most recently from 3b44d2d to 06a23cd Compare September 4, 2023 15:32
@bobvanderlinden
Copy link
Member

I think it's RFC133 👍 very cool to see the progress in this space!

@fricklerhandwerk
Copy link
Contributor

Discussed in Nix team meeting 2023-09-15:

The first four commits are independent cleanups, can be split out.

@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-09-15-nix-team-meeting-minutes-86/33171/1

@Ericson2314 Ericson2314 force-pushed the git-objects branch 2 times, most recently from 79fc9f6 to ef57479 Compare September 21, 2023 15:23
@github-actions github-actions bot removed the fetching Networking with the outside (non-Nix) world, input locking label Sep 21, 2023
@Ericson2314 Ericson2314 changed the base branch from master to 2.18-maintenance September 25, 2023 15:40
@Ericson2314 Ericson2314 changed the base branch from 2.18-maintenance to master September 25, 2023 15:40
@Ericson2314 Ericson2314 marked this pull request as ready for review September 25, 2023 15:42
@Ericson2314 Ericson2314 force-pushed the git-objects branch 2 times, most recently from fb0c11b to a37bd98 Compare October 6, 2023 16:43
@Ericson2314 Ericson2314 force-pushed the git-objects branch 2 times, most recently from 79c9cc9 to 6578193 Compare January 19, 2024 06:11
@Ericson2314 Ericson2314 marked this pull request as ready for review January 19, 2024 06:12
@Ericson2314 Ericson2314 requested a review from roberth as a code owner January 19, 2024 06:12
@Ericson2314 Ericson2314 force-pushed the git-objects branch 5 times, most recently from 36e5ff3 to aab19fa Compare January 22, 2024 15:30
Copy link

dpulls bot commented Feb 22, 2024

🎉 All dependencies have been resolved !

echo Run Hello World! > $TEST_ROOT/dummy3/dir/executable
path3=$(nix store add --mode git --hash-algo sha1 $TEST_ROOT/dummy3)
hash3=$(nix-store -q --hash $path3)
test "$hash3" = "sha256:08y3nm3mvn9qvskqnf13lfgax5lh73krxz4fcjd5cp202ggpw9nv"
Copy link
Member

Choose a reason for hiding this comment

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

This needs some tests for what happens with store paths that have an executable file or symlink at top-level.

@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-02-03-nix-team-meeting-127/40309/1

@Ericson2314 Ericson2314 force-pushed the git-objects branch 2 times, most recently from 42373e5 to 6becfa8 Compare February 26, 2024 19:11
src/libstore/binary-cache-store.cc Outdated Show resolved Hide resolved
@@ -147,7 +148,7 @@ public:

void narFromPath(const StorePath & path, Sink & sink) override;

ref<SourceAccessor> getFSAccessor(bool requireValidPath) override;
ref<SourceAccessor> getFSAccessor(bool requireValidPath = true) override;
Copy link
Member

Choose a reason for hiding this comment

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

Specifying defaults on overrides should be superfluous, so I think it's best to omit them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually see https://en.cppreference.com/w/cpp/language/default_arguments

The overriders of virtual functions do not acquire the default arguments from the base class declarations, and when the virtual function call is made, the default arguments are decided based on the static type of the object.

src/libutil/git.hh Outdated Show resolved Hide resolved
src/libstore/content-address.cc Outdated Show resolved Hide resolved
* An enumeration of the ways we can ingest file system
* objects, producing a hash or digest.
*/
enum struct FileIngestionMethod : uint8_t {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can get rid of FileSerialisationMethod by having isFlat() and isRecursive() methods on FileIngestionMethod.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wish, but that only helps when we are returning FileSerializationMethod, dumpPath and other things that take FileSerialisationMethod as an argument really want to statically, provably, exclude the "git" case.

Ericson2314 and others added 3 commits February 27, 2024 11:27
They were getting skipped for the test-against checks.
Part of RFC 133

Extracted from our old IPFS branches.

Co-Authored-By: Matthew Bauer <mjbauer95@gmail.com>
Co-Authored-By: Carlo Nucera <carlo.nucera@protonmail.com>
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
Co-authored-by: Florian Klink <flokli@flokli.de>
Instead, serialize as NAR and send that over, then rehash sever side.
This is alorithmically simpler, but comes at the cost of a newer
parameter to `Store::addToStoreFromDump`.

Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
@Ericson2314 Ericson2314 merged commit f489a6e into NixOS:master Feb 28, 2024
8 checks passed
@Ericson2314 Ericson2314 deleted the git-objects branch February 28, 2024 00:02
@roberth
Copy link
Member

roberth commented Feb 28, 2024

This seems to now cause a failure in the daemon compatibility test in #9546.
Could you have a look @Ericson2314?

@thufschmitt
Copy link
Member

This seems to now cause a failure in the daemon compatibility test in #9546.

This seems to be a wrong bound in requireDaemonNewerThan, should be fixed by 6147d27

@Ericson2314
Copy link
Member Author

Thanks, both of you!

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 new-cli Relating to the "nix" command RFC Related to an accepted RFC store Issues and pull requests concerning the Nix store 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.

8 participants