[RFC] Hash overriding of fetcher-constructed FODs?

[ShamrockLee]

Background

The adoption of fixed-point arguments (finalAttrs: { }) is enabling us to override fetcher arguments with standard <pkg>.overrideAttrs instead of custom, locally-added override.

Most fetchers support the hash argument to specify FOD hashes with SRI hashes or optionally an empty string, while current fetcher implementation typically exposes only the derivation attributes outputHash, and its siblings outputHashAlgo and outputHashMode to <pkg>.overrideAttrs.

There are another kind of fetchers which returns a wrapper of an FOD rather than the FOD itself. For instance, rustPlatform.fetchCargoVendor first creates the vendorStaging FOD and then return a derivation wrapping the FOD. I refer to them as "wrapped fetchers" in the following questions.

Questions

1. Should we override fetcher hashes with <pkg>.overrideHash?
   • (+) Custom overriders (like overridePythonAttrs) are causing enough problems. The unified <pkg>.overrideAttrs is the way.
   • (-) Custom override for fetcher results inherently have an interface identical to the fetcher itself.
     • (+) We could also make <pkg>.overrideAttrs accept hash and push similar changes to unify significant part of the <fetched-pkg>.overrideAttrs arguments toward those used by the fetcher itself.
   • (-) <pkg>.overrideAttrs doesn't white-list the overriding arguments and is prone to typos.
     • (+) We can implement optional checks to white-list the attributes.
   • (-) For fetchers like fetchFromGitHub that dynamically switch between base fetchers (fetchgit and fetchzip), it is hard to implement the dynamic switching functionality for <pkg>.overrideAttrs.
     • (+) It is possible to implement ( fetchFromGitProvider: init (optional dynamically switchable after <pkg>.overrideAttrs) #462034 ), but at significant performance overhead.
     • (+) We could implement manual switch via fetchFromGitHub.override.
2. Should we make hash overridable with <pkg>.overrideAttrs, or should we keep overriding outputHash?
   • (-) Every FOD has outputHash. Its overriding functionality is 100% backward compatible and mostly available.
   • (+) Making hash overridable is more intuitive and consistent with fetchers' interfaces.
   • (+) Making hash overridable allows outputHash = if finalAttrs.hash == "" then lib.fakeHash else finalAttrs.hash, making it easy to determine the hash with hash = "" and make the hash algorithm future proof when specifying empty hashes.
   • (+) This exposes src.hash, which sounds more intuitive than src.outputHash.
     • (-) We already have src.outputHash.
3. If we choose to override hash, should we also expose the hash attribute of wrapped fetchers's direct result attribute, or should we instruct the user to override the hash by overriding the inner FOD?
   • (-) Just instruct people to override the inner FOD, as they have to when overriding other attributes.
   • (+) hash is special to fetchers, and it's important to ensure src.overrideAttrs { hash = ...; } always work.

Related issues and PRs

• Impossible to override arguments of rustPlatform.fetchCargoVendor #415397
• fetchhg: fix hash assertion; make overridable when sha256 set #423877 (This is what it looks like to make hash overridable.)
• fetchurl: fix overrideAttrs working as expect with hash #459487
• Documentation: guide for using let in vs rec vs finalAttrs #315337

[ShamrockLee]

Ping:
@axelkar
@Pandapip1 @philiptaron @MattSturgeon
@SuperSandro2000
@isabelroses @Atemu @pluiedev
@adeci

[Pandapip1]

I personally think 2 is the cleanest

[ShamrockLee]

I personally think 2 is the cleanest

@Pandapip1 Do you mean the following?

1. Yes. (Override FOD with <pkg>.overrideAttrs)
2. Yes. (Override with hash)
3. No. (Override inner FOD when wrapped)

[axelkar]

I prefer having hash as an argument for consistency. I believe override is for functions (e.g. the one defining a package) so I think it shouldn't be used here.

overrideAttrs is commonly known as overriding arguments to stdenv.mkDerivation or its wrappers defined with lib.extendMkDerivation so there should be a pattern for it to support cargoHash and equivalents. Fetchers aren't defined with lib.extendMkDerivation so I don't know what should be done there 🤔.

Wasn't there a discussion about adding an overrideHash somewhere? Though I'm against it because it seems so ad-hoc.

I support that overriding e.g. cargoHash resets cargoDeps. Basically it means that finalAttrs.cargoHash should be used when defining cargoDeps in the mkDerivation wrapper.

[ShamrockLee]

Fetchers aren't defined with lib.extendMkDerivation so I don't know what should be done there 🤔.

They will be. I'm pushing the restructuring.
feturl, fetchzip, fetchgit, and fetchhg are already defined with lib.extendMkDerivation.

[MattSturgeon]

To check my understanding:

• Currently, FOD packages have a outputHash attribute (accessible on the final package and its finalAttrs) and a hash argument, which is not accessible on the final package.
• hash and outputHash are usually identical, unless the hash argument is an empty string, in which case outputHash is set to lib.fakeHash.
• If one was to override a FOD-package today, they would override the outputHash attribute manually.

Option 2 — promoting hash to an actual derivation attribute and using <FOD>.overrideAttrs to override it — makes the most sense to me. It has a little bit of redundancy (both hash and outputHash being derivation attrs), but is the most intuitive to work with.

Off topic thought

Having input args accessible as derivation attrs is convenient and intuitive for overriding, however having them directly affects package rebuilds, create build-time environment variables, etc, feels wrong.

Do we need a mechanism similar to passthru, where we can blacklist mkDerivation attrs from being passed down to builtins.derivation?

E.g.

# Simplified
mkDerivation =
  {
    extraPassthrus ? [],
    passthru ? {},
    ...
  }@args:
  builtins.derivation (
    builtins.removeAttrs args (extraPassthrus ++ [
      "extraPassthrus"
      "passthru"
    ])
  )
  // lib.genAttrs extraPassthrus (x: args.${x})
  // passthru;

That way attributes can be part of finalAttrs without (directly) affecting the actual derivation, and without needing to do the finalAttrs.passthru dance.

[ShamrockLee]

@MattSturgeon Thank you for your feedback.

To check my understanding:

* Currently, FOD packages have a `outputHash` attribute (accessible on the final package and its `finalAttrs`) and a `hash` argument, which is not accessible on the final package, which is overridable with `<pkg>.overrideAttrs`.

• All current FOD packages have a outputHash attribute, which is overridable as a stdenv.mkDerivation argument.
• All current fetchers I'm aware of takes the hash argument, but only some of them takes outputHash for compatibility purposes.
• For most fetchers, all hash-related arguments (including outputHash) are currently not overridable with <pkg>.overrideAttrs, and this does not contradict the fact that the outputHash passed to stdenv.mkDerivation is overridable. (Update: Fix typo overrideHash -> outputHash).

* `hash` and `outputHash` are usually identical,

Yes.

* ... unless the `hash` argument is an empty string, in which case `outputHash` is set to `lib.fakeHash`.

No. Current implementation tends to set outputHash to empty string and specify outputHashAlgo = "sha256".

* If one was to override a FOD-package today, they would override the `outputHash` attribute manually.

PR #158968 adds the custom overrider override to the result FOD constructed by some fetchers include fetchgit and fetchFromGitHub, and they are currently overridden by said custom override. We can deprecate such custom override if we answer "yes" to Question 1.

For other fetchers, manually <pkg>.overrideAttrs to re-specify outputHash* and other affected attributes is the only way of overriding.

[ShamrockLee]

The above comments share these points:

1. Use <pkg>.overrideAttrs instead of locally added override to override fetcher-constructed FODs.
2. Override hash consistently instead of outputHash.

[ShamrockLee]

I came across a potential deal-breaker of <pkg>.overrideAttrs-based FOD overriding: the <pkg>.overrideAttrs arguments are not white-listed, making it prone to typos.

Most generic fetchers, such as fetchurl or fetchgit, white-list their input arguments. In other words, they use a set pattern without an ellipsis, or pass all their not-explicitly-handled arguments into another fetcher without an ellipsis. The custom override inherits such a white-listing behavior, but not <pkg>.overrideAttrs.

[Pandapip1]

That seems like a more general overrideAttrs issue; can I suggest opening a separate issue for overrideAttrs argument validation?

[MattSturgeon]

That seems like a more general overrideAttrs issue; can I suggest opening a separate issue for overrideAttrs argument validation?

There's nothing to solve. overrideAttrs exists to override the attrs passed to mkDerivation. And mkDerivation allows passing arbitrary attrs.

[ShamrockLee]

That seems like a more general overrideAttrs issue; can I suggest opening a separate issue for overrideAttrs argument validation?

I could imagine some form of validation against finalAttrs, but that would be very strict and coupled to the makeDerivationExtensible part of stdenv.mkDerivation implementation.

BTW, that is one of the motivation for people to explore module-based packaging. (See @DavHau's drv-parts and dream2nix for detail.)