Skip to content

Weird store path dir #9654

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

Closed
alt-romes opened this issue Jan 24, 2024 · 8 comments
Closed

Weird store path dir #9654

alt-romes opened this issue Jan 24, 2024 · 8 comments
Assignees

Comments

@alt-romes
Copy link
Collaborator

Describe the bug
Non-local build packages are built in a temporary location within the cabal store.
Weirdly, this temporary location duplicates the cabal-store-path-prefix within the temporary directory, something like

/home/tchoutri/.cabal/store/ghc-9.4.7/incoming/new-3485596/home/tchoutri/.cabal/store/ghc-9.4.7/openapi3-3.2.4-10ab5f1c59bbb45dbd67f02e0bbc49fff86fce693481f1e870150b3d6fa049d1/bin

This was also noted in #9640.

To Reproduce
Steps to reproduce the behavior

cd $(mktemp -d)
cabal init --lib -m --simple -d hegg
cabal --store-dir=$(mktemp -d) build -v2 | grep incoming/new-

Prints out something like

/var/folders/tv/35hlch6s3y15hfvndc71l6d40000gn/T/tmp.BWFEH5jATm/ghc-9.8.1/incoming/new-45541/var/folders/tv/35hlch6s3y15hfvndc71l6d40000gn/T/tmp.BWFEH5jATm/ghc-9.8.1/hgg-0.5.0.0-db29a4ad

Expected behavior
The incoming package build path should not duplicate the cabal store prefix within it.

System information

  • cabal HEAD
@Mikolaj
Copy link
Member

Mikolaj commented Jan 24, 2024

I wonder if that's a silly mistake or if it (awkwardly) has some useful role, e.g., to make sure something is distinct from something else even in degenerate circumstances, e.g., with empty path prefixes somewhere. Whatever the case, I'm sure the extra path segment can be made shorter.

@alt-romes
Copy link
Collaborator Author

In my short investigation I think this was introduced by 5add3c9 (cc @angerman).

@alt-romes
Copy link
Collaborator Author

In fact, this is a bit harder to pin point. I no longer think the root cause is 5add3c9, sorry for the ping Moritz.
I'm working on it.

@alt-romes
Copy link
Collaborator Author

alt-romes commented Jan 24, 2024

The result of a lengthier investigation.

When compiling a non-local dependency, we will configure, build, and copy the package into a temporary store, before finally copying atomically the entry in the temporary store to the final store.

  1. When configuring, we will specify all of --prefix, --dynlibdir, --libdir, etc... flags based on a InstallDirs value.
  2. We configure --prefix to be the store directory (the one specified to cabal with --store-dir) plus the unit id, i.e. $store/$unitid. This sets the prefix used during the installation.
  3. We configure --libdir and friends with the usual unix directories such as $store/$unitid/lib, $store/$unitid/share/doc, etc.
    3.1. However, for macos, we configure --dynlibdir to be $store/lib (note: no $unitid). We want to install all different dynamic libraries in directory of the store, instead of having one different directory per dynamic library (namely, $store/$unitid/lib). The reason is that macOS load command has a low limit, and putting all dyn libraries into 1 directory means our load only needs to contain that directory for runtime dynamic libs.
  4. At copy time, the function newStoryEntry will invoke the Setup copy command by calling the copyFiles higher order parameter, which will mostly always be just copyPkgFiles in UnpackedPackage.
  5. The copy command will be passed --destdir=$store/incoming/new-tmpno. This means that copy will write to the the dest dir ($store/incoming/new-tempno) the package artifacts as per the configuration (which specified the install dirs with --prefix to be $store/$unitid), i.e. copy will write to $store/incoming/new-tmpno/$store/$unitid/...
    5.1. Except for dynlibs on macos which will be written to $store/incoming/new-tmpno/$store/lib (no unitid).
  6. copyPkgFiles then returns the path to where the files were copied, which it does by recomputing the appending of $store/$unitid to tmpdir=$store/incoming/new-tmpno.
    6.1. copyPkgFiles on macos additionally returns the full path to each dynlib written to that special lib dir.
  7. newStoreDir takes the destination of copying the package from copyPkgFiles, and maybe the additional dynlib files, and, if it wins the race to write to the final store dir (note that so far we have been writing to a temporary dir within the store dir), it will atomically rename the copy-result-dir ($store/incoming/new-xxx/$store/$unitid) to the final store dir $store/$unitid (note, no incoming/new-xxx).
    8.1. Lastly, we copy the remaining files from the macos $store/incoming/new-xxx/$store/lib special lib dir to the final store $store/lib.

What a journey...

I'll see how to fix this tomorrow.

@alt-romes alt-romes self-assigned this Jan 25, 2024
@alt-romes
Copy link
Collaborator Author

There is some prior discussion about this interaction of --prefix and --store-dir e.g. in #3473 (comment).

@alt-romes
Copy link
Collaborator Author

alt-romes commented Feb 1, 2024

It turns out that because the --prefix and flags like --bindir or --libdir may not share said prefix, it is not obvious how we could write into the store dir while ignoring the prefix or bindir or libdir, since these paths basically lay out the directory struture of the dist directory.

If --binddir or --libdir were always relative to the --prefix then we could always build in $(--destdir)/temp according to that directory hierarchy, and then relocate/copy to --prefix. But that is not the case.

Since it isn't necessarily a problem to write into destdir/tmp the configured prefix and other libs or bin directories, we just leave it as is. Nothing to be done or fixed. It would indeed be a bit cleaner to not have this duplication in the path, but that is not possible under the circumstances

@ulidtko
Copy link
Contributor

ulidtko commented Mar 3, 2024

At the very least, it does look buggy to double the path length, /home/user/.cabal/store/ghc-.../incoming/new-.../home/user/.cabal/store/ghc-.../actual-pkg-dir/bin. @alt-romes are you saying this is as expected?

@alt-romes
Copy link
Collaborator Author

@ulidtko it's unfortunate but there is no easy solution here (changing this would imply changing many other things and break backwards compatibility). It would be cleaner not to duplicate the path in the tmp installation dir, but this is not cause for problems -- not a bug.

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

3 participants