haskellPackages: fix build of packages maintained by me

[alexfmpe]

Things done

• 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, pkgs/README.md, maintainers/README.md and other READMEs.

---

Add a 👍 reaction to pull requests you find important.

[sternenseemann]

LGTM overall. Would be nice to have the name attribute set for fetchpatch so you can roughly tell what the patch does and differentiate the patches easier when e.g. one starts failing to apply.

[sternenseemann]

Can you open an issue?

[alexfmpe]

Hmm I think I screwed up the phrasing.
Getting PRs merged in the repo is what requires signing a custom license.
See google/proto-lens#494 (comment)
It's not that applying a patch requires a custom license... I think? This is really disruptive.

[wolfgangwalther]

I think the point here is, that opening an issue certainly does not require accepting that license?

No matter whether via Issue or PR, the ask is to "let upstream know about this".

[sternenseemann]

Yes, that's what I meant, I saw they want you to sign a CLA and agree this is ridiculous. (For this change, signing an CLA wouldn't even be necessary since it is almost certainly not copyrightable…)

[wolfgangwalther]

Would be nice to have the name attribute set for fetchpatch so you can roughly tell what the patch does and differentiate the patches easier when e.g. one starts failing to apply.

The contribution guidelines even require some kind of that: https://github.com/NixOS/nixpkgs/blob/master/pkgs/README.md#patches

Regardless of how the patch is included, you must ensure its purpose is clear and obvious. This enables other maintainers to more easily determine when old patches are no longer required. Typically, you can improve clarity with carefully considered filenames, attribute names, and/or comments; these should explain the patch's intention. Additionally, it may sometimes be helpful to clarify how it resolves the issue. For example: "fix gcc14 build by adding missing include".

[alexfmpe]

carefully considered filenames, attribute names, and/or comments; these should explain the patch's intention.

Well, I do link to the issue/PR. That gives more context than a fixed comment that might even turn out to be the wrong explanation depending on how the issue/PR evolves.
In theory one could add both, but I think an issue/PR is of higher priority as it interacts with upstream.
I see the "at a glance" benefit of inlining some text in a comment here, but I'm not sure it's worth the overhead and (admittedly rare) false positives.

Note that when it comes to the specific concern of knowing at a glance whether a jailbreak/patch can be removed, we have the means to do that automatically, a-la #384591

[wolfgangwalther]

I think giving the patch a name will give better output on the build failure (?), which would be especially relevant for an override with multiple patches. Even the automated script can't tell you which patch should be removed, right?

[sternenseemann]

Yes, I'm concerned with the build output since comparing sha1 hashes is not fun.

[alexfmpe]

Sorry for the delay. Been doing lots of travelling.

Even the automated script can't tell you which patch should be removed, right?

Huh good point. It sounds easy enough to build a srcOnly for each patch by removing it, but reporting multiple successes/failures per attribute looks like it's pretty incompatible with the way the script currently works.
I'll need to simmer on that for a bit.

In the meantime, I added name attributes to each patch, basically taken from the PR titles.
I wonder if it would make sense to use the issue/PR link as the name by default, which could push us a bit away from orphaned overrides.

[sternenseemann]

LGTM didn't test any builds

[alexfmpe]

LGTM didn't test any builds

$ nix-build \
  -A haskellPackages.basic-sop \
  -A haskellPackages.proto-lens-arbitrary \
  -A haskellPackages.proto3-wire \
  -A haskellPackages.records-sop
/nix/store/p30lmyqpfc8zfp6pv866i37nbplbszrh-basic-sop-0.3.0
/nix/store/59gs2gmy27k1kkmxirnsclp2qqw448hv-proto-lens-arbitrary-0.1.2.14
/nix/store/fgw1gbnfjyfxn36ah1r15mplvzw4xsii-proto3-wire-1.4.3
/nix/store/knhz60m4nk46rkan4jx9qdnx82ac58i8-records-sop-0.1.1.1