haskell generic-builder: Use strictDeps always, take II

[Ericson2314]

Motivation for this change

The regenerated code + I think the subset of the overrides we still need

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Fits CONTRIBUTING.md.

---

CC @matthewbauer

[domenkozar]

I've had to fix ~10 references before nix-build -A cachix would evaluate :)

[Ericson2314]

Yes this is very much WIP. I did a quick & dirty rebase, and then Friday one out. I'll probably make it no overrides, and let hydra guide me to the minimal collection.

[domenkozar]

I've been trying to get this to work for a week and after fixing many references and adding 10+ overrides I still can't cachix to compile. I don't think this is practical yet, so I'm switching focus on finding why we see up to 6 times of dependencies in GCC flags in the first place: #41340 (comment)

[Ericson2314]

I couldn't reproduce the fatal eval error from https://hydra.nixos.org/jobset/nixpkgs/ericson2314-haskell-updates locally.

[matthewbauer]

@peti Unfortunately there are still many build failures even with the cabal2nix changes. I have added a few more overrides that I think are reasonable (and happen to fix lots of packages) but I wonder how many overrides you consider reasonable? Almost all of them are relying on hspec-discover to somehow propagate through from some other dependencies.

[matthewbauer]

Can we leave this out and just merge #43538 first?

[peti]

@matthewbauer,

I wonder how many overrides you consider reasonable?

1. Bugs in upstream's Cabal files should be fixed upstream. I would much rather have invested all those hours our contributors spent figuring out overrides in Nixpkgs into communication with upstream to make sure they fix their builds. It's particularly important to get @sol on board because the user manual for hspec-discover actually recommends the bad practice of assuming that depending on the hspec library will install the hspec-discover tool, too.

2. I am not particularly enthusiastic about the StrictDeps patch because it is a kludge. That change is intended to mitigate the effects of the ARG_MAX issue, but in the process it creates a ton of new issues. StrictDeps is not the proper fix for ARG_MAX. The proper solution is to remove all Haskell packages from buildInputs and to compute the dependency closure statically, inNix, instead of letting the generic builder's shell code to do it at run-time. It makes no sense that Haskell packages show up in any environment variables because Cabal doesn't use them anyway.

3. If it turns out that StrictDeps is desirable for some reason other than to patch up "ARG_MAX" -- which remains to be seen --, then the proper fix for the missing hspec-discover dependency is to add code into cabal2nix, i.e. at https://github.com/NixOS/cabal2nix/blob/master/src/Distribution/Nixpkgs/Haskell/FromCabal/PostProcess.hs#L22, that adds the appropriate dependency into the derivation every time a dependency on the hspec library is discovered.

[sol]

Bugs in upstream's Cabal files should be fixed upstream.

This is a cabal-install bug, not a bug in any package specifications.

Executables of dependencies are on the PATH by design. This was implemented by @23Skidoo for the specific purpose of bringing build tools into scope.

Here is the corresponding code, git commit haskell/cabal@7dc0a10 and issue haskell/cabal#1120.

That it doesn't work with cabal new-build anymore is a breaking change to how cabal-install interprets the .cabal spec; and that people like @hvr try to disguise it as a "bug fix" does not change the picture.

Don't get me wrong, I'm not saying that breaking changes to the .cabal spec itself are a absolute no-go, but they should be done with caution and a deprecation cycle. This did not happen here.

stack chose not to follow cabal-installs's way of re-interpreting the .cabal spec. Would that also be an option for Nix?

As a side note: Specifying hspec-discover in build-tool-depends does not guarantee that you get a version of hspec-discover that matches hspec unless you specify exact versions for both hspec and hspec-discover. On the other hand, pulling in hspec-discover as a dependency guarantees that the two versions match.

[peti]

@sol,

This is a cabal-install bug, not a bug in any package specifications.

I understand your point of view. Mine is different, though. cabal-install is merely catching up with features that many package managers have had for a long time. The dependency "make the hspec library available in the current environment" is not the same as "make the hspec library and all its executables and all the libraries and executables it depends on available in the current environment". It was a mistake to assume that expressing the former will accomplish the latter, and this assumption has been false for Nix and rpm and many other tools for years, really, and now it's false for cabal-install, too.

Executables of dependencies are on the PATH by design. This was implemented by @23Skidoo for the specific purpose of bringing build tools into scope.

Okay, but that was in 2012. Apparently, the perception these days is that dependencies on build-tools should be expressed explicitly, not implicitly.

Don't get me wrong, I'm not saying that breaking changes to the .cabal spec itself are a absolute no-go, but they should be done with caution and a deprecation cycle. This did not happen here.

