Skip to content
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

Allow user to disable use of buildFromCabalSdist #220

Closed
wants to merge 6 commits into from
Closed

Conversation

srid
Copy link
Owner

@srid srid commented Jan 31, 2024

Using buildFromCabalSdist, as we do here:

fromSdist = self.buildFromCabalSdist or
(log.traceWarning "Your nixpkgs does not have hs.buildFromCabalSdist" (pkg: pkg));

# Make sure all files we use are included in the sdist, as a check
# for release-worthiness.
fromSdist
(x: log.traceDebug "${name}.fromSdist ${x.outPath}" x)

is problematic, because it uses its own cabal-install from a different package set called buildHaskellPackages. Thus, the user's own own overrides are not respected when building cabal-install.

See:

  1. https://nixos.zulipchat.com/#narrow/stream/413949-haskell-flake/topic/cabal-install.20ignoring.20user's.20package.20set
  2. https://nixos.zulipchat.com/#narrow/stream/413949-haskell-flake/topic/Using.20horizon.20package.20set.20with.20haskell-flake (where it prevents use of horizon package sets from @locallycompact)

To make this user configurable I made it part of the package settings (all.nix). So toggling this is now same as toggling haddock/jailbreak/check and the like, viz. this patch for https://github.com/srid/haskell-multi-nix :

--- a/flake.nix
+++ b/flake.nix
@@ -13,6 +14,7 @@
           projectFlakeName = "haskell-multi-nix";
           # Want to override dependencies?
           # See https://haskell.flake.page/dependency
+          settings.foo.buildFromCabalSdist = false;
         };
         packages.default = self'.packages.bar;
       };

@srid srid requested a review from roberth January 31, 2024 10:07
@srid
Copy link
Owner Author

srid commented Jan 31, 2024

@roberth What's your opinion on leaving this option disabled by default (as opposed to enabling it by default)?

Because these kinds of issues are not straightforward to debug without knowledge of the nixpkgs Haskell infrastructure.

@roberth
Copy link
Collaborator

roberth commented Jan 31, 2024

We should try not to add more configuration for things that should just work, and haskell-flake has a role in CI to ensure release-worthiness. Building from sdist is an important part of that.

Either using buildHaskellPackages was a mistake (not sure; I believe it makes cross-compilation work), or we should apply the user overrides to buildHaskellPackages too. That's how Nixpkgs overlays work. It has a special crossOverlays in case you want to edit the final package set only, whereas overlays applies to all instances of the Nixpkgs fixpoint.

Taking a step back though, what's the point of having a broken buildFromSdist in a haskell package set when it's cross? Arguably that'd be a bug in the Nixpkgs Haskell infrastructure. What if we change it to just take that function from the basePackages? Does that really break anything? If it does, it can and should be fixed in Nixpkgs and/or Horizon. (Probably by making it use buildHaskellPackages.cabal-install internally, and managing that package set correctly, if needed.)

not straightforward to debug without knowledge of the nixpkgs Haskell infrastructure.

I see this as the purpose of tools like haskell-flake: to distill all such knowledge into code that just does the right thing.
A corollary to this is that we risk encoding ignorance instead, and making its true resolution harder to accomplish.

@srid
Copy link
Owner Author

srid commented Jan 31, 2024

What if we change it to just take that function from the basePackages? Does that really break anything?

I tried using basePackages.buildFromCabalSdist (https://github.com/srid/haskell-flake/compare/transform2) but that didn't do anything different.

Here's the repro for the (1) case in the PR description:

srid/rings@f8864c3

Since I can't contribute to nixpkgs anyway, I'd be in favour of keeping this option (albeit as undocumented), but your points are all valid - so we should leave it enabled by default. Obviously if someone resolves this upstream we wouldn't even need this PR. Is there an official nixpkgs bug for this?

@roberth
Copy link
Collaborator

roberth commented Jan 31, 2024

Is there an official nixpkgs bug for this?

Doesn't seem so; no open issues for the buildFromSdist or buildFromCabalSdist functions.

I tried using basePackages.buildFromCabalSdist (https://github.com/srid/haskell-flake/compare/transform2) but that didn't do anything different.

Shouldn't that use the right cabal-install? Maybe it's not just about Cabal/cabal-install,but caused by a customSetup.hs? Did you try buildFromSdist`?

Note the docs on the cabal variations in pkgs/development/haskell-modules/make-package-set.nix

  Run `cabal sdist` on a source.

  Unlike `haskell.lib.sdistTarball`, this does not require any dependencies
  to be present, as it uses `cabal-install` instead of building `Setup.hs`.
  This makes `cabalSdist` faster than `sdistTarball`.

If buildFromSdist solves the problem, I think we can

  • Use buildFromSdist when a Setup.hs is present, and/or it has a component with a build-type: that isn't Simple
  • In nixpkgs, maybe speed up buildFromSdist by only forwarding the setupHaskellDepends - not the libraryHaskellDepends etc. Not sure if ./Setup.hs sdist works without library deps, but IMO it should.
    • If this doesn't work out, we only lose a bit of build latency, because of less concurrency, but no extra work is performed, fwiw. Also only in cases where ./Setup.hs exists.

Since I can't contribute to nixpkgs anyway

I could help out with the communication if needed.

@roberth
Copy link
Collaborator

roberth commented Jan 31, 2024

Or perhaps even ignore the little performance optimization and just go for buildFromSdist.

@srid srid mentioned this pull request Jan 31, 2024
@srid
Copy link
Owner Author

srid commented Jan 31, 2024

Or perhaps even ignore the little performance optimization and just go for buildFromSdist.

Let's do that then: #221

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants