libfetchers/git: fetch submodules by default#4922
Conversation
|
I agree this is the correct default, but there should still be a way to opt out for |
|
I agree, and thought of that as well. I'll take a look at it later and see if I can figure out a simple way to do just that, but setting attributes on |
That sounds more like git submodule abuse. I know that submodules are an easy solution to hard problems.... maybe nix has just changed me, idk. |
|
Obviously with Nix no-one would do that, but outside of our bubble that seems like a somewhat popular approach |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
|
Anyone know where I could start on implementing @Kha's suggestion? It'd be great to set attributes on self, but after grepping around a while, I'm not really sure where in the codebase I would start from? |
|
It looks like the nix/src/libexpr/flake/flake.cc Lines 93 to 94 in 223e056 inputName == "self" (or sSelf?) there since most other input attributes don't make sense on it, and then check for the value of the new attribute wherever self is actually instantiated.
|
|
We should definitely disable this by default in nixpkgs as it will break hashes and we don't want to fetch submodules by default anyway. |
This is not the same git fetcher that is being used in nixpkgs. The change here is likely only affecting the builtin fetcher. |
☝️that's true. go ahead and do a This solves a very real problem that I'm once again finding myself having to contort around. If I had one loonie (that is one Canadian dollar 😌) for every hour lost on git submodules in the past year I could've bought y'all a dozen rounds of very fancy drinks |
|
I second @kreisys proposal to simply merge this, though bias, we are really feeling some pain for this 😄 . I would love to continue to look into making it configurable in a future PR, but I am also extremely time constricted atm, and also not super great with C++ in general. I do think this is a saner default, since if submodules exist, we probably want them in most cases. And better to write a builtin style fetch for the rare case than the oft. Of course, if someone more familiar with the code base wants to add a commit to this PR to enable proper configuration of But the original scope of this PR was meant to simply change the default behavior, and maybe open a discussion on how to make it configurable as future work. |
|
@edolstra Any objection? If not will merge next week. |
|
Is there a way to disable submodules being pulled in for One idea could be to have a {
inputs = {
disabled_submodules = [ "llvm_project"];
};
outputs = inputs@{self}:
{
tiny_project = import ./tiny_project;
};
}We could then make |
|
@DieracDelta, if you skim the history of this very PR you will see that idea was requested already, but I didn't have time or the knowledge of this codebase to yet implement. I've talked with a few colleagues who have more experience with the Nix codebase, but they are similarly time constrained atm unfortunately. I'd be happy to have such a feature though. Conceptually, I see no reason to limit As a brief aside, one can still get a reference to the flake root without submodules by using a |
|
Obligatory "this broke my workflow". We develop software in a large monorepo with submodules that point to extremely large third party projects. We use a To be clear, we're using nix purely for a developer environment that can operate on a folder. That folder happens to contain source code as a git clone, but nix is not used to actually build any part of the project. As a result of this change, I have tried the following to no avail:
For now, I'm looking to revert my own install of the |
|
We should probably make it so that people can opt-in or opt-out of using submodules. something like {
inputs.self = {
enabledSubmodules = [ "./llvm/project" ];
disabledSubmodules = [ "./vendored/libasn" ];
};
... }edit: integrated @DieracDelta 's suggestion |
|
@nrdxp I am not super experienced with c++ (or the nix codebase for that matter), but I'm happy to attempt this once we are certain of the behavior. Was proposed behavior already worked out? @jonringer I quite like your proposal here. The follow up question I have is: what should the default behavior be? My vote would be for:
This also opens up the question of what the default behavior ought to be for nested submodules... |
|
The case where Nix is used just for dev-shells would be addressed by #3121 since then the top-level flake wouldn't have to be copied to the Nix store. |
|
@jonringer, if you would agree, instead of having special attributes on self, why not just allow setting the same options as on any other git flake input type? |
if you're referring to just having a |
Revert "Merge pull request #4922 from nrdxp/default-submodules"
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
This is the desired behavior according to @edolstra's comment: #4423 (comment)
In addition to making submodules opt-out instead of opt-in, this allows submodules to be pulled for a top-level flake's
selfattribute automatically, which was previously impossible.Note:
This doesn't work with the "github:owner/repo" syntax, but neither do submodules in general. I can open a separate ticket for that if desired
Resolves #4423