fetchgit, fetchFromGitHub: preserve attributes through overrideAttrs

[adeci]

Taking a stab at this after seeing it, hopefully fixes #444503.

When using overrideAttrs on a derivation from fetchFromGitHub, attributes like owner, repo, and rev were lost. This particularly affected derivations using the fetchgit backend (whenleaveDotGit = true, fetchSubmodules = true, etc.).

Things done

Following the idea from @ConnorBaker to have the fetcher implementation accept variable arguments and pass through those which are not explicitly used:

1. fetchgit: Now accepts passthru parameter and arbitrary arguments via ..., matching fetchzip's behavior
2. fetchFromGitHub: Passes GitHub-specific attributes through passthru to ensure they survive overrideAttrs

Testing

Tests passed:

• nix-build . -A tests.fetchFromGitHub --no-out-link
• nix-build . -A tests.fetchgit --no-out-link
• Custom test showing extremely minimal recreation of overrideAttrs issue

Expand for custom test

# test.nix
{ pkgs }:

let
  # Test 1: fetchzip path (default)
  srcZip = pkgs.fetchFromGitHub {
    owner = "NixOS";
    repo = "nix";
    rev = "2.3.16";
    sha256 = "sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=";
  };

  srcZipOverridden = srcZip.overrideAttrs (old: {
    sha256 = "sha256-BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB=";
  });

  # Test 2: fetchgit path (with leaveDotGit)
  srcGit = pkgs.fetchFromGitHub {
    owner = "NixOS";
    repo = "nix";
    rev = "2.3.16";
    sha256 = "sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=";
    leaveDotGit = true;
  };

  srcGitOverridden = srcGit.overrideAttrs (old: {
    sha256 = "sha256-BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB=";
  });
in
{
  fetchzip = {
    before = { inherit (srcZip) owner repo rev; };
    after = {
      owner = srcZipOverridden.owner or "MISSING";
      repo = srcZipOverridden.repo or "MISSING";
      rev = srcZipOverridden.rev or "MISSING";
    };
  };

  fetchgit = {
    before = { inherit (srcGit) owner repo rev; };
    after = {
      owner = srcGitOverridden.owner or "MISSING";
      repo = srcGitOverridden.repo or "MISSING";
      rev = srcGitOverridden.rev or "MISSING";
    };
  };
}

Before this PR:

$ nix eval --impure --expr 'import ./test.nix { pkgs = import <nixpkgs> {}; }'
{ fetchgit = { after = { owner = "MISSING"; repo = "MISSING"; rev = "2.3.16"; }; before = { owner = "NixOS"; repo = "nix"; rev = "2.3.16"; }; }; fetchzip = { after = { owner = "MISSING"; repo = "MISSING"; rev = "MISSING"; }; before = { owner ="NixOS"; repo = "nix"; rev = "2.3.16"; }; }; }

After this PR:

$ nix eval --impure --expr 'import ./test.nix { pkgs = import ./. {}; }'
{ fetchgit = { after = { owner = "NixOS"; repo = "nix"; rev = "2.3.16"; }; before = { owner = "NixOS"; repo = "nix"; rev = "2.3.16"; }; }; fetchzip = { after = { owner = "NixOS"; repo = "nix"; rev = "2.3.16"; }; before = { owner = "NixOS"; repo = "nix"; rev = "2.3.16"; }; }; }

• Built on platform:
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• Tested, as applicable:
  • NixOS tests in nixos/tests.
  • Package tests at passthru.tests.
  • Tests in lib/tests or pkgs/test for functions and "core" functionality.
• Ran nixpkgs-review on this PR. See nixpkgs-review usage.
• Tested basic functionality of all binary files, usually in ./result/bin/.
• Nixpkgs Release Notes
  • Package update: when the change is major or breaking.
• NixOS Release Notes
  • Module addition: when adding a new NixOS module.
  • Module update: when the change is significant.
• Fits CONTRIBUTING.md

---

Add a 👍 reaction to pull requests you find important.

[adeci]

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 455637
Commit: 6c039d2d2d5a1c8aa25d216b968010aed17c7d0c