To be fair, this issue has existed for a long time and you have been aware of it for a long time. You just chose to ignore it: sol/markdown-unlit#1 (comment).

stack chose not to follow cabal-installs's way of re-interpreting the .cabal spec. Would that also be an option for Nix?

Yes, we can do that as a workaround. Please note that the underlying problem is not going to go away, though. Your dependency specifications are inaccurate and the problems this causes will not get better in the future -- they'll get worse.

[sol]

This is what I intend to do sol/hpack#311.

[Ericson2314]

@sol

The commit you mention has "temporary" in the name commercialhaskell/stack#4132 (comment). For the record, I agree a deprecation cycle would have been prudent, as I argued in haskell/cabal#5412. But if they called the feature "temporary", I'd say that's a pretty clear indication they don't intend it as a first class solution and reserve the right to remove it.

The killer app for explicit tool dependencies is not per-component builds, but cross compilation, as I articulated here commercialhaskell/stack#4132 (comment). I hope this ends up turning stack's policy of no breakage here into a deprecation cycle; they do support GHCJS if not general cross compilation, after all. Your sol/hpack#311 solves the immediate crisis, but potentially causes a lot more unneeded work in the cross case than it does in the native case.

[Ericson2314]

@peti strictDeps isn't a kludge. Rather, the lack of strictDeps is a kludge as I explained here: #41340 (comment) . I don't disagree with computing the closure statically with closePropagation (thought that function needs to be fixed to match my changes to the stdenv original), but we still want strictDeps = true; here (and everywhere) so that native dependencies of Haskell stuff work more correctly with less duplicated flags.

I agree the thing to do as convince upstream, (and at least @sol has a workaround). I view our overrides, whether in nixpkgs or cabal2nix, as not an essential part of this change, but just as a gift to packages that don't follow the (updated) cabal spec.

[sol]

The commit you mention has "temporary" in the name commercialhaskell/stack#4132 (comment). For the record, I agree a deprecation cycle would have been prudent, as I argued in haskell/cabal#5412. But if they called the feature "temporary", I'd say that's a pretty clear indication they don't intend it as a first class solution and reserve the right to remove it.

@Ericson2314 as I mentioned elsewhere:

One more thing, the discussion on this feature and a code comment use the term temporarily. I want to point out that temporarily in this context means "temporarily, while the action passed to withSandboxBinDirOnSearchPath is running" and not temporarily as in "temporary workaround".

[Ericson2314]

OK my bad. Thanks for linking me that thread too.

[dezgeg]

Why is this strictDeps = false issue attempted to be worked around in Haskell infrastructure? If just plain strictDeps = false causes inappropriate things to happen then the bug is in stdenv, not Haskell infra.

[matthewbauer]

If just plain strictDeps = false causes inappropriate things to happen then the bug is in stdenv, not Haskell infra.

strictDeps = false alone causes every flag to be listed twice. Setting strictDeps = true would fix the ARG_MAX problem because we would just have less total args (although other things in Haskell like proposed by @angerman & @peti could also fix the ARG_MAX issue).

The reason strictDeps = false causes doubled flags is a consequence of the new cross compilation infrastructure. I think only @Ericson2314 understands 100% what all is going on but you can see some of how it works in setup.sh: https://github.com/NixOS/nixpkgs/blob/master/pkgs/stdenv/generic/setup.sh#L497-L506. It's meant to maintain compatibility with some old cross unfriendly things that are still extensively used in Nixpkgs (most common is listing build tools in buildInputs instead of nativeBuildInputs - but apparently there are also other things that strictDeps = true will break.

[dezgeg]

The reason strictDeps = false causes doubled flags is a consequence of the new cross compilation infrastructure.

Well yes, that is the exact bug that needs fixing. Just make it do the same thing as the old code did in the native compilation case, no?

but you can see some of how it works in setup.sh: https://github.com/NixOS/nixpkgs/blob/master/pkgs/stdenv/generic/setup.sh#L497-L506.

That's just the code for computing PATH, nothing that affects CFLAGS / LDFLAGS as far as I can see.

[domenkozar]

https://github.com/NixOS/nixpkgs/blob/master/pkgs/stdenv/generic/setup.sh#L516-L530

this part triggers setup-hooks multiple times, which is what binutils setup hook does to LDFLAGS.

[peti]

After some deliberation I've concluded to be neutral w/r to this PR. I'll trust other people's judgement to do the right thing.

[mmahut]

What is the status of this pull request?

[cdepillabout]

There has been no action on this PR since mmahut asked if it was still active last year, so I will go ahead and close it.

If this was premature, please feel free to re-open it or send a new PR. (I think most of the people on this thread should have permissions to just re-open.)