Build dynamic libs #325
Build dynamic libs #325
Conversation
|
It can successful compile a hello-world dynamically now: |
|
@izgzhen Whoa, hang on, you're moving too fast! You were supposed to start the Summer of Haskell yesterday, but you already resolved the two main issues :D I'll do my best to review both PRs tomorrow -- today's is a bit busy. |
:-) Take your time, thanks |
src/Settings/Flavours/Quickest.hs
Outdated
| , libraryWays = append [vanilla] | ||
| , libraryWays = mconcat | ||
| [ append [vanilla] | ||
| , notStage0 ? platformSupportsSharedLibs ? append [dynamic] ] |
There was a problem hiding this comment.
The quickest flavour is supposed to be the quickest one that produces a working GHC, to be used by CI bots and avoid time limits, so I suggest we stick to pure vanilla.
There was a problem hiding this comment.
But this is not consistent with the behavior of quickest flavor specified in mk/build.mk. I remember that dynamic libs will be produced in that one. Will it be a surprise if we redefine this?
There was a problem hiding this comment.
The difference between quick and quickest in the Make build system seemed too marginal to me, so I thought it would be better to deviate from the existing definition. The quickest flavour should be the one that can't be beaten in terms of speed or else why name it this way :)
At the moment, quickest in Hadrian is already different, so I suggest not to try to change this in this PR. This discussion deserves a separate issue.
src/Rules.hs
Outdated
| @@ -61,6 +61,10 @@ packageRules = do | |||
| [ Rules.Compile.compilePackage readPackageDb | |||
| , Rules.Library.buildPackageLibrary ] | |||
|
|
|||
| let dynContexts = liftM3 Context [Stage1 ..] knownPackages [dynamic] | |||
There was a problem hiding this comment.
I am not a fan of using contractions. Can we use dynamic throughout the code instead of dyn? This will be more consistent with the rest of Hadrian.
| mconcat [ arg "-no-auto-link-packages" | ||
| mconcat [ (Dynamic `wayUnit` way) ? | ||
| append [ "-shared", "-dynamic", "-dynload", "deploy" ] | ||
| , arg "-no-auto-link-packages" |
There was a problem hiding this comment.
I'm confused why -no-auto-link-packages is not included in the above append.
There was a problem hiding this comment.
Isn't it still on the line after the append? Both static and dynamic needs that flag I think.
There was a problem hiding this comment.
Ah, my bad! Please disregard this comment.
|
Nice, looks a lot simpler than I imagined :-) I've added a few minor comments. Does this fully solve #4? I remember varous mysterious settings like |
|
By the way, Linux CI fails. |
Ah, my bad, |
Including Windows, I will work on the platform thing soon |
Well, I found that it is hard to test Windows-version on my machine for now. Can I leave it for future? |
Yes, sure. |
Relevant wiki:
Looks like this issue is around Then how should I define this PR? "Build dynamic libs for GHC executable compilation"? |
|
BTW, I should have fixed the Linux dynamic lib. The weird bit is, for object like |
Yes, sounds good.
Works for me! |
Yes, I never found out what was the reason to use GHC for compiling C files, so I decided to use |
|
I pushed the patch to follow the upstream change of dropping Also, it should really work now, but both Linux and OS X CI timeout due to the lengthy process of building dynamic libs. |
|
@izgzhen I'd prefer a separate PR for dropping
So, let's remove |
Done ( |
|
@izgzhen Thanks! Shall I merge this one too? |
src/Settings/Builders/Cc.hs
Outdated
| [ append =<< getPkgDataList CcArgs | ||
| , argSettingList . ConfCcArgs =<< getStage | ||
| , cIncludeArgs | ||
|
|
||
| , builder (Cc CompileC) ? mconcat [ arg "-Werror" | ||
| , (Dynamic `wayUnit` way) ? | ||
| mconcat [ arg "-fPIC" |
There was a problem hiding this comment.
Should this be append ["-fPIC", "-DDYNAMIC"]?
src/Rules/Library.hs
Outdated
|
|
||
| -- This will create split objects if required (we don't track them | ||
| -- explicitly as this would needlessly bloat the Shake database). | ||
| need $ noHsObjs ++ hsObjs |
There was a problem hiding this comment.
Can we get rid of this code duplication?
There was a problem hiding this comment.
See buildPackageLibrary below, which seems to have a lot of overlap with this build rule.
There was a problem hiding this comment.
Yes there is code duplication since buildDynamicLib is adapted from buildPackageLibrary. Before that I am not sure if I should refactor or not, but let's refactor it.
|
I've added a couple of more comments after looking through the code again. |
I am ready now, you may merge if as you see fit. |
|
Thanks! Merged. |
Solve #4