[ConnorBaker]

This allows (but ignores) additional arguments -- args isn't used anywhere so any of the arguments not explicitly listed are lost. Not sure if that's the intention.

[adeci]

You're right that @args isn't used, so any extra arguments beyond the explicitly listed ones (and passthru) are dropped. This is intentional since we only need to:

1. Accept extra arguments without error (via ...)
2. Pass through passthru specifically (which we merge)

I also figured it may be best to drop everything not explicitly listed since passing all arbitrary arguments to mkDerivation could actually be risky, like setting builder or outputHash, etc.

Do you think it's best to remove @args?

[ConnorBaker]

I think removing args would make the intention to drop other arguments clearer.
Since binding the arguments to a variable allows you to access the arguments which weren’t declared (since you’re using no …), I assumed that you were going to use args in some way (conditional logic, filtering it and passing it on, etc.).

[ShamrockLee]

Why adding ellipsis? Functions with ellipsis are prone to misspelled/unsupported argument names.

[adeci]

I had @args originally to follow fetchzip's pattern, as it passes extra args through to fetchurl via removeAttrs, however while I did the same we only explicitly handle passthru instead of passing all args to mkDerivation. The difference here is that while fetchzip passes to another fetcher (fetchurl), fetchgit is passing directly to mkDerivation. I think it is a good idea to remove @args here like @ConnorBaker suggests for clarity and since we aren't using it for any logic later down the line.

Why adding ellipsis? Functions with ellipsis are prone to misspelled/unsupported argument names.

Regarding the ellipsis you're right it is prone to typo risks and we could remove it and only address what we need with passthru ? {}, but keeping it is consistent with fetchzip and allows for future flexibility if we later need to pass other attributes like custom meta etc.

I will remove ... since we don't immediately have any use for other attributes within fetchgit.

[emilazy]

cc @ShamrockLee – perhaps using lib.extendMkDerivation here would be more suitable?

Edit: whoops, Philip beat me to it.

[adeci]

cc @ShamrockLee – perhaps using lib.extendMkDerivation here would be more suitable?

Edit: whoops, Philip beat me to it.

I believe because fetchFromGitHub dynamically chooses to base itself on fetchzip or fetchgit depending on the args it takes, it makes it infeasible to use lib.extendMkDerivation here, since lib.extendMkDerivation requires a single static base builder.

@ShamrockLee actually covered precisely this here:
#294329 (comment)

I think we'd have to modify both fetchzip AND fetchgit to use lib.extendMkDerivation which would be a much larger refactor than this, which is just a fix for attr preservation in existing builders.

[ShamrockLee]

I think we'd have to modify both fetchzip AND fetchgit to use lib.extendMkDerivation which would be a much larger refactor than this, which is just a fix for attr preservation in existing builders.

Your proposal inspires me to reconsider its possibility.

As both fetchzip and fetchurl is based on stdenvNoCC.mkDerivation and can be reconstrtucted with use lib.extendMkDerivation, we could keep the fetchgithub's automatical power if we could dynamically decide on the path to stdenvNoCC.mkDerivaton after receiving the fixed-point argument, and it turns out to be mathematically possible.

Considering that

• extendMkDerivation composition:
  For all fArg, constructDrv0, excludeDrvArgNames1, excludeDrvArgNames0, extendDrvArgs1, extendDrvArgs0, transform1, transform0,

  Where ${"constructDrv${i + 1}"} is defined as

  extendMkDerivation {
    constructDrv = ${"constructDrv${i}"};
    excludeDrvArgNames = ${"excludeDrvArgNames${i}"};
    extendDrvArgs = ${"extrendDrvArgs${i}"};
    transform = ${"transform${i}"};
  }

  constructDrv2 is equivalent to:

  extendMkDerivation {
    constructDrv = constructDrv0;
    excludeDrvArgNames = excludeDrvArgNames1 ++ excludeDrvArgNames0;
    extendDrvArgs =
      finalAttrs: args:
      lib.flip removeAttrs excludeDrvArgNames0 (extendDrvArgs1 finalAttrs args)
      // extendDrvArgs0 finalAttrs (
        removeAttrs args excludeDrvArgNames1
        // extendDrvArgs1 finalAttrs args
      );
    transform = result: transform1 (transform0 result);
  }

We can formulate both fetchzip and fetchgit as an extendMkDerivation-based expression where constructDrv is stdenvNoCC.mkDerivation. Let the components be <name>0a and <name>0b.

Given that transform0a == transform0b == id, we only need to resolve excludeDrvArgNames and extendDrvArgs.

The extendDrvArgs part is easy, as finalAttrs and args are given as function arguments. The trick is to make excludeDrvArgNames the union of excludeDrvArgNames1a, 1b, 0a, and 0b, and add back the mis-excluded portion of args in extendDrvArgs.

Update:

• I fixed the extendMkDerivation composition part to include filtered args when passing down extendDrvArgs0.
• Here is the function to formulate a extendMkDerivation-based contsructor to base on the ancestor constructor (in this case, stdenvNoCC.mkDerivation):

  {
    accumulateConstructorMetadata =
      constructDrv:
      if constructDrv ? constructDrv.constructDrv then
        let
          constructDrvM1 = accumulateConstructorMetadata constructDrv.constructDrv;
        in
        constructDrv
        // {
          constructDrv = constructDrvM1.constructDrv;
          excludeDrvArgNames = constructDrv.excludeDrvArgNames ++ constructDrvM1.excludeDrvArgNames;
          extendDrvArgs =
            finalAttrs: args:
            removeAttrs (constructDrv.extendDrvArgs finalAttrs args) constructDrvM1.excludeDrvArgNames
            // (constructDrvM1.extendDrvArgs finalAttrs (
              removeAttrs args constructDrv.excludeDrvArgNames // constructDrv.extendDrvArgs finalAttrs args
            ));
          transformDrv = result: constructDrv.transformDrv (constructDrvM1.transformDrv result);
        }
    else
      constructDrv;
  }

[ShamrockLee]

@emilazy The passthru issue needs to solve first before fetchFromGitHub could be reconstructed with lib.extendMkDerivation.

BTW, there are at least three competing implementation addressing this issue. The other two are:

• fetchFromGitHub: takes passthru and enhance overriding #294329
• fetchFromGitHub: use passthru instead of // to expose rev and friends; fetchgit: support adding extra passthru values #370432

[adeci]

I dropped @args and ... within fetchgit since we only need to accept passthru explicitly. This keeps it minimal and avoids typo prone issues that ... could introduce. I originally included it to match fetchzip's pattern but it is used in a different way entirely.

BTW, there are at least three competing implementation addressing this issue.

We all take a similar approach, #294329 never included ... and after feedback this PR also avoids it now.

Your proposal inspires me to reconsider its possibility.

@ShamrockLee your extendMkDerivation approach can be built on this change! We need:

1. both fetchers to accept the same interface (passthru, meta, etc)
2. attributes to move through the construction pipeline
3. derivations to preserve these attributes

This provides all of the above, and with fetchgit accepting passthru, an extendMkDerivation based refactor becomes feasible.

[ShamrockLee]

@adeci My apology for being rude in my previous comment.

Build helpers with ellipses are surely more flexible, and ellipses are necessary to define a build helper with a mkDerivation-like interface. This is why language- or framework-specific build helpers often have ellipses. As for generic fetchers like fetchurl or fetchFromGitHub where hash stability often outweighs infinite flexibility, I would lean toward tightening the input control.

A down side of passing arguments (say repo) via passthru is that they will appear in finalAttrs as finalAttrs.passthru.repo instead of finalAttrs.repo, and one need to override passthru.repo in <pkg>.overrideAttrs (once we fix the interface), which is less intuitive.

If you are adding an ellipsis to pass arguments from the derived fetchers down the stdenvNoCC.mkDerivation, you can do it with derivationArgs instead. Some trivial build helpers like writeShellScriptBin already use this argument.

[adeci]

Closing, succeeded by #456751

[ShamrockLee]

I updated the composition expression in my above comment to fix an error and add a working code to rewrite the metadata of a build helpers to base on stdnevNoCC.mkDerivation.