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

fetchgit: install git LFS with --local #113580

Closed
wants to merge 1 commit into from

Conversation

panicgh
Copy link
Contributor

@panicgh panicgh commented Feb 18, 2021

Motivation for this change

nix-builders run with GIT_CONFIG_NOSYSTEM (#63774) and git lfs install fails. The approach taken in PR #105998 sets HOME=$TMPDIR to make it work. However, git lfs install supports a --local flag to set itself up in the local repository's git config. I believe this would be cleaner than the current approach. Furthermore, nix-prefetch-git lacks the git-lfs dependence and does not publicly document the command line switch --fetch-lfs.

Note: I suspect this PR could cause git-lfs files in submodules not be fetched, so it may still need minor refinement.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@r-rmcgibbo
Copy link

Result of nixpkgs-review pr 113580 at e51cf7b5 run on aarch64-linux 1

11 packages built:
2 suggestions:
  • warning: license-missing

    Package is missing a license

    Near pkgs/tools/package-management/nix-prefetch-scripts/default.nix:22:5:

       |
    22 |     meta = with lib; {
       |     ^
    
  • warning: missing-phase-hooks

    installPhase should probably contain runHook preInstall and runHook postInstall.

    Near pkgs/tools/package-management/nix-prefetch-scripts/default.nix:13:5:

       |
    13 |     installPhase = ''
       |     ^
    

Copy link
Contributor

@bjornfor bjornfor left a comment

Choose a reason for hiding this comment

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

This is much better than the current situation: I'm having problems using nix-prefetch-git --fetch-lfs at all (on current master). I think we should (rebase and) merge.

@bjornfor
Copy link
Contributor

bjornfor commented Aug 4, 2021

I tested this on top of master, success! Keep in mind that --fetch-lfs CLI flag has been added already, and git doesn't catch that as a conflict, so I ended up with two --fetch-lfs in the help output :-)

The only thing I think we should add is

diff --git a/pkgs/build-support/fetchgit/nix-prefetch-git b/pkgs/build-support/fetchgit/nix-prefetch-git
index 879f187994e..30b046abdd3 100755
--- a/pkgs/build-support/fetchgit/nix-prefetch-git
+++ b/pkgs/build-support/fetchgit/nix-prefetch-git
@@ -403,6 +403,7 @@ print_results() {
   "date": "$(json_escape "$commitDateStrict8601")",
   "path": "$(json_escape "$finalPath")",
   "$(json_escape "$hashType")": "$(json_escape "$hash")",
+  "fetchLFS": $([[ -n "$fetchLFS" ]] && echo true || echo false),
   "fetchSubmodules": $([[ -n "$fetchSubmodules" ]] && echo true || echo false),
   "deepClone": $([[ -n "$deepClone" ]] && echo true || echo false),
   "leaveDotGit": $([[ -n "$leaveDotGit" ]] && echo true || echo false)

@bjornfor
Copy link
Contributor

bjornfor commented Aug 4, 2021

Note: I suspect this PR could cause git-lfs files in submodules not be fetched, so it may still need minor refinement.

Oh, I see. I'll try to test it.

@bjornfor
Copy link
Contributor

Sorry, I haven't had time to test this wrt. git submodules, but I created this PR (#134122) now to at least make nix-prefetch-git --fetch-lfs work out of the box.

@panicgh panicgh closed this Dec 27, 2021
@panicgh panicgh deleted the fetchgit-lfs branch December 27, 2021 20:41
@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants