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 -> fetchFromGitHub where possible #27327

Closed
wants to merge 1 commit into
base: staging
from

Conversation

Projects
None yet
5 participants
@volth
Contributor

volth commented Jul 12, 2017

Motivation for this change

fetchFrom* functions support tags since #26877 so they could be used where fetchgit had to be used to workaround the issue with forbidden characters in tag names.

Rebased to staging

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot

This comment has been minimized.

mention-bot commented Jul 12, 2017

@volth, thanks for your PR! By analyzing the history of the files in this pull request, we identified @svanderburg, @taku0, @the-kenny and @FRidh to be potential reviewers.

@volth volth force-pushed the volth:git-cleanup branch 4 times, most recently from a45704f to 04891fb Jul 12, 2017

@volth volth force-pushed the volth:git-cleanup branch from 04891fb to a1c6f2a Jul 12, 2017

@Mic92

This comment has been minimized.

Contributor

Mic92 commented Jul 12, 2017

This should probably quickly get merged before we have to many merge conflicts.

@edolstra

This comment has been minimized.

Member

edolstra commented Jul 12, 2017

I'm against the use of fetchSubmodules with fetchFromGitHub, and this adds dozens of uses of that. fetchFromGitHub was intended as a trivial wrapper around fetchzip (i.e. a fast HTTP download), not a complicated wrapper around fetchgit. So we should not promote its use. (I have a commit laying around reverting the addition of fetchSubmodules, but I haven't pushed it yet.)

@edolstra

This comment has been minimized.

Member

edolstra commented Jul 12, 2017

Arguably, fetchFromGitHub should not have been introduced in the first place since it doesn't really provide any value over calling fetchzip directly. In fact, fetchFromGitHub calls are more verbose than the equivalent fetchzip calls. But at least it was simple:

  fetchGitHub = { owner, repo, rev, sha256 }: fetchzip {
    url = "https://github.com/${owner}/${repo}/archive/${rev}.zip";
    inherit sha256;
  };

Now it has turned into this monstrosity:

  fetchFromGitHub = {
    owner, repo, rev, name ? gitRepoToName repo rev,
    fetchSubmodules ? false, private ? false,
    githubBase ? "github.com", varPrefix ? null,
    ... # For hash agility
  }@args: assert private -> !fetchSubmodules;
  let
    baseUrl = "https://${githubBase}/${owner}/${repo}";
    passthruAttrs = removeAttrs args [ "owner" "repo" "rev" "fetchSubmodules" "private" "githubBase" "varPrefix" ];
    varBase = "NIX${if varPrefix == null then "" else "_${varPrefix}"}_GITHUB_PRIVATE_";
  in if fetchSubmodules then
    fetchgit ({
      inherit name rev fetchSubmodules;
      url = "${baseUrl}.git";
    } // passthruAttrs)
  else
    # We prefer fetchzip in cases we don't need submodules as the hash
    # is more stable in that case.
    fetchzip ({
      inherit name;
      url = "${baseUrl}/archive/${rev}.tar.gz";
      meta.homepage = "${baseUrl}/";
    } // lib.optionalAttrs private {
      netrcPhase = ''
        if [ -z "''$${varBase}USERNAME" -o -z "''$${varBase}PASSWORD" ]; then
          echo "Error: Private fetchFromGitHub requires the nix building process (nix-daemon in multi user mode) to have the ${varBase}USERNAME and ${varBase}PASSWORD env vars set." >&2
          exit 1
        fi
        cat > netrc <<EOF
        machine ${githubBase}
                login ''$${varBase}USERNAME
                password ''$${varBase}PASSWORD
        EOF
      '';
      netrcImpureEnvVars = [ "${varBase}USERNAME" "${varBase}PASSWORD" ];
    } // passthruAttrs) // { inherit rev; };

which either calls fetchzip or fetchgit (so its semantics are no longer obvious) and does some magic impure netrc stuff that doesn't belong in Nixpkgs since nothing in Nixpkgs fetches private git repos. So I no longer feel we should promote the use of fetchFromGitHub.

@Mic92

This comment has been minimized.

Contributor

Mic92 commented Jul 12, 2017

I think node.js and go is a separate issue. fetchzip also needs a separate implementation, which would only work for github again. This needs to implemented in the wrapper.

Deprecating fetchgit support from fetchFromGitHub seems easier compared to removing it completely, since we have so many packages using it.

@volth

This comment has been minimized.

Contributor

volth commented Jul 12, 2017

Flipping between fetchgit and fetchFromGitHub depending on whether the current version of upstream software requires fetchSubmodules is not nice either...
There are basically two ways:

  1. Github is just a git hosting, one in a row, nothing special except of nice fetchzip-caching, which is anyway not for all github repos, but only for those which do not require fetchSubmodules nor leaveDotGit nor private. Then fetchFromGitHub is needless and we should use fetchzip and fetchgit depending on if fetchzipcould help in a particular case (this is subject to change, Github may introduce support of zip-download with submodules)
  2. Github is something special (which is probably true, there are much more Github repos than others), and pointer to repo is not a git://-url, but owner and repo. Then fetchFromGitHub is an interface to Github hiding implementation details (fetchzip or fetchgit) under the hood. Although it might be a complex and scary code, it is the only place to change if interface of github would change or require special attention (something like "to use http/2 or not").

The evolution of fetchFromGitHub function is shifting from the first vision to the second.

@grahamc grahamc closed this Jul 16, 2017

@edolstra edolstra referenced this pull request Jul 17, 2017

Closed

fetchnixpkgs: init #26802

4 of 6 tasks complete

@kamilchm kamilchm referenced this pull request Nov 2, 2017

Merged

goDeps: support fetchFromGitHub to fetch Go libs #19311

4 of 7 tasks complete

@schneefux schneefux referenced this pull request Oct 8, 2018

Merged

treewide: Use fetchFromGitHub instead of fetchurl #48044

2 of 9 tasks complete

@yegortimoshenko yegortimoshenko referenced this pull request Nov 1, 2018

Open

uefitool: init at 0.25.1 #45963

4 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment