Allow callCabal2nix callers to tweak its source filtering

[ramirez7]

Description of changes

Fixes #256769

Repro + test of fix here: https://gitlab.com/ramirez7/bug-repros/-/merge_requests/1

This PR introduces a new callCabal2nix variation: callCabal2nixGeneric. I called it "generic" because it has an additional attrset arg that can be used to stuff various options the callCabal2nix Nix code needs.

In this case, I've added extraSrcFilter, which the caller can use to tell callCabal2nix to keep certain files around if they are needed to generate the cabal file. For instance, extra yaml files that a package.yaml might include.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.11 Release Notes (or backporting 23.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[cdepillabout]

A couple random thoughts:

• I've run into callCabal2nix filters too much (Doesn't play nice with hpack yaml includes or local subpaths) #256769 as well, and it is somewhat annoying to work around. However, it seems like a relatively(?) niche problem, and just defining your own callCabal2nix function is pretty straightforward.

• I don't know how I feel about adding another callCabal2nix variation. If we did want to add this additional functionality, I'd instead make the argument that we should change callCabal2nixWithOptions to dynamically check whether its 3rd argument is a string or a attr set, and execute differently depending on which it is passed. If it is a string, then it can just do the same as it currently does (in "backward-compat mode"). If it is an attr set, then we can add more configuration options, similar to as you've done in this PR.

• Instead of extraSrcFilter, I'd suggest just having a generic srcModifier argument:

  callCabal2nixWithOptions = name: src: {extraCabal2nixOptions ? "", srcModifier ? null}: args:
    let
      filter = path: type:
                 pkgs.lib.hasSuffix ".cabal" path ||
                 baseNameOf path == "package.yaml";
                 baseNameOf path == "package.yaml" ||
                 extraSrcFilter path type;
      defaultSrc =
        if pkgs.lib.canCleanSource src
        then pkgs.lib.cleanSourceWith { inherit src filter; }
        else src;
      expr = self.haskellSrc2nix {
        inherit name extraCabal2nixOptions;
        src = if srcModifier == null then defaultSrc else srcModifier src;
      };
    in overrideCabal (orig: {
         inherit src;
       }) (callPackageKeepDeriver expr args);

[ramirez7]

@cdepillabout I've incorporated your feedback:

• I've extended callCabal2nixWithOptions in a backwards compatible way.
  • Goes to show that using an attrset in general is a good default. It's a similar philosophy to JSON APIs always taking/returning objects instead of raw strings.
• I use a more general srcModifier arg now.
• I also wrote it in such a way that we still get attrset arg checking, which is nice.
• I've updated my repro MR to use it. It still works great!

re: whether this should be in upstream vs not:

• I considered writing my own callCabal2nix but it felt like I was just copying the guts of this function. And it wasn't obvious that using the lower-level things was correct. So I preferred calling a proper library function.
  • Would I have to just write my own wrapper around this?

let expr = self.haskellSrc2nix {
          inherit name extraCabal2nixOptions;
          src = if pkgs.lib.canCleanSource src
                  then pkgs.lib.cleanSourceWith { inherit src filter; }
                else src;
        };
in overrideCabal (orig: {
  inherit src;
}) (callPackageKeepDeriver expr args);

• While this is somewhat niche, I think if callCabal2nix is going to support package.yaml, it has to make an effort to actually support the format fully. YAML includes are one attraction of package.yaml, so I think nixpkgs having a somewhat easy way to support them (without writing your own callCabal2nix) would be nice.

[cdepillabout]

@ramirez7 Thanks, this looks better to me. Let's see what the other maintainers have to say.

[ramirez7]

Bumping this 😁

[maralorn]

Looks good to me.

I would love to see the behaviour documented in the manual!

[ramirez7]

@maralorn I'll add something!

[cdepillabout]

@ramirez7 Hey, just following up, I think we'd like to merge this, but can you do the following first:

• Rebase on the haskell-updates branch. (We generally ask for all Haskell-related changes to be based on that branch)
• Rewrite commit titles to be in line with https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#commit-conventions (and potentially squash commits if it makes sense)
• Add documentation to the Haskell section of the manual for this change.

[ramirez7]

Sorry I forgot to get back to you here! I'll hopefully make myself some time in the next month to do this - or I'll hand it off to someone at work (since we are using it there for our codebase).

[eldritch-cookie]

@cdepillabout some time passed and nothing happened, does the team still want to merge this? i could apply the changes you requested. @ramirez7 could i do so?

[maralorn]

I think the general idea is still sensible even though I kinda wished cabal2nix hadn’t supported this in the first place.

The commit history here feels kinda artificial to me. The steps are very small and do not even eval correctly in between.

Still some docs would be awesome.

[maralorn]

Unused x@.

[maralorn]

I feel like this whole section could be simplified by e.g. using or or inherit syntax.

[ramirez7]

@eldritch-cookie I'm cool handing this off to you!

@maralorn eh I think nixpkgs is somewhat stuck supporting hpack. Too many libraries and projects use it. I guess an alternative would be a separate intermediate source-to-source derivation that just runs hpack and then people could hand that munged source to callCabal2nix

Regardless, the fact that the hpack support can't handle basic monorepo features is definitely worth fixing! Multi-package, monorepo is a very common Haskell code architecture in the wild after all :)

[9999years]

@maralorn @eldritch-cookie I've addressed the review comments and added documentation, PTAL.