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.

Sign up for GitHub

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

Fix path redundancy new-build output folders. #4978

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Fix path redundancy new-build output folders. #4978

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Cabal/Distribution/Simple/InstallDirs.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ import Distribution.Text
import System.Directory (getAppUserDataDirectory)
import System.FilePath
( (</>), isPathSeparator
, pathSeparator, dropDrive )
, pathSeparator, takeBaseName )

#ifdef mingw32_HOST_OS
import qualified Prelude
Expand Down Expand Up @@ -286,7 +286,7 @@ absoluteInstallDirs :: PackageIdentifier
-> InstallDirs FilePath
absoluteInstallDirs pkgId libname compilerId copydest platform dirs =
(case copydest of
CopyTo destdir -> fmap ((destdir </>) . dropDrive)
CopyTo destdir -> fmap ((destdir </>) . takeBaseName)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is right. Here you take InstallDirs and substitute all absolute paths by destdir </> basename absPath. E.g. if libdir was $prefix/lib/$abi/$libname, you make it $destdir/$libname.

Copy link
Member

Choose a reason for hiding this comment

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

Though it seems to me that it's underspecified what copy --destdir should mean. I'd expect the value of --destdir to replace $prefix, but apparently it just gets prepended to the full absolute path including $prefix minus the drive letter.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like there was a --copy-prefix feature that actually worked that way, but it was removed ages ago (981473b).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm thanks! that's a great hint. I've also managed to reproduce it on Linux so hopefully will have a fix today.

_ -> id)
. appendSubdirs (</>)
. fmap fromPathTemplate
Expand Down
10 changes: 4 additions & 6 deletions cabal-install/Distribution/Client/ProjectBuilding.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -940,12 +940,10 @@ buildAndInstallUnpackedPackage verbosity
annotateFailure mlogFile InstallFailed $ do

let copyPkgFiles tmpDir = do
setup Cabal.copyCommand (copyFlags tmpDir)
-- Note that the copy command has put the files into
-- @$tmpDir/$prefix@ so we need to return this dir so
-- the store knows which dir will be the final store entry.
let prefix = dropDrive (InstallDirs.prefix (elabInstallDirs pkg))
let prefix = let path = InstallDirs.prefix (elabInstallDirs pkg)
in takeBaseName path
entryDir = tmpDir </> prefix
setup Cabal.copyCommand (copyFlags entryDir)
LBS.writeFile
(entryDir </> "cabal-hash.txt")
(renderPackageHashInputs (packageHashInputs pkgshared pkg))
Expand All @@ -954,7 +952,7 @@ buildAndInstallUnpackedPackage verbosity
-- While this breaks the prefix-relocatable property of the lirbaries
-- it is necessary on macOS to stay under the load command limit of the
-- macOS mach-o linker. See also @PackageHash.hashedInstalledPackageIdVeryShort@.
otherFiles <- filter (not . isPrefixOf entryDir) <$> listFilesRecursive tmpDir
otherFiles <- filter (not . isPrefixOf entryDir) <$> listFilesRecursive tmpDir
-- here's where we could keep track of the installed files ourselves
-- if we wanted to by making a manifest of the files in the tmp dir
return (entryDir, otherFiles)
Expand Down
5 changes: 4 additions & 1 deletion cabal-install/Distribution/Client/Store.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,10 @@ newStoreEntry verbosity storeDirLayout@StoreDirLayout{..}
-- Atomically rename the temp dir to the final store entry location.
renameDirectory incomingEntryDir finalEntryDir
forM_ otherFiles $ \file -> do
let finalStoreFile = storeDirectory compid </> makeRelative (incomingTmpDir </> (dropDrive (storeDirectory compid))) file
let finalStoreFile = storeDirectory compid
</> makeRelative (incomingTmpDir
</> (takeBaseName (storeDirectory compid)))
file
createDirectoryIfMissing True (takeDirectory finalStoreFile)
renameFile file finalStoreFile

Expand Down
2 changes: 2 additions & 0 deletions cabal-install/changelog
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@
* Added support for '--enable-tests' and '--enable-benchmarks' to
'cabal fetch' (#4948).
* Removed support for building cabal-install with GHC < 7.10.
* Redundancy in new-build incoming store paths removed. This makes incoming
tmp paths much shorter. (#4515, #3972)

2.0.0.1 Mikhail Glushenkov <[email protected]> December 2017
* Support for GHC's numeric -g debug levels (#4673).
Expand Down