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

haskell generic-builder: Use strictDeps always, take II #43498

Closed
wants to merge 10 commits into from

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jul 13, 2018

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
Copy link
Member

domenkozar commented Jul 14, 2018

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

@Ericson2314
Copy link
Member Author

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
Copy link
Member

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)

These seems to be common omissions due to a) non-new-build being overly
flexible, and b) code gen most often used for test boilerplate.
This helps avoid the `ARG_MAX` issues we've been having, and is
generally a good idea to ensure cross comparability anyways.
@Ericson2314
Copy link
Member Author

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

Fixes build error in haskellPackages.alex:

https://hydra.nixos.org/build/77569481/
Adds test tool inputs for hspec packages.
This is from the reverted branch but apparently still needed even
after the cabal2nix changes.
Needed to get cabal2nix building again.
@matthewbauer
Copy link
Member

@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.

@@ -5,7 +5,7 @@
# stripLen acts as the -p parameter when applying a patch.

{ lib, fetchurl, patchutils }:
{ stripLen ? 0, extraPrefix ? null, excludes ? [], ... }@args:
{ stripLen ? 0, extraPrefix ? null, excludes ? [], includes ? [], ... }@args:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@peti
Copy link
Member

peti commented Jul 17, 2018

@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
Copy link
Contributor

sol commented Jul 17, 2018

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
Copy link
Member

peti commented Jul 17, 2018

@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
Copy link
Contributor

sol commented Jul 17, 2018

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

@Ericson2314
Copy link
Member Author

Ericson2314 commented Jul 17, 2018

@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
Copy link
Member Author

Ericson2314 commented Jul 17, 2018

@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
Copy link
Contributor

sol commented Jul 17, 2018

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
Copy link
Member Author

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

@dezgeg
Copy link
Contributor

dezgeg commented Jul 18, 2018

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
Copy link
Member

matthewbauer commented Jul 18, 2018

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
Copy link
Contributor

dezgeg commented Jul 19, 2018

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
Copy link
Member

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.

Copy link
Member

@peti peti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

mmahut commented Aug 3, 2019

What is the status of this pull request?

@cdepillabout
Copy link
Member

cdepillabout commented May 5, 2020

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.)

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

Successfully merging this pull request may close these issues.

None yet

9 participants