-
Notifications
You must be signed in to change notification settings - Fork 689
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
Add --dynlibdir
#3979
Add --dynlibdir
#3979
Conversation
@christiaanb, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ezyang, @23Skidoo and @dcoutts to be potential reviewers. |
dd9e6c2
to
2076850
Compare
Thanks @christiaanb! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good modulo minor comments.
import Distribution.Text | ||
import qualified Distribution.Compat.Graph as Graph | ||
|
||
import Data.List (stripPrefix) | ||
import System.FilePath | ||
import qualified Data.Map as Map | ||
|
||
import System.Directory (doesDirectoryExist, canonicalizePath) | ||
import System.Directory (doesDirectoryExist, canonicalizePath, getDirectoryContents) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting seems off.
@@ -273,6 +274,19 @@ depLibraryPaths inplace relative lbi clbi = do | |||
|
|||
allDepLibDirs' = internalLibs ++ allDepLibDirs | |||
allDepLibDirsC <- traverse canonicalizePathNoFail allDepLibDirs' | |||
-- Only get the directories that contain dynamic libraries | |||
let soExtension = case buildOS of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use dllExtension
from D.S.BuildPaths
here.
2076850
to
389039a
Compare
@christiaanb Will there be a Sierra solution for the static libraries too? |
@ilovezfs There is a problem with static libraries on Sierra???? |
Probably not ... just trying to get git-annex to build in a cabal sandbox using this PR and still getting stuck at yesod-auth, but I'm probably doing something wrong. |
Closing this PR, as it lead to problems with RPATH calculations in GHC. The definitive solution for https://ghc.haskell.org/trac/ghc/ticket/12479 should be #3982. I'm terribly sorry that it took 3 PRs to fix this. |
Oh, you shouldn't be. We are all very grateful to you for working so hard on this. |
@@ -266,24 +268,37 @@ depLibraryPaths inplace relative lbi clbi = do | |||
-} | |||
getLibDir sub_clbi | |||
| inplace = componentBuildDir lbi sub_clbi | |||
| otherwise = libdir (absoluteComponentInstallDirs pkgDescr lbi (componentUnitId sub_clbi) NoCopyDest) | |||
| otherwise = dynlibdir (absoluteComponentInstallDirs pkgDescr lbi (componentUnitId sub_clbi) NoCopyDest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get this bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to resolve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that entire function is there to find the locations of dynamic libraries. Since libdir
now only contains the location of static
libraries, we should use dynlibdir
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep ok
So we're planning to resurrect this approach and make ghc do the right thing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that much to do to get this working with the new approach based on using dynamic-library-dirs
.
then libdir installDirs : extraLibDirs bi | ||
else extraLibDirs bi, | ||
then libdir installDirs : | ||
dynlibdir installDirs : extraLibDirs bi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bit will need to be extended to set the libraryDynDirs
if that's supported by the target compiler, and otherwise put both libdir and dynlibdir in the libraryDirs
as the patch does now.
@@ -779,18 +779,20 @@ tests config = do | |||
uid = componentUnitId (targetCLBI target) | |||
dir = libdir (absoluteComponentInstallDirs pkg_descr lbi uid | |||
NoCopyDest) | |||
dyndir = dynlibdir (absoluteComponentInstallDirs pkg_descr lbi | |||
uid NoCopyDest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should share this better, e.g.
InstallDirs{libdir,dynlibdir} = absoluteComponentInstallDirs pkg_descr lbi uid NoCopyDest
internalLibsC <- traverse canonicalizePathNoFail internalLibs | ||
|
||
let allDepLibDirs' = internalLibsC ++ allDepLibDirsC | ||
p = prefix installDirs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So all this stuff can be reverted.
@@ -266,24 +268,37 @@ depLibraryPaths inplace relative lbi clbi = do | |||
-} | |||
getLibDir sub_clbi | |||
| inplace = componentBuildDir lbi sub_clbi | |||
| otherwise = libdir (absoluteComponentInstallDirs pkgDescr lbi (componentUnitId sub_clbi) NoCopyDest) | |||
| otherwise = dynlibdir (absoluteComponentInstallDirs pkgDescr lbi (componentUnitId sub_clbi) NoCopyDest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to resolve this.
Other |
389039a
to
6be6365
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really good.
@@ -266,24 +268,37 @@ depLibraryPaths inplace relative lbi clbi = do | |||
-} | |||
getLibDir sub_clbi | |||
| inplace = componentBuildDir lbi sub_clbi | |||
| otherwise = libdir (absoluteComponentInstallDirs pkgDescr lbi (componentUnitId sub_clbi) NoCopyDest) | |||
| otherwise = dynlibdir (absoluteComponentInstallDirs pkgDescr lbi (componentUnitId sub_clbi) NoCopyDest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep ok
comp = compiler lbi | ||
supportsLibraryDynDir = case compilerFlavor comp of | ||
GHC -> compilerVersion comp > mkVersion [8,0,1] | ||
_ -> False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now try to avoid sprinkling ghc version tests throughout the code. They now all live in the GhcImplInfo
record (from Distribution.Simple.GHC.ImplInfo
) or for (g)hc-pkg they live in the HcPkgInfo
record from Distribution.Simple.Program.HcPkg
.
So initially I was going to say that this is a hc-pkg feature, and so it should be defined as a new field in HcPkgInfo
, e.g. supportsLibDynDirs
. However, I've changed my mind because it will be quite awkward since we cannot trivially get the HcPkgInfo
value for the compiler, since not all of them support a hc-pkg.
@23Skidoo what do you reckon? In principle, perhaps this belongs in the Compiler
value (from D.Simple.Compiler
) since this question is valid whether or not the compiler uses a hc-pkg program. Indeed perhaps the Compiler
type should contain a Maybe HcPkgInfo
.
@christiaanb so if @23Skidoo agrees with me then I think you'd add this as a Bool
feature of the Compiler
type from Distribution.Simple.Compiler
, and set it appropriately in the per-compiler configure functions.
Alternatively, could follow the approach of your TODO above and add per-compiler InstalledPackageInfo -> InstalledPackageInfo munging functions. Indeed we might be able to move the ABI stuff into those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd make libDynDirsSupported
a function of type Compiler -> Bool
instead of a field, which seems to be more in line with existing precedent (see profilingSupported
and coverageSupported
in D.S.Compiler
). The alternative is probably also fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a function D.S.Compiler.supportsLibraryDynDir
in the latest version of the PR.
@@ -390,11 +391,15 @@ filterConfigureFlags flags cabalLibVersion | |||
configAllowNewer = Just (Cabal.AllowNewer Cabal.RelaxDepsNone) | |||
} | |||
|
|||
-- Cabal < 1.25.0 doesn't know about --dynlibdir. | |||
flags_1_25_0 = flags_latest { configInstallDirs = configInstallDirs_1_25_0} | |||
configInstallDirs_1_25_0 = (configInstallDirs flags) { dynlibdir = NoFlag } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@23Skidoo I'm never too confident about the logic here. Can you double check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filterConfigureFlags
changes are solid IMO.
@@ -423,7 +428,7 @@ filterConfigureFlags flags cabalLibVersion | |||
-- Cabal < 1.18.0 doesn't know about --extra-prog-path and --sysconfdir. | |||
flags_1_18_0 = flags_1_19_1 { configProgramPathExtra = toNubList [] | |||
, configInstallDirs = configInstallDirs_1_18_0} | |||
configInstallDirs_1_18_0 = (configInstallDirs flags) { sysconfdir = NoFlag } | |||
configInstallDirs_1_18_0 = (configInstallDirs flags_1_19_1) { sysconfdir = NoFlag } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an independent bug fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't a "bug" until I added another configInstallDirs
restriction. I mean, flags
is the completely unrestricted flags, we should be using the restricted version: flags_1_19_1
.
then dynlibdir installDirs : | ||
extraLibDirs bi | ||
else extraLibDirs bi | ||
else [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be easier to pull these out to the where clause (like the absinc, relink) and share the logic, e.g.
(libdir, dynlibdir)
| not hasLibrary
= (extraLibDirs bi, [])
-- the dynamic-library-dirs defaults to the library-dirs if not specified,
-- so this works if the dynamic-library-dirs field is supported or not
| supportsLibraryDynDir comp
= (libdir installDirs : extraLibDirs bi,
dynlibdir installDirs : extraLibDirs bi)
| otherwise
= (libdir installDirs : dynlibdir installDirs : extraLibDirs bi, [])
-- the compiler doesn't understand the dynamic-library-dirs field so we
-- add the dyn directory to the "normal" list in the library-dirs field
but do double check I've transcribed the logic correctly! :-)
-- more compilers start to support this, we should drop the | ||
-- `supportsLibraryDynDir` check here, and just handle `IPI.libraryDynDirs` | ||
-- in the actual package registration function belonging to those particular | ||
-- compilers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately we cannot do this yet given the structure of the code. We need to be able to generate the InstalledPackageInfo so we can write it to a file or script, without ending up calling the compiler specific registerPackage
. What would be nice is if we had a compiler specific bit of generating the registration info, then we could do this compat bit there.
FYI, doing that might be an alternative to the thing I suggest below about compiler version tests.
6be6365
to
123018a
Compare
AppVeyor failure seems to be real. |
123018a
to
7bddd44
Compare
It looks like this regressed tests on GHC HEAD https://travis-ci.org/haskell/cabal/jobs/168959154 |
@ezyang Yes... this patch expects https://phabricator.haskell.org/D2611 to be merged into HEAD. What is the proper way to handle this? |
@christiaanb I think we can use the same approach as in a088119 - change the version check from |
@@ -321,6 +322,13 @@ unitIdSupported = ghcSupported "Uses unit IDs" | |||
backpackSupported :: Compiler -> Bool | |||
backpackSupported = ghcSupported "Support Backpack" | |||
|
|||
-- | Does this compiler support a package database entry with: | |||
-- "dynamic-library-dirs"? | |||
supportsLibraryDynDir :: Compiler -> Bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bikeshedding: libraryDynDirSupported
would be more in line with existing precedent (see above).
Though I don't get why it's failing with 8.0.1 if the dynlibdir code path is > 8.0.1 only. |
It fails on the 8.0.1 because that travis build has |
`--dynlibdir` indicates the directory in which dynamic libraries are installed. By default this setting is equal to: `$libdir/$abi` The static libraries will still end up in: `$libdir/$libsubdir` With `$libsubdir/$abi` as the default directory for dynamic libraries, dynamic libraries will by default end up in a single shared directory (per package database). This has the potential to reduce start-up times for dynamically linked executable as only one RPATH per package database will be needed. This commit uses the functionality defined in https://phabricator.haskell.org/D2611 to tell GHC's > 8.0.1 package database that dynamic libraries are copied to the directories mentioned in the `dynamic-library-dirs` field.
7bddd44
to
d2da655
Compare
Merged, thanks! |
OK, why is CI still broken if the version bound is now correct? |
It's because it was testing >= 8.0.1.20161021, but 8.1 nightlies are always greater than that, even without the fix. I pushed a fix. |
Gah, I could've guessed. |
--dynlibdir
indicates the directory in which dynamic librariesare installed. By default this setting is equal to:
$libdir/$abi
The static libraries will still end up in:
$libdir/$libsubdir
With
$libsubdir/$abi
as the default directory for dynamiclibraries, dynamic libraries will by default end up in a
single shared directory (per package database). This has the
potential to reduce start-up times for dynamically linked
executable as only one RPATH per package database will be
needed.