Skip to content

Revert "Merge pull request #4922 from nrdxp/default-submodules"#5284

Merged
edolstra merged 1 commit intoNixOS:masterfrom
edolstra:revert-4922
Sep 23, 2021
Merged

Revert "Merge pull request #4922 from nrdxp/default-submodules"#5284
edolstra merged 1 commit intoNixOS:masterfrom
edolstra:revert-4922

Conversation

@edolstra
Copy link
Member

#4922 causes a few issues, see here and here. So it seems best to revert for now and figure out what to do after the 2.4 release.

@edolstra edolstra added this to the nix-2.4 milestone Sep 22, 2021
@edolstra edolstra merged commit 1359c2c into NixOS:master Sep 23, 2021
@edolstra edolstra deleted the revert-4922 branch September 23, 2021 09:13
@knedlsepp
Copy link
Member

knedlsepp commented Sep 23, 2021

😕 (I'm somewhat on the other end of the spectrum that not having submodule support is a showstopper for flakes for me)

@nrdxp
Copy link

nrdxp commented Sep 27, 2021

I don't really understand the complaints personally, and they can be easily worked around with an alternate fetcher. I don't know why you'd want to pull all of llvm as a submodule in a flake input anyway 😕

But alas, we are using flakes in production and this broke our workflow so we are probably going to maintain a patch and build our own Nix on our Hydra until a better solution emerges unfortunately. I originally opened the PR so we wouldn't have to do this.

I'd really love to study up on C++ and solve this myself, but Nix is an especially intimidating codebase and my backlog just keeps growing unfortunately 🤦

I guess the most annoying thing about it is that one guys usecase seems to trump like 15 people in favor of this 😒

@alexbakker
Copy link
Member

Unfortunately the commit being reverted here was already included in nixpkgs' nixUnstable, so this will break my workflow as well.

@archseer
Copy link
Member

archseer commented Sep 28, 2021

I don't understand this decision at all. While the patch causes inconvenience to some users with a whole bunch of unnecessary cloning, reverting the patch makes our usecase impossible because critical dependencies don't show up during build.

@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

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/using-git-crypt-with-flakes/15372/3

@knedlsepp
Copy link
Member

knedlsepp commented Aug 7, 2022

@edolstra As there are plans to stabilise flakes soonish, I'd like to shed light again on this unfortunate revert. I'm really hoping that we can somehow make git submodules enabled by default before flakes is declared stable. I imagine that changing defaults for submodules is a breaking change and the current UX of nix build "git+file://$(pwd)?submodules=1" is pretty sub-optimal compared to nix build. It would be unfortunate if we'll be stuck with it.

For reference the original comment of submodules should be the default #4423 (comment)
Was very well received. A lot of people seem to anticipate this change.

I wonder if the lazy-trees feature would somehow be helpful here?

Luflosi added a commit to Luflosi/Bachelor-Thesis that referenced this pull request Feb 7, 2025
Nix (with flakes) has an annoying behaviour where a Git subproject won't automatically be available, so a workaround is required.
See NixOS/nix#5284 and NixOS/nix#12421.
Luflosi added a commit to Luflosi/Bachelor-Thesis that referenced this pull request Feb 20, 2025
Nix (with flakes) has an annoying behaviour where a Git subproject won't automatically be available, so a workaround is required.
See NixOS/nix#5284 and NixOS/nix#12421.
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.

6 participants