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

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

Merged
merged 1 commit into from
Sep 23, 2021

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?

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