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

Cabal/Haskell: Importing generated path module leads to large closure #164630

Open
m4dc4p opened this issue Mar 17, 2022 · 12 comments
Open

Cabal/Haskell: Importing generated path module leads to large closure #164630

m4dc4p opened this issue Mar 17, 2022 · 12 comments

Comments

@m4dc4p
Copy link

m4dc4p commented Mar 17, 2022

Describe the bug

When an executable uses the "paths" module exported by a library (due to the data-dir or data-files directive in the library's cabal file), the closure of that executable is very large. It includes packages such as GHC and its documentation.

Steps To Reproduce

Steps to reproduce the behavior:

  1. Clone the repo https://github.com/m4dc4p/nix-cabal-data-dir-surprise
  2. Run nix-store --query -R --include-outputs $(nix-build -A app).
  3. Notice closure is small & GHC does not appear.
  4. Run nix-store --query -R --include-outputs $(nix-build -A big-app).
  5. Closure has many more entries, including GHC (/nix/store/...-ghc-8.10.7) and its documentation (/nix/store/...-ghc-8.10.7-doc)

Expected behavior

I would not expect that using the exported "paths" module from a library would make the closure so much larger. This is pretty impactful when building docker images. In the above repo, the docker image for big-app is 425MB, while the docker image for app is only 24M:

$ ls -lah $(nix-build -A docker-big-app)
-r--r--r--  1 justin.bailey  admin   475M Dec 31  1969 /nix/store/0bvvq9sdw9bwiqbwyaidxj0wwkjmdif5-docker-image-docker-big-app.tar.gz
$ ls -lah $(nix-build -A docker-app)
-r--r--r--  1 justin.bailey  admin    26M Dec 31  1969 /nix/store/kbs7i25jnnlfcd0i9wxq68q3i829323m-docker-image-docker-app.tar.gz

Additional context

app/Main.hs conditionally imports the Paths_library module from the library package. If the module is imported and used, the closure blows up. This is due to a dependency found in the package.conf.d directory installed with library.

$ nix-store --query -R --include-outputs $(nix-build -A big-app) | grep ghc-8.10.7
/nix/store/4ycwcc6rmyx2bchr7zbigfiyklqdkfik-ghc-8.10.7-doc
/nix/store/qmcdw0bq6590082f3g6sqflqvlyvap5g-ghc-8.10.7
$ nix why-depends --precise $(nix-build -A big-app) /nix/store/4ycwcc6rmyx2bchr7zbigfiyklqdkfik-ghc-8.10.7-doc
/nix/store/j17x9cwaj4japr0mwk8nqvjgx7i6aysf-app-0.1.0.0
└───bin/app: …rld..library_datadir./nix/store/jkndj3lpq6256xyrx2miqglllpqiv4sm-library-0.1.0.0/share/x86_64-os…
    → /nix/store/jkndj3lpq6256xyrx2miqglllpqiv4sm-library-0.1.0.0
    └───lib/ghc-8.10.7/package.conf.d/library-0.1.0.0-4DahppUPAwH8P1w4JjXODY.conf: …dock-interfaces:.    /nix/store/qlcrbcy2la79hkz81wggdrwh720k7yhj-library-0.1.0.0-doc/share/doc/l…
        → /nix/store/qlcrbcy2la79hkz81wggdrwh720k7yhj-library-0.1.0.0-doc
        └───share/doc/library-0.1.0.0/html/Library.html: …> :: <a href="file:///nix/store/4ycwcc6rmyx2bchr7zbigfiyklqdkfik-ghc-8.10.7-doc/share/doc/ghc/ht…
            → /nix/store/4ycwcc6rmyx2bchr7zbigfiyklqdkfik-ghc-8.10.7-doc

The app executable does not have any dependency on GHC:

$ nix-store --query -R --include-outputs $(nix-build -A app) | grep ghc-8.10.7
< no output >
$

Note that the app and big-app derivations compile the same source, but big-app passes a CPP flag so that the Paths_library modules gets imported and used in app\Main.hs.

This bug has come up before, though not with the data-dir aspect, as NixOS/cabal2nix#539 and #155924.

Notify maintainers

@sternenseemann @maralorn

Metadata

 - system: `"x86_64-darwin"`
 - host os: `Darwin 21.3.0, macOS 10.16`
 - multi-user?: `no`
 - sandbox: `no`
 - version: `nix-env (Nix) 2.6.1`
 - channels(justin.bailey): `"nixpkgs, nixpkgs-21.11-darwin-21.11-darwin"`
 - nixpkgs: `/Users/justin.bailey/.nix-defexpr/channels/nixpkgs`
 - 

(Current nixpkgs revision is 3e644bd)

@m4dc4p m4dc4p changed the title Cabal/Haskell: Importing exports Use of data-dir leads to large closure Cabal/Haskell: Importing generated path module leads to large closure Mar 17, 2022
@m4dc4p
Copy link
Author

m4dc4p commented Mar 18, 2022

Setting enableSeparateDataOutput to true fixes the problem!

@m4dc4p
Copy link
Author

m4dc4p commented Mar 18, 2022

Strangely, changing data-dir to data-files (w/o the enableSeparateDataOutput flag) also fixes the problem. 🤷

@maralorn
Copy link
Member

I agree with the problem description.

I am sadly not sure if this issue is actionable. i.e. what could we change to fix this without breaking anything else?

@m4dc4p
Copy link
Author

m4dc4p commented Mar 18, 2022

I thought setting enableSeparateDataOutput to true might do it, but just learned that breaks the build if there is no data files defined.

Could cabal2nix be smart enough to recognize when data-dir or data-files are used and set the flag appropriately?

@sternenseemann
Copy link
Member

sternenseemann commented Mar 18, 2022

Could cabal2nix be smart enough to recognize when data-dir or data-files are used and set the flag appropriately?

It does.

Strangely, changing data-dir to data-files (w/o the enableSeparateDataOutput flag) also fixes the problem. shrug

You should always set data-files, in fact it's the primary way of doing data file distribution in cabal. data-dir is just the place where Cabal looks for data-files. More specifically, setting data-dir, but having an empty data-files does nothing.


That a reference to GHC is generated seems to be a weird edge case (maybe related to the fact that a Paths_ module is re-exposed?), I'll have to look into more.

@lf-
Copy link
Member

lf- commented Oct 5, 2022

enableSeparateDataOutput is not seeming to solve the closure size issue in my case, where I am also not even using the Paths_* modules (note that I have remove-references-to in place, and forgot to turn it off, but the references were there in the first place):

It's notable that warp-3.3.21-data has nothing in it, so I'm really not sure what's going on there.

 » strings result/bin/slacklinker | grep '/nix/store'
/nix/store/swwsjflmdpg5500xgcszkixsc3kc847q-slacklinker-0.0.0/etc
/nix/store/swwsjflmdpg5500xgcszkixsc3kc847q-slacklinker-0.0.0/libexec/aarch64-osx-ghc-9.2.4/slacklinker-0.0.0
/nix/store/zc4l6a1qql49qylzxa6hfdl5nyya7j2x-slacklinker-0.0.0-data/share/ghc-9.2.4/aarch64-osx-ghc-9.2.4/slacklinker-0.0
.0
/nix/store/swwsjflmdpg5500xgcszkixsc3kc847q-slacklinker-0.0.0/lib/ghc-9.2.4/aarch64-osx-ghc-9.2.4
/nix/store/swwsjflmdpg5500xgcszkixsc3kc847q-slacklinker-0.0.0/lib/ghc-9.2.4/aarch64-osx-ghc-9.2.4/slacklinker-0.0.0-2ts7
v7lYjTM1K3VbyyLQIs-slacklinker
/nix/store/swwsjflmdpg5500xgcszkixsc3kc847q-slacklinker-0.0.0/bin
/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-warp-3.3.21/etc
/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-warp-3.3.21/libexec/aarch64-osx-ghc-9.2.4/warp-3.3.21
/nix/store/770fab07qrnc41q7pmlv7dxw8v89c2jd-warp-3.3.21-data/share/ghc-9.2.4/aarch64-osx-ghc-9.2.4/warp-3.3.21
/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-warp-3.3.21/lib/ghc-9.2.4/aarch64-osx-ghc-9.2.4
/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-warp-3.3.21/lib/ghc-9.2.4/aarch64-osx-ghc-9.2.4/warp-3.3.21-KgMrgV3QBolBwpD4
ttB2Wl
/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-warp-3.3.21/bin
/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-hs-opentelemetry-sdk-0.0.3.3/etc
/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-hs-opentelemetry-sdk-0.0.3.3/libexec/aarch64-osx-ghc-9.2.4/hs-opentelemetry-
sdk-0.0.3.3
/nix/store/y03j5mpyvmv7r7ylfh4zjpsz64i3bzff-hs-opentelemetry-sdk-0.0.3.3-data/share/ghc-9.2.4/aarch64-osx-ghc-9.2.4/hs-o
pentelemetry-sdk-0.0.3.3
/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-hs-opentelemetry-sdk-0.0.3.3/lib/ghc-9.2.4/aarch64-osx-ghc-9.2.4
/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-hs-opentelemetry-sdk-0.0.3.3/lib/ghc-9.2.4/aarch64-osx-ghc-9.2.4/hs-opentele
metry-sdk-0.0.3.3-4okP8RUIhRPJ82kxo9f3AG
/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-hs-opentelemetry-sdk-0.0.3.3/bin

@edrex
Copy link
Contributor

edrex commented Nov 7, 2022

It looks as though this PR should allow replacing those paths imports for programs that just want to report version info for various dependencies/self: haskell/cabal#8534

It is in need of qualified reviewers (which I am not).

@Atemu
Copy link
Member

Atemu commented Nov 15, 2023

So, I'm still hitting this issue with git-annex. Exploring the nix-tree revealed that almost every package has a 2.something GiB closure.

@maralorn
Copy link
Member

maralorn commented Nov 15, 2023

Yeah, this is always the same issue, and there is no general fix for this:

  1. Packages use the Cabal generated Paths_ module. Often to get the package version.
  2. During the linking of a binary, some other (almost always unused) variables in that module that contain references to the store path of the package do not get removed.

This means that the binary contains a reference to the store path of the library, which contains references to a lot of stuff, including GHC.

Possible solutions:

  1. Find a way to do dead code elimination during linking. (I assume this is hard.)
  2. Patch our Cabal so that it does, by default, not create the fields that contain a store path reference. Make the old behavior restorable via an environment variable. (This is a good idea from @Profpatsch, but no one has implemented it or worked on a consensus that it’s the way to go).
  3. Since a few months ago, Cabal can generate a PackageInfo_ module, which does not contain store path references but contains a lot of useful information, which the Paths_ module previously has been used for. If libraries switch to using that module, this problem goes away.

Current workaround:

  1. Use strings on the binary to figure out which paths are referenced, confirm that it is this issue, by seeing the usual pattern of 6 times the same store path, as above in this thread.
  2. Add a postInstall phase that uses remove-references-to on every found store path to replace the store hash with e's.
  3. Add a disallowedRequisites = [pkgs.ghc] to the derivation to detect regressions.

The advantage of all solutions against the workaround is that all three have the possibility to actually figure out at compile time whether the store paths we don’t want referenced are save to remove.

@Atemu
Copy link
Member

Atemu commented Nov 15, 2023

2. (This is a good idea from @Profpatsch, but no one has implemented it or worked on a consensus that it’s the way to go).

We can always remove such hacks again. I think it'd be worth trying out after the release.

If libraries switch to using that module, this problem goes away.

Is that something each library must choose to do individually?

Add a postInstall phase that uses remove-references-to on every found store path to replace the store hash with e's.

Could we do that by default on all haskellPackages?

I checked again and all of git-annex' direct dependencies have a direct dependence on GHC. I would not be surprised if this issue affected >90% of haskellPackages.

@maralorn
Copy link
Member

Is that something each library must choose to do individually?

Yes, sadly.

Could we do that by default on all haskellPackages?

No, because it means one remove-references-to call for every unwanted dependency. I mean we could theoretically "grep through" the resulting binary to somehow detect these cases, but that feels terribly brittle, and because it can theoretically cause runtime errors, this workaround should be opt-in.

I checked again and all of git-annex' direct dependencies have a direct dependence on GHC. I would not be surprised if this issue affected >90% of haskellPackages.

Maybe you are misunderstanding the problem, maybe you don’t. Either way, I am gonna explain:

git-annex, like every package, has build-time dependencies and runtime dependencies. Nix tracks those independently. Build-time dependencies are (the runtime closure of) all store paths mentioned in the .drv. Runtime dependencies are all build-time dependencies that are mentioned in an output of the build.

git-annex, like basically every Haskell package, has a lot of Haskell libraries as build-time dependencies. Generally, all Haskell libraries have a huge runtime closure because they reference all their dependencies and our ghc package, which is quite huge. Thus, all of git-annex direct dependencies having a huge closure is not the core issue here. It is to be expected.

The aim of justStaticExecutables is to create a binary that has none of its Haskell build-time dependencies as runtime dependencies. Thus the issue is, that git-annex has direct runtime dependencies in the first place. That is (probably) caused by the described bug here and can be solved like described above, where the solutions 2. and 3. would fix the problem on the side of all libraries in the git-annex closure that cause the bug (by preventing store-path references to itself in the compiled library), where solution 1 and the workaround fix the problem by preventing the inclusion of that references in the resulting git-annex binary. Thus the workaround can be applied locally in the git-annex package.

@maralorn
Copy link
Member

Hm, after rereading your comment and my message, my message does not feel as clear, as it could be.

You cannot detect whether a Haskell library triggers this bug by looking at its runtime closure. As I said, the closure is expected to be huge. A Haskell library triggers this bug, if it contains a store-path reference to itself in its compilation output. (A library might also contain self-references in other parts of its outputs, e.g., in the docs, but that’s not an issue because those don’t get linked into the final binary.) I assume you could try to figure that out with the strings utility, but I have never tried.

sternenseemann added a commit to 9999years/nixpkgs that referenced this issue May 27, 2024
This makes `justStaticExecutables` error if the produced store path
contains references to GHC. This is almost always erroneous and due to
the generated `Paths_*` module being imported. This helps prevent
`justStaticExecutables` from producing binaries with closure sizes in
the gigabytes.

See: NixOS#164630

Co-authored-by: sternenseemann <[email protected]>
sternenseemann added a commit that referenced this issue May 27, 2024
This makes `justStaticExecutables` error if the produced store path
contains references to GHC. This is almost always erroneous and due to
the generated `Paths_*` module being imported. This helps prevent
`justStaticExecutables` from producing binaries with closure sizes in
the gigabytes.

See: #164630

Co-authored-by: sternenseemann <[email protected]>
sternenseemann pushed a commit that referenced this issue Jun 12, 2024
This makes `justStaticExecutables` error if the produced store path
contains references to GHC. This is almost always erroneous and due to
the generated `Paths_*` module being imported. This helps prevent
`justStaticExecutables` from producing binaries with closure sizes in
the gigabytes.

See: #164630

Co-authored-by: sternenseemann <[email protected]>

(cherry picked from commit d261882)
(minus release note)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants