Add fetchSubmodules to builtins.fetchGit #3166

blitz · 2019-10-25T15:52:59Z

This pull request adds the ability to fetch submodules with builtins.fetchGit. Use it like: builtins.fetchGit { fetchSubmodules = true; ... }

There are some downsides to this feature:

Submodules are not cached (unlike the root repo),
Full checkouts are created in a temporary directory.

These could be fixed, but require far more code and would require re-implementing some of the submodule cloning feature. This PR does not change any of the fetchGit behavior when submodules are not used (the default), so only people using this feature are paying these costs.

Other missing things:

Tests
Documentation

Note: This is my first contribution to Nix, so please expect stupid mistakes. :)

Fixes #2151

@edolstra @tfc

src/libexpr/primops/fetchGit.cc

blitz · 2019-10-26T09:27:58Z

I've added documentation and a test.

nmattia · 2019-11-22T15:19:29Z

@edolstra WDYT? this would be super helpful!

blitz · 2019-12-03T16:49:56Z

@edolstra @tfc I've resolved merge conflicts.

blitz · 2020-01-08T12:50:06Z

@edolstra It would be really nice to find a solution here to get submodule support working with builtins.fetchGit, because I'm currently building workarounds again. :)

src/libexpr/primops/fetchGit.cc

tests/fetchGitSubmodules.sh

bjornfor · 2020-01-13T07:51:45Z

Here is a patch with my proposed changes:

diff --git a/src/libexpr/local.mk b/src/libexpr/local.mk
index 26b9f14ba..e8c019365 100644
--- a/src/libexpr/local.mk
+++ b/src/libexpr/local.mk
@@ -8,7 +8,7 @@ libexpr_SOURCES := $(wildcard $(d)/*.cc) $(wildcard $(d)/primops/*.cc) $(d)/lexe
 
 libexpr_LIBS = libutil libstore libnixrust
 
-libexpr_LDFLAGS =
+libexpr_LDFLAGS = -lstdc++fs
 ifneq ($(OS), FreeBSD)
  libexpr_LDFLAGS += -ldl
 endif
diff --git a/src/libexpr/primops/fetchGit.cc b/src/libexpr/primops/fetchGit.cc
index b08e48404..c58d28bbc 100644
--- a/src/libexpr/primops/fetchGit.cc
+++ b/src/libexpr/primops/fetchGit.cc
@@ -6,6 +6,7 @@
 #include "hash.hh"
 #include "tarfile.hh"
 
+#include <filesystem>
 #include <sys/time.h>
 
 #include <regex>
@@ -182,9 +183,13 @@ GitInfo exportGit(ref<Store> store, const std::string & uri,
 
         runProgram("git", true, { "-C", tmpDir, "checkout", "--quiet", "FETCH_HEAD" });
         runProgram("git", true, { "-C", tmpDir, "remote", "add", "origin", uri });
-        runProgram("git", true, { "-C", tmpDir, "submodule", "--quiet", "update", "--init" });
+        runProgram("git", true, { "-C", tmpDir, "submodule", "--quiet", "update", "--init", "--recursive" });
 
-        deletePath(tmpDir + "/.git");
+        for (const auto& p : std::filesystem::recursive_directory_iterator(tmpDir)) {
+            if (p.path().filename() == ".git") {
+                std::filesystem::remove_all(p.path());
+            }
+        }
 
         gitInfo.submodules = true;
     } else {
diff --git a/tests/fetchGitSubmodules.sh b/tests/fetchGitSubmodules.sh
index c821d2caa..987bfd69e 100644
--- a/tests/fetchGitSubmodules.sh
+++ b/tests/fetchGitSubmodules.sh
@@ -47,4 +47,5 @@ pathWithSubmodules=$(nix eval --raw "(builtins.fetchGit { url = file://$rootRepo
 [[ ! -e $pathWithoutSubmodules/sub/content ]]
 [[ -e $pathWithSubmodules/sub/content ]]
 
-
+# No .git directory or submodule reference files must be left
+test "$(find "$pathWithSubmodules" -name .git)" = ""

I used std::filesystem for removing the .git paths. Nix already depends on C++17, but apparently one needs -lstdc++fs to get this library. The internet says clang needs a different flag, but the current flag worked in my testing (./shell.nix has a useClang flag which I enabled). So perhaps I was reading outdated info.

nmattia · 2020-01-13T09:59:30Z

By the way, I had a chat with @edolstra . He's refactoring the fetcher, which will need to land before this. Sharing this to clarify, and ideally to save people some time in case some of the changes don't apply anymore!

blitz · 2020-01-13T19:40:03Z

@bjornfor Thanks for the review and fixes! As @nmattia pointed out this will likely conflict with upcoming changes, so I'd rather wait before spending more time here. Or do you think there is a chance of sneaking this in before the refactoring?

@nmattia @edolstra It would be nice if there is some issue to watch regarding the fetcher changes.

bjornfor · 2020-01-24T08:09:22Z

I'm eagerly awaiting a status update here :-)

bjornfor · 2020-02-18T19:37:36Z

I did some real life testing with this patch and found two issues.

The result of builtins.fetchGit { fetchSubmodules = true; ... } changes from the first run to the next. The first run results in { outPath = ; ...; submodules = true; }, but the next calls result in { outPath = ; ...; submodules = false; }. I think the output submodules attribute would follow the input fetchSubmodules parameter exactly and not change on the 2nd (and following) runs.
Using fetchSubmodules = true on repos that have no submodules is a fatal error:

nix --experimental-features nix-command eval '(builtins.fetchGit { url = "https://github.com/nixos/nix.git"; ref = "refs/heads/master"; fetchSubmodules = true; } )'
fatal: Refusing to fetch into current branch refs/heads/master of non-bare repository
fatal: The remote end hung up unexpectedly
error: program 'git' failed with exit code 128

Allowing fetchSubmodules = true on non-submodule using repos is a requirement for my use case, and is supported by git itself: git clone --recursive https://github.com/nixos/nix.git is no error even if nix.git uses no submodules.

@edolstra: Are you refactoring the fetcher(s)? I would really like for nix to have git submodule support and can probably spend a bit more time on this if there is interest. But no sign of interest kills my motivation.

edolstra · 2020-02-18T20:39:12Z

Yes, the new fetcher infrastructure is in the flakes branch: https://github.com/NixOS/nix/tree/flakes/src/libstore/fetchers. It replaces fetchGit / fetchMercurial with a generic fetchTree builtin that uses the same attributes as flake inputs (e.g. fetchTree { type = "github"; owner = "NixOS"; repo = "nix"; rev = ...; }). So submodules would be an attribute supported by the git fetcher. And then flakes would automatically support Git repos with submodules as an input.

src/libexpr/primops/fetchGit.cc

blitz · 2020-02-20T12:31:31Z

@edolstra @tfc @bjornfor I was wondering how to implement submodule support while keeping all the nice caching in place. I'm thinking about something along the lines of:

Fetch a git repo
Export it into the Nix store
If submodule fetching is enabled:

enumerate all submodules, their URLs and commit hashes,
recursively use the normal git fetching logic to fetch them into the Nix store
join all submodules together using hard/symlinks

This would keep the normal caching of git repositories in place and submodules wouldn't be treated any differently from normal git checkouts.

The downside is that the implementation is somewhat complex compared to just using git clone --recursive and we are partially reimplementing the git submodule command.

To limit the complexity in Nix itself, most of this could be implemented as Nix expression, given that a builtin is available that returns a list of submodules and their properties.

Opinions?

edolstra · 2020-02-20T12:46:50Z

I think we should ignore the caching issue for now. I still want to add a generic caching mechanism to the fetcher infrastructure that generalizes the Git-specific caching (in ~/.cache/nix/git-revs). So Nix would keep a mapping fetcher input attributes -> store path which would also work for git-with-submodules.

bjornfor · 2020-02-26T20:43:00Z

Yes, the new fetcher infrastructure is in the flakes branch: https://github.com/NixOS/nix/tree/flakes/src/libstore/fetchers.

@blitz @edolstra: I'm confused about what the next action is. Ok, so flakes branch has new fetcher infrastructure, and this is where the future git fetcher with submodule support will live. But flakes is an experimental branch and it would be nice to have submodules support in standard Nix. (I don't know how long until flakes get stabilized and land in master.)

So, if this PR is refreshed (fixing the outstanding issues as commented in this thread), would it be merged?

blitz · 2020-02-26T21:05:10Z

@bjornfor I'll post an updated version soon. If I should rebuild this on top of flakes, this would be good to know.

edolstra · 2020-02-27T16:29:04Z

@bjornfor Yes, I'll take care of porting it to the new fetcher infrastructure.

blitz · 2020-03-02T13:57:06Z

I've incorporated all fixes. I just have to investigate the test failures.

blitz · 2020-03-03T10:42:09Z

@edolstra @bjornfor @nmattia @tfc I'd appreciate a second look here. I think I have added tests for all issues that were pointed out and these tests are passing now.

Two tests are failing (tests/multiple-outputs.sh and `tests/nix-build.sh), but these are not regressions and are already failing on the master branch.

bjornfor · 2020-03-05T19:58:00Z

@blitz: Did you build it? I'm getting this error:

/nix/store/m8ih8gw7qnd2c7p0qgv3cn4b6abkbanb-binutils-2.31.1/bin/ld: /build/nix-2.4pre7279_7fee022/src/libexpr/primops/fetchGit.cc:188: undefined reference to `std::filesystem::__cxx11::recursive_directory_iterator::operator++()'

In my patch I added libexpr_LDFLAGS = -lstdc++fs, but I don't see that in your PR.

blitz · 2020-03-09T13:04:39Z

@bjornfor

@blitz: Did you build it? I'm getting this error:
/nix/store/m8ih8gw7qnd2c7p0qgv3cn4b6abkbanb-binutils-2.31.1/bin/ld: /build/nix-2.4pre7279_7fee022/src/libexpr/primops/fetchGit.cc:188: undefined reference to `std::filesystem::__cxx11::recursive_directory_iterator::operator++()'
In my patch I added libexpr_LDFLAGS = -lstdc++fs, but I don't see that in your PR.

My understanding was that -lstdc++fs is only required for the experimental filesystem support and is not required anymore since the filesystem support moved out of its experimental state. That may be wrong for some platforms. I'm using:

% nix-build release.nix -A build.x86_64-linux

Some platforms seem to still require linking with stdc++fs to enable STL std::filesystem support.

…ue with submodules

Before this change it would be false for all evaluations but the first. Now it follows the input argument (as it should).

The .link file is used as a lock, so I think we should put the "submodule" attribute in there since turning on submodules creates a new .link file path.

Major bugfix for the submodules = true code path. TODO: Add tests.

Due to fetchGit not checking if rev is an ancestor of ref (there is even a FIXME comment about it in the code), the cache repo might not have the ref even though it has the rev. This doesn't matter when submodule = false, but the submodule = true code blows up because it tries to fetch the (missing) ref from the cache repo. Fix this in the simplest way possible: fetch all refs from the local cache repo when submodules = true. TODO: Add tests.

Using std::filesystem means also having to link with -lstdc++fs on some platforms and it's hard to discover for what platforms this is needed. As all the functionality is already implemented as utilities, use those instead.

blitz · 2020-03-29T22:45:37Z

@edolstra @bjornfor

I've removed std::filesystem use in favor of using the filter support in addToStore. This seemed simpler than using readDirectory and deletePath.

tfc · 2020-04-02T17:09:13Z

@edolstra @bjornfor anything else that needs to be done? i am very interested in seeing this patch merged.

edolstra · 2020-04-07T11:46:25Z

Merged, thanks!

nixos-discourse · 2020-04-08T08:48:49Z

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

https://discourse.nixos.org/t/why-doesnt-fetchfromgithub-support-submodules-with-private/6600/6

nixos-discourse · 2020-04-13T16:00:12Z

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

https://discourse.nixos.org/t/fetchgit-and-fetchgit-are-not-working-for-private-repos-with-submodules/6673/2

nixos-discourse · 2020-06-30T17:01:56Z

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

https://discourse.nixos.org/t/nix-flakes-and-submodules/7904/2

nixos-discourse · 2021-04-13T15:45:49Z

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

https://discourse.nixos.org/t/using-crate2nix-to-create-a-cargo-alternative-or-wrapper-that-transparently-uses-nix-store-as-a-cache/12095/5

qolii reviewed Oct 25, 2019

View reviewed changes

src/libexpr/primops/fetchGit.cc Show resolved Hide resolved

blitz force-pushed the fetchgit-recursive branch from 0fc3a9b to ead3356 Compare October 26, 2019 09:23

blitz changed the title ~~PoC: Add fetchSubmodules to builtins.fetchGit~~ Add fetchSubmodules to builtins.fetchGit Oct 28, 2019

blitz force-pushed the fetchgit-recursive branch from ead3356 to 45b1f1d Compare December 3, 2019 16:48

bjornfor reviewed Jan 9, 2020

View reviewed changes

src/libexpr/primops/fetchGit.cc Outdated Show resolved Hide resolved

bjornfor reviewed Jan 9, 2020

View reviewed changes

src/libexpr/primops/fetchGit.cc Outdated Show resolved Hide resolved

bjornfor reviewed Jan 9, 2020

View reviewed changes

tests/fetchGitSubmodules.sh Show resolved Hide resolved

kolloch mentioned this pull request Jan 31, 2020

Prefetch-less crate2nix by using builtin.fetchGit again, once it has submodule support nix-community/crate2nix#81

Closed

edolstra reviewed Feb 19, 2020

View reviewed changes

src/libexpr/primops/fetchGit.cc Outdated Show resolved Hide resolved

blitz force-pushed the fetchgit-recursive branch from 45b1f1d to 020066c Compare March 2, 2020 13:51

blitz requested a review from edolstra March 3, 2020 10:42

blitz and others added 11 commits March 29, 2020 22:29

Fix typo in submodule test

6c00a95

Link to stdc++fs

cc4fe97

Some platforms seem to still require linking with stdc++fs to enable STL std::filesystem support.

fetchGit: fix "fatal: couldn't find remote ref refs/heads/master" iss…

002a3a9

…ue with submodules

tests/fetchGitSubmodules: add more tests

587e259

fetchGit: fix submodule output attribute

6864ad7

Before this change it would be false for all evaluations but the first. Now it follows the input argument (as it should).

fetchGit: add submodules attribute to the .link file

369fffd

The .link file is used as a lock, so I think we should put the "submodule" attribute in there since turning on submodules creates a new .link file path.

tests/fetchGitSubmodules.sh: more checks

be84049

fetchGit: checkout rev instead of latest ref

b306b70

Major bugfix for the submodules = true code path. TODO: Add tests.

fetchGit: fix submodules = true for dirty trees

cc522d0

fetchGit: don't use std::filesystem to filter git repos

40c023e

Using std::filesystem means also having to link with -lstdc++fs on some platforms and it's hard to discover for what platforms this is needed. As all the functionality is already implemented as utilities, use those instead.

blitz force-pushed the fetchgit-recursive branch from 514a025 to 40c023e Compare March 29, 2020 22:32

edolstra self-assigned this Apr 7, 2020

edolstra merged commit 55cefd4 into NixOS:master Apr 7, 2020

blitz deleted the fetchgit-recursive branch April 7, 2020 13:53

Hoverbear mentioned this pull request Jun 20, 2020

Git submodules not being initialized? nix-community/naersk#110

Closed

evanjs mentioned this pull request Aug 7, 2020

Support for git sub modules in vendoring in tools.nix nix-community/crate2nix#101

Open

nmattia mentioned this pull request Aug 14, 2020

Fetching a github repo with submodules and checksum nmattia/niv#229

Open

This was referenced Nov 10, 2020

post-build-hook broken when using remote builders #4245

Closed

builtins.fetchGit can fail with "error: cannot lock ref [...]" #4252

Open

timbertson mentioned this pull request May 2, 2021

Fetchgit for submodules should be able to fetch arbitrary refs #4760

Open

Kha mentioned this pull request Sep 21, 2021

Huge performance regression when evaluating clean flake #5280

Closed

AndersonTorres mentioned this pull request Mar 5, 2023

RFC: hostapd: extend module to allow multiple APs. NixOS/nixpkgs#49171

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fetchSubmodules to builtins.fetchGit #3166

Add fetchSubmodules to builtins.fetchGit #3166

blitz commented Oct 25, 2019 •

edited

Loading

blitz commented Oct 26, 2019

nmattia commented Nov 22, 2019

blitz commented Dec 3, 2019

blitz commented Jan 8, 2020

bjornfor commented Jan 13, 2020

nmattia commented Jan 13, 2020

blitz commented Jan 13, 2020

bjornfor commented Jan 24, 2020

bjornfor commented Feb 18, 2020

edolstra commented Feb 18, 2020

blitz commented Feb 20, 2020

edolstra commented Feb 20, 2020

bjornfor commented Feb 26, 2020

blitz commented Feb 26, 2020

edolstra commented Feb 27, 2020

blitz commented Mar 2, 2020

blitz commented Mar 3, 2020

bjornfor commented Mar 5, 2020

blitz commented Mar 9, 2020

blitz commented Mar 29, 2020

tfc commented Apr 2, 2020

edolstra commented Apr 7, 2020

nixos-discourse commented Apr 8, 2020

nixos-discourse commented Apr 13, 2020

nixos-discourse commented Jun 30, 2020

nixos-discourse commented Apr 13, 2021

Add fetchSubmodules to builtins.fetchGit #3166

Add fetchSubmodules to builtins.fetchGit #3166

Conversation

blitz commented Oct 25, 2019 • edited Loading

blitz commented Oct 26, 2019

nmattia commented Nov 22, 2019

blitz commented Dec 3, 2019

blitz commented Jan 8, 2020

bjornfor commented Jan 13, 2020

nmattia commented Jan 13, 2020

blitz commented Jan 13, 2020

bjornfor commented Jan 24, 2020

bjornfor commented Feb 18, 2020

edolstra commented Feb 18, 2020

blitz commented Feb 20, 2020

edolstra commented Feb 20, 2020

bjornfor commented Feb 26, 2020

blitz commented Feb 26, 2020

edolstra commented Feb 27, 2020

blitz commented Mar 2, 2020

blitz commented Mar 3, 2020

bjornfor commented Mar 5, 2020

blitz commented Mar 9, 2020

blitz commented Mar 29, 2020

tfc commented Apr 2, 2020

edolstra commented Apr 7, 2020

nixos-discourse commented Apr 8, 2020

nixos-discourse commented Apr 13, 2020

nixos-discourse commented Jun 30, 2020

nixos-discourse commented Apr 13, 2021

blitz commented Oct 25, 2019 •

edited

Loading