Skip to content

fix: Add lastModified to git sources#3

Merged
nikstur merged 1 commit intonikstur:mainfrom
Tom-Hubrecht:main
Apr 21, 2025
Merged

fix: Add lastModified to git sources#3
nikstur merged 1 commit intonikstur:mainfrom
Tom-Hubrecht:main

Conversation

@Tom-Hubrecht
Copy link
Copy Markdown
Contributor

This is needed for nix to consider looking in the store instead of fetching the source each time. We also need to set shallow to true (which is the case technically).

I modified the v1 lock, which is incorrect, as a migration will be needed to add the lastModified field. But I didn't want to duplicate all the code, as this is only a filed addition.

Fixes: #2

@nikstur
Copy link
Copy Markdown
Owner

nikstur commented Apr 11, 2025

Can you point me to some documentation for the lastModified field (or source code). Couldn't find anything on a quick glance. Afaik Nix doesn't fetch the sources again if they're in the store already as long as it has the hash.

@Tom-Hubrecht
Copy link
Copy Markdown
Contributor Author

So, both lix and nix start at the same point, fetchGit is an alias to fetchTree, the rest is quite different:

CppNix

The real fetch is done here: https://github.com/NixOS/nix/blob/master/src/libexpr/primops/fetchTree.cc#L207
In fetchToStore, the store path is derived from the accessor ( https://github.com/NixOS/nix/blob/master/src/libfetchers/fetchers.cc#L197 ), which tries to lookup the store at https://github.com/NixOS/nix/blob/master/src/libfetchers/fetchers.cc#L313 but only if the input is final, which is not the case for inputs comming from fetchGit. Hence it will fallback to the default git accessor. As rev is known, the function called will be getAccessorFromCommit (c.f. https://github.com/NixOS/nix/blob/master/src/libfetchers/git.cc#L843 ). For remote repos, it will then use the user's local cache (c.f. https://github.com/NixOS/nix/blob/master/src/libfetchers/git.cc#L585 ), but when absent it will need to fetch it.

So in the case of cppnix, the remote git repo will always be fetched using fetchGit, even if the result is already in the store.
This can be masked by the repo cached in ~/.cache/nix/gitv3.

Lix

For lix, the ending is better. We start at https://git.lix.systems/lix-project/lix/src/branch/main/lix/libexpr/primops/fetchTree.cc#L194
and, if all the info required is available (c.f. https://git.lix.systems/lix-project/lix/src/branch/main/lix/libfetchers/fetchers.cc#L133 ) then we try to look in the store. Which for git inputs will be https://git.lix.systems/lix-project/lix/src/branch/main/lix/libfetchers/git.cc#L353 i.e.

           maybeGetIntAttr(input.attrs, "lastModified")
               && (shallow || maybeDirty || maybeGetIntAttr(input.attrs, "revCount"));

Thus, specifying lastModified and setting shallow to true will ensure that we first lookup the store before doing any fetching.

@Tom-Hubrecht
Copy link
Copy Markdown
Contributor Author

Tom-Hubrecht commented Apr 11, 2025

Also, just having a narHash was valid in cppnix until NixOS/nix#10612
More info at NixOS/nix#11701 for the __final thing

@nikstur
Copy link
Copy Markdown
Owner

nikstur commented Apr 18, 2025

This seems like a bug in Nix and Lix to be honest. Why would I need this piece of metadata information to substitute? This seems like a flakeism that should be fixed in Lix.

Not saying I'm not accepting the change, but this would just be a workaround for a bug in Nix/Lix IMO.

@Tom-Hubrecht
Copy link
Copy Markdown
Contributor Author

True, the whole issue is that fetchGit goes through fetchTree and the flake machinery, I'll try to work out a fix for Lix

@nikstur
Copy link
Copy Markdown
Owner

nikstur commented Apr 18, 2025

I'll try to work out a fix for Lix

Awesome! In the meanwhile I'm happy to merge this as a stopgap solution when my comments are addressed.

This is needed for nix to consider looking in the store instead of
fetching the source each time. We also need to set shallow to `true`
(which is the case technically).
@Tom-Hubrecht
Copy link
Copy Markdown
Contributor Author

I did https://gerrit.lix.systems/c/lix/+/3009, it's a hack, but it works

@nikstur nikstur merged commit a9ebed2 into nikstur:main Apr 21, 2025
1 check passed
@nikstur
Copy link
Copy Markdown
Owner

nikstur commented Apr 21, 2025

Thank you!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Store more information for git inputs

2 participants