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

libfetchers/git: fetch submodules by default #4922

Merged
merged 1 commit into from
Sep 7, 2021

Conversation

nrdxp
Copy link

@nrdxp nrdxp commented Jun 17, 2021

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 self attribute 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

@Kha
Copy link
Contributor

Kha commented Jun 23, 2021

I agree this is the correct default, but there should still be a way to opt out for self. Consider, for example, writing a flake.nix for a project that includes LLVM as a submodule. If the Nix build uses Nixpkgs LLVM anyway, copying the submodule to the store on every change would be a great waste of work and disk space.

@nrdxp
Copy link
Author

nrdxp commented Jun 23, 2021

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 self might just be more suitably handled in a separate PR. Thoughts?

@jonringer
Copy link
Contributor

I agree this is the correct default, but there should still be a way to opt out for self. Consider, for example, writing a flake.nix for a project that includes LLVM as a submodule. If the Nix build uses Nixpkgs LLVM anyway, copying the submodule to the store on every change would be a great waste of work and disk space.

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.

@Kha
Copy link
Contributor

Kha commented Jun 29, 2021

Obviously with Nix no-one would do that, but outside of our bubble that seems like a somewhat popular approach
https://github.com/rust-lang/rust/tree/master/src
https://github.com/ghc/ghc
(counterpoint: ziglang/zig-bootstrap#17)

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/how-can-i-make-a-nix-flake-source-recuse-through-submodules/14345/2

@nrdxp
Copy link
Author

nrdxp commented Aug 1, 2021

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?

@Kha
Copy link
Contributor

Kha commented Aug 2, 2021

It looks like the inputs attributes are parsed here:

static FlakeInput parseFlakeInput(EvalState & state,
const std::string & inputName, Value * value, const Pos & pos)
. You'll probably need to special-case 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.

@SuperSandro2000
Copy link
Member

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.

@andir
Copy link
Member

andir commented Aug 2, 2021

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.

@kaii-zen
Copy link
Contributor

kaii-zen commented Aug 12, 2021

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 git grep fetchGit (with a capital G) in a nixpkgs tree.
Can anyone counter this point? If not then PLEASE, I implore you, let's move forward with this.

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

@nrdxp
Copy link
Author

nrdxp commented Aug 12, 2021

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 self that'd be great too. 😅

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 edolstra added this to the nix-2.4 milestone Aug 13, 2021
@shlevy
Copy link
Member

shlevy commented Sep 2, 2021

@edolstra Any objection? If not will merge next week.

@DieracDelta
Copy link
Member

DieracDelta commented Sep 14, 2021

Is there a way to disable submodules being pulled in for self? I think there's a compelling use-case here. For example, consider if you want to bring in dependencies with nix but are also using a fork of LLVM. You have the LLVM fork pinned as a submodule, and are actively developing it in conjunction with other software. You probably don't want to entirely rebuild llvm using nix (you'd just want to rely on make+caching) every time. You also wouldn't want it to be copied into the store every time you try to build some other much smaller tool contained within the same repo.

One idea could be to have a disabled_submodules attribute in inputs specifying where the submodules we want to disable live:

{
  inputs = {
     disabled_submodules = [ "llvm_project"];
  };
  outputs = inputs@{self}:
  {
    tiny_project = import ./tiny_project;
  };
}

We could then make disabled_submodules attribute optional and leave the default behavior as "copy all submodules".

@nrdxp
Copy link
Author

nrdxp commented Sep 14, 2021

@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 self; we should be able to set many of the same options we use on other flakes. I felt that the request was a bit out of scope for this PR, and so hopefully another PR will emerge when someone can get around to it to allow for setting relevant options on self.

As a brief aside, one can still get a reference to the flake root without submodules by using a path to it instead of referencing self.

@rraval
Copy link

rraval commented Sep 16, 2021

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 flake.nix to fully specify a devShell environment with all developer dependencies specified. We then use nix-direnv to conveniently expose the devShell as a facade you can simply cd into.

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, nix print-dev-env path:/some/repo is now trying to clone all submodules for no particularly good reason (in this usecase! I acknowledge that cloning submodules seems reasonable when nix is actually building something from self).

I have tried the following to no avail:

  • nix print-dev-env git+file:///some/repo?submodules=false
  • nix print-dev-env git+file:///some/repo?submodules=0

For now, I'm looking to revert my own install of the nix binary to something right before this was merged. However, that's a stopgap and real support for avoiding submodules would be appreciated.

@jonringer
Copy link
Contributor

jonringer commented Sep 16, 2021

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

@DieracDelta
Copy link
Member

DieracDelta commented Sep 16, 2021

@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:

  • no submodules are cloned by default
  • submodules added to inputs.self.enabledSubmodules are cloned
  • submodules added to inputs.self.disabledSubmodules are not cloned
  • if a submodule is in both inputs.self.enabledSubmodules and inputs.self.disabledSubmodules, the flake will refuse to evaluate.
  • if a submodule is listed in either inputs.self.enabledSubmodules or inputs.self.disabledSubmodules but does not exist, the flake will refuse to evaluate.
  • if a submodule is not listed in either inputs.self.enabledSubmodules or inputs.self.disabledSubmodules, the flake will evaluate but not clone the submodule. This preserves backwards compatibility.

This also opens up the question of what the default behavior ought to be for nested submodules...

@edolstra
Copy link
Member

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.

@nrdxp
Copy link
Author

nrdxp commented Sep 16, 2021

@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?

@jonringer
Copy link
Contributor

jonringer commented Sep 16, 2021

@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 inputs.self.submodules = <bool>;, then I'm okay with that. This should be at least a solution for people who really don't or really do want submodules. Individual submodules could be considered a "nice to have" feature, and will likely besignificantly more complex implement.

edolstra added a commit to edolstra/nix that referenced this pull request Sep 22, 2021
edolstra added a commit that referenced this pull request Sep 23, 2021
Revert "Merge pull request #4922 from nrdxp/default-submodules"
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/tweag-nix-dev-update-18/15300/1

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.

nix flakes: add support for git submodules