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

Feature/no more haskell libs #43713

Closed
wants to merge 3 commits into from

Conversation

angerman
Copy link
Contributor

Motivation for this change

Broken out of #43559

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.

@angerman angerman requested a review from peti as a code owner July 18, 2018 02:37
@matthewbauer matthewbauer reopened this Jul 18, 2018
peti
peti previously requested changes Jul 18, 2018
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.

I don't like this change very much. Removing the lib hierarchy goes against all kinds of common practices that we have in the Unix world in general and the Nix world in particular. I am sorry, but I'd have to say that I'm against this change.

@angerman
Copy link
Contributor Author

@peti, I'm find with this not being merged. I've only split #43559 apart. And I do need this fix (or a comparable one) right now. I believe @domenkozar is in the same camp, where a fix right now is needed an nixpkgs without it is simply broken.

@peti
Copy link
Member

peti commented Jul 18, 2018

My favorite solutions are (in order of preference):

  1. Remove everything but build tools from buildInputs in the Haskell builder.
  2. Revoke the changes that caused the duplication of flags.
  3. Turn on StrictDeps to mitigate the duplication of flags.

And those options are not mutually exclusive, i.e. we might conveivable want to do all three of those.

@nh2
Copy link
Contributor

nh2 commented Jul 19, 2018

  1. Remove everything but build tools from buildInputs in the Haskell builder.

@peti What's your estimate on how much work it is to implement this preferred solution? Can we just modify the generic builder or would that make many Haskell packages fail?

For my work on #43795 I would love to have a good long-term solution available (right now I have cherry-picked this PR).

@matthewbauer
Copy link
Member

matthewbauer commented Jul 19, 2018

@peti, I'm find with this not being merged. I've only split #43559 apart. And I do need this fix (or a comparable one) right now. I believe @domenkozar is in the same camp, where a fix right now is needed an nixpkgs without it is simply broken.

FWIW strictDeps should already be enabled for cross everywhere. So at least (3) is not going to change anything for cross mingw.

@angerman At what point (setup or build) & on what platform (linux or darwin) are you hitting ARG_MAX?

@nh2
Copy link
Contributor

nh2 commented Jul 19, 2018

@angerman At what point (setup or build) & on what platform (linux or darwin) are you hitting ARG_MAX?

@matthewbauer I'm not him but I can tell you my case: When building intray-web-server (a Haskell program with many deps) on Linux with musl (#43795). That creates 2500 -L flags.

@peti
Copy link
Member

peti commented Jul 19, 2018

@nh2,

What's your estimate on how much work it is to implement this preferred solution?

I think it's quite easy. We just replace the shell loop that collects all propagated dependencies by Nix code that does the same thing at evaluation time with lib.closePropagation. The Haskell builds don't need those packages in buildInputs. The only reason they are there is so to make them propagate as dependencies to other builds that use the package. We never relied on the fact that Nix sets up search paths and whatnot.

Can we just modify the generic builder or would that make many Haskell packages fail?

I believe the change would make no difference with respect to what compiles and what doesn't. In fact, packages that don't compile after that modification are broken already, we just didn't notice.

@angerman
Copy link
Contributor Author

@matthewbauer I see this when cross compiling macOS -> Windows with nix. But it's really just a function of the transitive dependencies. macOS ARG_MAX is lower than that on Linux usually. Which is why you need more dependencies to see it on linux as well. In any case the underlying reason is the explosion of Library Search paths, and that we don't use response files. Even then though GCC has only recently been fixed to pass them internally via response files.

This is very closely related to the LOAD COMMAND SIZE limit on macOS that haskell used to hit, and the MAX_ARG issue you hit easily on windows when building haskell applications natively.

Even though this reduces the number of arguments passed, you could potentially still exceed the ARG_MAX by blowing up your transitive dependency closure.

@matthewbauer
Copy link
Member

I think it's quite easy. We just replace the shell loop that collects all propagated dependencies by Nix code that does the same thing at evaluation time with lib.closePropagation. The Haskell builds don't need those packages in buildInputs. The only reason they are there is so to make them propagate as dependencies to other builds that use the package. We never relied on the fact that Nix sets up search paths and whatnot.

While I think this could be worth it in the long run- currently at least closePropagation is broken. It doesn't do the same things the setup.sh now does. We probably want to fix it in the long run but that makes this solution a little more difficult right now.

@peti
Copy link
Member

peti commented Jul 19, 2018

currently at least closePropagation is broken

Can you be more specific, please? What exactly is broken about closePropagation?

matthewbauer added a commit to matthewbauer/nixpkgs that referenced this pull request Jul 19, 2018
This is a new flag that is checked to prevent the wrappers from adding
CFLAGS & LDFLAGS. It should be used sparingly but in the case of
Haskell packages, it is necessary to avoid hitting ARG_MAX limits.
Haskell has its own way of handling deps which is done in
generic-builder.nix. Skipping the {C,LD}FLAGS is a harmless way to
reduce ARG_MAX.

Related to PR NixOS#43713.

/cc @angerman @Ericson2314 @peti
@matthewbauer
Copy link
Member

Can you be more specific, please? What exactly is broken about closePropagation?

It hasn't been updated with the new setup.sh. It's technically deprecated and I don't think anyone has changed it in 5+ years (3fbd694). It's possible that it will work for most cases - just we will need some changes for things introduced like depsTargetTargetPropagated, depsBuildBuildPropagated, depsBuildTargetPropagated, depsHostHostPropagated.

@matthewbauer
Copy link
Member

I have opened #43825 which I am hoping will be avoid some of the issues raised with this PR. It should have a similar effect to this - but avoid moving any directories around.

@Ericson2314
Copy link
Member

lib.closePropagation disregards the many different types of dependencies we have. It's possible that doesn't bite haskell because we only propoagated buildInputs, but it doesn't really bode well with me, as now I have to reason about two different types of propagation. I mean, I'd love to ditch bash-based propagation entirely, and do nix-based everywhere, but doing that piecemeal just leads to more complexity.

I suppose one could argue I also want strictDeps everywhere, and am advocating doing it piecemeal, so why's this different. But we already but in that case we already have the two cases (with and without strictDeps) and I am trying to get rid of one.

@peti
Copy link
Member

peti commented Jul 20, 2018

How come closePropagation is deprecated when it's still an integral part of the ghc wrapper?

This all seems very weird to me.

Anyhow, I've decided that I'll be neutral w/r to these changes. Please go ahead and do whatever you feel is best.

@peti peti dismissed their stale review July 20, 2018 07:19

I'll trust others to do the right thing.

@matthewbauer
Copy link
Member

I would suggest merging #43825 for now. Then in staging do what @dezgeg suggests in matthewbauer@110b5e0#r29781349.

Copy link
Member

@matthewbauer matthewbauer left a comment

Choose a reason for hiding this comment

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

@angerman
Copy link
Contributor Author

This was fixed in #43825.

@angerman angerman closed this Jul 27, 2018
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

6 participants