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

nix: fix build by deferring submodule fetching #6613

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nrdxp
Copy link

@nrdxp nrdxp commented Jun 21, 2024

Describe your PR, what does it fix/add?

Currently, it isn't possible to fetch submodules on inputs.self of a Nix flake. As a workaround, use builtins.fetchGit with self.rev of the current checkout to include submodules.

This implementation defers submodule fetching to build time instead of before the flake evaluation begins, which would be the case if it were possible to specify submodule fetching in the inputs of self as with other inputs. This way, when interacting with the other outputs of the flake, the cost of fetching submodules is avoided.

Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)

Unfortunately, since the flake.lock format does not encode information about a canonical remote location for inputs.self, the url must be hardcoded.

Is it ready for merging, or does it need work?

I believe this is likely the best workaround, currently, to fix the build. We could alternately use one of the nixpkgs fetchers to make the source an FOD, however the benefit of the builtin is that we only need specify a git rev as the integrity hash.

Currently, it isn't possible to fetch submodules on `inputs.self` of a
Nix flake. As a workaround, use `builtins.fetchGit` with `self.rev` of
the current checkout to include submodules.

This implementation defers submodule fetching to build time instead of
before the flake evaluation begins, which would be the case if it were
possible to specify submodule fetching in the `inputs` of `self` as with
other inputs. This way, when interacting with the other outputs of the
flake, the cost of fetching submodules is avoided.
@github-actions github-actions bot added the Nix NixOS issue label Jun 21, 2024
@vaxerski vaxerski requested a review from fufexan June 21, 2024 13:55
Copy link
Member

@fufexan fufexan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solution is interesting. I can see the pros it would bring, but it also has some cons:

  • Building locally after making a change (uncommitted) still requires nix build '.?submodules=1'.
  • However, if you commit (but not push) the changes, it's impossible to build hyprland anymore.
  • The repo is downloaded twice (first without submodules, second with).

@@ -1,4 +1,5 @@
{
self,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not necessary to pass self here, as we already have the commit arg.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I'm sorry, you are right, I didn't notice that. I will push a fix.

@nrdxp
Copy link
Author

nrdxp commented Jun 21, 2024

This solution is interesting. I can see the pros it would bring, but it also has some cons:

  • Building locally after making a change (uncommitted) still requires nix build '.?submodules=1'.
  • However, if you commit (but not push) the changes, it's impossible to build hyprland anymore.
  • The repo is downloaded twice (first without submodules, second with).
  • indeed, I noticed this as well, it is a bit annoying
  • also annoying, true
  • yeah not ideal for sure

Actually, now that you point this out, I wonder if it is possible to use builtins.fetchGit on a local path. That would potentially fix these issues, but since flakes strip the .git directory, I'm not sure it would work. Maybe there is some way to do it though 🤔

@fufexan
Copy link
Member

fufexan commented Jun 21, 2024

Actually, now that you point this out, I wonder if it is possible to use builtins.fetchGit on a local path. That would potentially fix these issues, but since flakes strip the .git directory, I'm not sure it would work. Maybe there is some way to do it though 🤔

I've tried this for the "dirty" case, and Nix complains that we require a hash:

error: in pure evaluation mode, 'fetchTree' requires a locked input, at «none»:0

When passing --impure, git fails with exit code 128.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Nix NixOS issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants