Skip to content

Conversation

hamishmack
Copy link
Collaborator

This change avoids duplicates in propagatedBuildInputs it uses checkUnique to enforce uniqueness at the top level and uniqueWithName fix places where duplicates might arise.

For instance:

  • packageInputs in shell-for.nix gave rise to duplicates when the shell was for building more than one component (and they shared dependencies).

  • libDeps in make-config-files.nix would include duplicates if a build-dependency was repeated in a .cabal files (Cabal 3.8.1.0 includes two references to process).

It should also remove duplicate framework lib and pkgconfig-depends.

Fixes #1933

hamishmack added 3 commits May 6, 2023 21:26
This change avoids duplicates in propagatedBuildInputs it uses `checkUnique` to enforce uniqueness at the top level and `uniqueWithName` fix places where duplicates might arise.

For instance:

* `packageInputs` in `shell-for.nix` gave rise to duplicates when the shell was for building more than one component (and they shared dependencies).

* `libDeps` in `make-config-files.nix` would include duplicates if a `build-dependency` was repeated in a `.cabal` files (Cabal 3.8.1.0 includes two references to `process`).

It should also remove duplicate `framework` `lib` and `pkgconfig-depends`.

Fixes #1933
Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Looks like an improvement! I'm still worried that we can end up with a really big propagatedBuildInputs and I'm not sure what we can do about it (except maybe actually try to use structuredAttrs). Suppose that:

  • A and B depend on C
  • D depends on A and B

Now I believe that A and B will put C in propagatedBuildInputs, and deduplicate that (no-op). Then when the stdenv creates D's buildInputs, it will jam together the propagatedBuildInputs for a A and B, which will now duplicate C.

So concretely I'd expect to end up with a lot of e.g. instances of containers in there. I guess we could check by inspecting some actual derivations, which I haven't. Maybe Nix is clever enough to get around this somehow.

# Not sure why pkgconfig needs to be propagatedBuildInputs but
# for gi-gtk-hs it seems to help.
++ map pkgs.lib.getDev (builtins.concatLists pkgconfig)
++ haskellLib.uniqueWithName (map pkgs.lib.getDev (builtins.concatLists pkgconfig))
# These only need to be propagated for library components (otherwise they
# will be in `buildInputs`)
++ lib.optionals (haskellLib.isLibrary componentId) configFiles.libDeps
Copy link
Collaborator

Choose a reason for hiding this comment

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

worth a comment that this is already deduplicated

@hamishmack
Copy link
Collaborator Author

Maybe Nix is clever enough to get around this somehow.

I think it is. We use the pkgsHostTarget env variable (where buildInputs and propagatedBuildInputs end up) and I have not observed duplicates in that. Part of the reason for switching from our DIY solution to using propagatedBuildInputs was the assumption that it would deduplicate more efficiently than we were (and even if it does not fixing that would benefit all nix packages not just haskell.nix).

The problem in #1933 seems to be that we were putting duplicates in propagatedBuildInputs itself and nix was not set up to deal with that.

It is concerning that there seems to be a limit to the number of propagatedBuildInputs a single derivation can have. The duplicates that caused problems were only at the very top level of the shell (we were including the direct dependencies of each component we selected for the shell).

@hamishmack hamishmack marked this pull request as ready for review May 8, 2023 09:02
@hamishmack hamishmack merged commit a1921f5 into master May 8, 2023
@iohk-bors iohk-bors bot deleted the hkm/arg-list-too-long branch May 8, 2023 09:03
@ocharles
Copy link
Contributor

ocharles commented May 9, 2023

This works for me - thanks!

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.

bash: Argument list too long when entering dev shell
3 participants