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

Experimentally return treeHash on git and github fetchers #10344

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ericson2314
Copy link
Member

Motivation

We already had some #if 0 for this, indicating a sort of "TODO" experiment. I would like to make this part of the git-hashing experimental feature.

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.

We already had some `#if 0` for this, indicating a sort of "TODO"
experiment. I would like to make this part of the git-hashing
experimental feature.
@github-actions github-actions bot added the fetching Networking with the outside (non-Nix) world, input locking label Mar 27, 2024
@Ericson2314 Ericson2314 requested a review from edolstra as a code owner March 27, 2024 17:13
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.

This seems similarly unprincipled to #9025.
We have at least these kinds of tree hashes:

  • derived from tarball
  • taken from git commit
  • derived after doing whatever transformation on the source
  • maybe putting a tree hash into rev works

Some of these may coincide, but I'm pretty sure we'll have at least two, so the attributes should be qualified.

I don't think we should commit to anything before we know more about the new github: approach discussed in https://discourse.nixos.org/t/2024-03-25-nix-team-meeting-minutes-133/42167, especially because additions to the lock file are breaking changes.

Comment on lines +653 to +654
/* If we don't have submodules and aren't doing export
ignore, then the tree hash is useful info to provide. */
Copy link
Member

Choose a reason for hiding this comment

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

  • How is it useful?
  • getSubmodulesAttr(input) doesn't tell us anything about whether the repo has submodules. false doesn't imply anything, and true only correlates.

@Ericson2314
Copy link
Member Author

Yeah I agree it is lacking in principles. I want to take incremental steps towards supporting the "no submdules, tree hash is consistent through the entire process including of CA of store object" happy path, but I am not sure how. Per the end of the call maybe it is ignoring the per-fetcher stuff and starting with outputHashMode for fetchTree.

@Ericson2314 Ericson2314 marked this pull request as draft April 3, 2024 14:16
@Ericson2314 Ericson2314 added this to the git-hashing stabilisation milestone Apr 5, 2024
@thufschmitt
Copy link
Member

Triaged during the Nix maintainers team meeting: Not ready yet.

  • @Ericson2314: explains some of the issues that he and Robert had discussed

  • @roberth points at unresolved questions about just how much we want to trust fetchers today. Laziness complicates that

    • @Ericson2314: Agreed. E.g. if we are no longer always putting the entire source accessor in the store as one big store object, it is unclear when we would recheck a narHash in the lock file.

    • @roberth: We could use an in-memory overlay store for both .drvs (optimize the I/O latency) and as a store for lazy sources. This would allow the narHash to be computed without I/O and storage in the real store.

  • @roberth would be nice to just store commit objects so we can faithfully use original hash for commits (root commit object) and submodules (nested commit object)

    • two options for the extra data

      • store with dot files in tree (like the case hack)
      • store in DB (store object, but not file system object)

      Awkward to pollute tree, but also awkward to store arbitrary amounts of extra data in DB.

  • @Ericson2314: In any event, can side-step this unexplored design space for now by focusing on getting outputHashMode argument attributes on all the built-ins that need it (fetchTree, path, etc.). That is an easier-to-think-about next step, and will buy us some time to mull on these issues.

@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-04-08-nix-team-meeting-136/42963/1

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants