Use Cabal directly in place of ghc-cabal + make build root configurable #531

alpmestan · 2018-03-19T14:10:57Z

This commit implements two significant changes (that were not easy to
separate):

Don't use ghc-cabal anymore for getting information about Haskell packages.
We now instead directly use Cabal-the-library.
Make the build root configurable. This effectively gets rid of the inplace
logic and allows us to place all build artefacts in some directory of
our choice, by passing '--build-root ' to hadrian. The GHCs we
produce are now relocatable.

The code for this was mostly taken from #445. I have successfully built functional quick/prof/perf/devel2/quick-cross builds of GHC with this branch. I might commit
another tweak or two as I'm now testing the test/docs rules. But I'm not expecting
major changes there.

I'm also not clear on what we want to do with the install/wrapper rules, with
relocatable GHC builds.

izgzhen · 2018-03-19T15:16:37Z

src/Base.hs

+    hadrianPath, configPath, configFile, sourcePath, shakeFilesDir,
+    generatedDir, generatedPath,
+    stageBinPath, stageLibPath,
+templateHscPath, ghcDeps,


nit: indentation

izgzhen · 2018-03-19T15:21:05Z

I found some remaining bits by grepping inplace, are they relevant now?

src/Base.hs:-- | Path to the inplace package database used in 'Stage1' and later.
src/Context.hs:-- | Path to inplace package configuration file of a given 'Context'.
src/Context.hs:    return $ path -/- "inplace-pkg-config"                           
src/GHC.hs:             return (top -/- "inplace/mingw/bin/strip.exe")              
src/Rules/PackageData.hs:    dir -/- "inplace-pkg-config" %> \conf -> do            
src/Rules/Program.hs:          -- Rules for the GHC package, which is built 'inplace'

izgzhen · 2018-03-19T15:40:29Z

src/UserSettings.hs

@@ -18,7 +16,7 @@ import {-# SOURCE #-} Settings.Default

 -- | All build results are put into the 'buildRoot' directory.
 userBuildRoot :: BuildRoot
-userBuildRoot = BuildRoot "_build"
+userBuildRoot = error "build root not set" -- BuildRoot "_build"


I think it is better to use the default value here, since we should pass in --build-root explicitly if we want to change it.

Oh yes, that's just a leftover from some debugging I did. Nice catch :-)

Looks like CI bots are broken by this error.

I hope this PR will actually fix the recent series of breakages due to ghc-cabal, so ideally we should be getting green light from CI :)

izgzhen · 2018-03-19T15:42:13Z

it told me Warning: No want/action statements, nothing to do when I run ./build.sh, why?

alpmestan · 2018-03-19T15:43:03Z

it told me Warning: No want/action statements, nothing to do when I run ./build.sh, why?

It tweaks the toplevel rules and surely changes the default behaviour, you can try "stage2" as a target if you want. We'll probably want to change this a little.

izgzhen · 2018-03-19T15:55:46Z

src/CommandLine.hs

@@ -36,6 +38,7 @@ defaultCommandLineArgs = CommandLineArgs
    , progressColour = Auto
    , progressInfo   = Brief
    , splitObjects   = False
+    , buildRoot      = UserSettings.userBuildRoot


BTW, I think you can just move the _build from UserSettings to here.

Indeed, one less hoop to go through to figure out the default value. I'll make that change in an upcoming commit.

izgzhen · 2018-03-19T15:57:20Z

It tweaks the toplevel rules and surely changes the default behaviour, you can try "stage2" as a target if you want. We'll probably want to change this a little.

Thanks. You can try updating the README to reflect the changes that will effect users as well :)

alpmestan · 2018-03-19T16:02:28Z

Thanks. You can try updating the README to reflect the changes that will effect users as well :)

Oh yes, I definitely plan to augment the README with a section about --build-root, and possibly more depending on what we decide for the toplevel rules. I just would like to fix at least docs (done since a few minutes ago) & test rules before that.

izgzhen · 2018-03-20T02:21:24Z

src/Builder.hs

@@ -139,8 +139,8 @@ builderProvenance = \case
    GhcCabal _ _     -> context Stage1 ghcCabal
    GhcPkg _ Stage0  -> Nothing
    GhcPkg _ _       -> context Stage0 ghcPkg
-    Haddock _        -> context Stage2 haddock
-    Hpc              -> context Stage1 hpcBin
+    Haddock _        -> context Stage1 haddock


Did we change the semantics of StageN in your PR?

We shouldn't be changing the semantics of stages in this PR. I remember @angerman had some arguments for doing this and there were also some arguments against. We should do this separately and after a discussion in a separate issue, since it's not a trivial decision.

izgzhen · 2018-03-20T02:32:51Z

src/Settings/Builders/Haddock.hs

-            [ arg "--verbosity=0"
+            [ arg $ "-B" ++ root -/- "stage1" -/- "lib"
+            , arg $ "--lib=" ++ root -/- "lib"
+            , arg "--verbosity=0"


nit: add new arg below it for a smaller diff

izgzhen · 2018-03-20T02:35:23Z

a weird directory appeared in ghc after building, what is that?

-> % tree path
path
└── to
    └── ghc-split

izgzhen · 2018-03-20T05:01:11Z

Regarding the docs target:

-> % hadrian/build.sh -j docs
Old pre cabal 2.1 version detected. Falling back to legacy 'cabal sandbox' mode.
Preprocessing executable 'hadrian' for hadrian-0.1.0.0..
Building executable 'hadrian' for hadrian-0.1.0.0..
Running hadrian...
| Run Sphinx Html: docs/users_guide => _build/docs/html/users_guide
| Run Sphinx Latex: utils/haddock/doc => /tmp/extra-dir-48015799677772
| Run Sphinx Latex: docs/users_guide => /tmp/extra-dir-48015799677773
| Run Tar Create: _build/docs/html/Haddock => _build/docs/archives/Haddock.html.tar.xz
Warning: libraries/text/text.cabal:4:1: The field "bug-reports" is specified
more than once at positions 4:1, 42:1
Warning: libraries/terminfo/terminfo.cabal:4:1: The field "category" is
specified more than once at positions 4:1, 10:1
| Run Xelatex: Haddock.tex => /tmp/extra-dir-48015799677772
This is XeTeX, Version 3.14159265-2.6-0.99992 (TeX Live 2015/Debian) (preloaded format=xelatex)
 restricted \write18 enabled.
entering extended mode
(./Haddock.tex
LaTeX2e <2016/02/01>
Babel <3.9q> and hyphenation patterns for 3 language(s) loaded.
(./sphinxmanual.cls
Document Class: sphinxmanual 2009/06/02 Document class (Sphinx manual)
(/usr/share/texlive/texmf-dist/tex/latex/base/report.cls
Document Class: report 2014/09/29 v1.4h Standard LaTeX document class
(/usr/share/texlive/texmf-dist/tex/latex/base/size10.clo)))
(/usr/share/texlive/texmf-dist/tex/latex/base/inputenc.sty

Package inputenc Warning: inputenc package ignored with utf8 based engines.

)
! Undefined control sequence.
<recently read> \DeclareUnicodeCharacter 
                                         
l.5 \DeclareUnicodeCharacter
                            {00A0}{\nobreakspace}
No pages of output.
Transcript written on Haddock.log.
shakeArgsWith   0.000s    0%                           
Function shake  8.277s   77%  =========================
Database read   0.393s    3%  =                        
With database   0.027s    0%                           
Running rules   2.041s   19%  ======                   
Total          10.737s  100%                           
Error when running Shake build system:
* docs
* _build/docs/pdfs/Haddock.pdf
user error (Development.Shake.cmd, system command failed
Command: /usr/bin/xelatex -halt-on-error Haddock.tex
Current directory: /tmp/extra-dir-48015799677772
Exit code: 1
Stderr:
)

izgzhen · 2018-03-20T10:46:52Z

src/GHC/Packages.hs

+unix                = hsLib  "unix"
+win32               = hsLib  "Win32"
+xhtml               = hsLib  "xhtml"
+-- libiserv            = hsLib  "libiserv"


is dynamic linking broken? add TODO for such case will help future investigation

@izgzhen, what makes you believe dynamic linking is broken? The libiserv comment? libiserv doesn’t exist right now as it requires a diff in ghc to land first.

Oh, I see ... thanks!

izgzhen · 2018-03-20T10:49:43Z

src/Hadrian/Haskell/Cabal/Configured.hs

+instance Hashable ConfiguredCabal where
+    hashWithSalt salt = hashWithSalt salt . show
+
+instance NFData ConfiguredCabal where


Could we derive that with Generic?

Done in latest commit.

alpmestan · 2018-03-20T14:11:46Z

Regarding the docs target:
[...]

Does the docs rule work with the tip of master? I don't think I've changed anything major for xelatex but I'll double-check.

This commit implements two significant changes (that were not easy to separate): - Don't use ghc-cabal anymore for getting information about Haskell packages. We now instead directly use Cabal-the-library. - Make the build root configurable. This effectively gets rid of the inplace logic and allows us to place _all_ build artefacts in some directory of our choice, by passing '--build-root <some path>' to hadrian. The code for this was mostly taken from snowleopard#445.

snowleopard · 2018-03-20T23:59:05Z

Does the docs rule work with the tip of master?

@alpmestan Unfortunately there is no way to check since Hadrian is currently broken. However, it used to work just fine on my machine around a month ago.

izgzhen · 2018-03-21T01:59:53Z

src/Hadrian/Haskell/Cabal/Type.hs


 instance Hashable Cabal where
    hashWithSalt salt = hashWithSalt salt . show

 instance NFData Cabal where
-    rnf (Cabal a b c d) = a `seq` b `seq` c `seq` d `seq` ()
+    rnf (Cabal a b c d e f) = a `seq` b `seq` c `seq` d `seq` e `seq` f `seq` ()


use derivation here as well?

Turns out we can derive NFData but not Hashable because GenericPackageDescription does not have a Hashable instance. Will be done in an upcoming commit.

izgzhen · 2018-03-21T02:01:07Z

src/Rules.hs

-                     , ghcPkg, hp2ps, hpc, hsc2hs, runGhc ]
+-- programsStage1Only :: [Package]
+-- programsStage1Only = [ deriveConstants, genapply, genprimopcode, ghc, ghcCabal
+--                      , ghcPkg, hp2ps, hpc, hsc2hs, runGhc ]


delete if useless?

izgzhen · 2018-03-21T02:44:08Z

@alpmestan update on docs: the previous error is probably caused by my outdated sphinx-build. It is fixed with a newer version. But I bumped into another error:

 Run Ghc CompileCWithGhc Stage0: utils/unlit/unlit.c => _build/stage0/utils/unlit/build/c/unlit.o
shakeArgsWith   0.000s    0%                           
Function shake  8.225s   47%  =======================  
Database read   0.360s    2%  =                        
With database   0.026s    0%                           
Running rules   8.790s   50%  =========================
Total          17.401s  100%                           
Error when running Shake build system:
* docs
* _build/docs/archives/libraries.html.tar.xz
* _build/docs/html/libraries/index.html
* _build/docs/html/libraries/ghc-prim/ghc-prim.haddock
* _build/docs/html/libraries/ghc-prim/haddock-prologue.txt
* OracleQ (ConfiguredCabalFile (Context {stage = Stage1, package = Package {pkgLanguage = Haskell, pkgType = Library, pkgName = "ghc-prim", pkgPath = "libraries/ghc-prim"}, way = v}))
* _build/stage1/libraries/ghc-prim/setup-config
* _build/stage1/lib/package.conf.d/rts-1.0.conf
* OracleQ (ConfiguredCabalFile (Context {stage = Stage1, package = Package {pkgLanguage = C, pkgType = Library, pkgName = "rts", pkgPath = "rts"}, way = v}))
* _build/stage1/rts/setup-config
* _build/stage0/bin/ghc
* OracleQ (ConfiguredCabalFile (Context {stage = Stage0, package = Package {pkgLanguage = Haskell, pkgType = Program, pkgName = "ghc-bin", pkgPath = "ghc"}, way = v}))
* _build/stage0/ghc/setup-config
* _build/stage0/lib/package.conf.d/ghc-8.5.conf
* OracleQ (ConfiguredCabalFile (Context {stage = Stage0, package = Package {pkgLanguage = Haskell, pkgType = Library, pkgName = "ghc", pkgPath = "compiler"}, way = v}))
* _build/stage0/compiler/setup-config
* _build/stage0/lib/package.conf.d/binary-0.8.5.1.conf
* _build/stage0/lib/package.conf.d/base-4.10.1.0.conf
* _build/stage0/lib/package.conf.d/rts.conf
user error (Development.Shake.cmd, system command failed
Command: /usr/local/bin/ghc-pkg --global-package-db _build/stage0/lib/package.conf.d register -v0 -
Exit code: 1
Stderr:
rts-1.0: package(s) with this id already exist: rts
rts-1.0: package rts-1.0 is already installed
rts-1.0: Package names may be treated case-insensitively in the future.
Package rts-1.0 overlaps with: rts-1.0 (use --force to override)
)

angerman · 2018-03-21T03:28:08Z

@izgzhen nuke _build and try again. There's probably a check missing to not re-register the rts in the bootstrap database.

izgzhen · 2018-03-21T03:28:21Z

I deleted the _build and restarted. Looks like docs builds well :)

angerman · 2018-03-21T03:30:00Z

We could also probably just --force register the packages. We'd be hosed in any case if someone switched the bootstrap compiler between runs.

Maybe not even trying to register the package in the bootstrap db, if it's present might be the cleaner choice.

angerman · 2018-03-21T05:38:39Z

Regarding stages

In general there are only two sets of file we generate: we either compile them by the bootstrap compiler, or we compile them with the stage1 compiler.

For proper stage separation, we'd want to keep the artifacts build with the same compile in the same stage.

Stage 0 (bootstrap)

This stage contains packages and utilities built by the bootstrap compiler, that are necessary to build the final compiler. We rely on packages that are shipped with the bootstrap compiler where we do not build those packages. That is: bootpackages + packages form the bootstrap compiler make up the package set that is used to build the next stage compiler.

No where do we place those? The are built artifacts of the bootstrap compiler and end up in stage0/bin and stage0/lib. For packages that are not in the bootpackages, we clone (copy & register) the packages from the bootstrap compiler.

Stage 1 (the final stage)

This stage contains packages and utilities built by the compiler from the previous stage.

Now we could just bump the stage by (+1). However, it also means that it doesn't make sense to build stage2/bin/haddock, and it should be stage1/bin/haddock.

I also trust @alpmestan to have tried to minimize the diff and extract only the necessary bits from #445, I also believe this has already taken a lot more time than initially anticipated.

alpmestan · 2018-03-29T19:10:29Z

By the way, Simon (Marlow) just released a new version of alex (see haskell/alex#123 (comment)), so this should "unlock" the CI scenarios that use 8.4 as stage0.

angerman · 2018-03-30T13:55:33Z

@snowleopard I've opened #538 for the rule replacement.

snowleopard · 2018-03-30T14:45:43Z

OK, I think we are mostly done with this PR. There are a few things that I'd like to look at after merging:

Try to break import loops by adding more .Type modules (no need to worry about this now).
Work on Make rules generic (to reenable -c flag as well). #538 to fix -c.
Figure out where we can replace buildDir with contextDir.
Finish the Settings/Packages.hs story.

@angerman There is one question that I think I haven't got an answer from you yet:

I don't understand why Hadrian is different from Make in terms of which GHC is used to build Haddock: Make uses ghc-stage2, i.e. the final GHC we build, whereas you propose for Hadrian to use ghc-stage1, i.e. the intermediate version of GHC. My understanding was that ghc-stage1 just didn't have enough features (or latest libraries) to build Haddock.

Could you clarify?

@alpmestan Do you still have any unfinished work on this PR? Anything I missed that needs further discussion?

alpmestan · 2018-03-30T15:28:05Z

I created an issue about reviving wrapper/install rules under a "modernised" form (in light of this PR, that is) at #539.

@alpmestan Do you still have any unfinished work on this PR? Anything I missed that needs further discussion?

I still need to update the README as well as the user settings document. And address a couple of small things here and there. But nothing huge. Unless I'm missing something? Github's UI has certainly not made it easy for us to track what got addressed or not as I was pushing new commits. Please do let me know if you spot anything.

…ld root

alpmestan · 2018-03-30T15:42:29Z

Updated {README, doc/user-settings}.md.

alpmestan · 2018-03-30T16:44:27Z

Look at this beautiful travis CI page: https://travis-ci.org/snowleopard/hadrian/builds/360321326 🎉

alpmestan · 2018-03-30T17:26:45Z

Alright, I pushed another patch that addresses some minor things.

I also tried to get rid of buildDir/buildPath/getBuildPath, replacing them by their context counterparts, but ran into various problems that have to do with autogen paths I think. For reference, if anyone wants to pick this up, the WIP patch is available in this gist. @snowleopard Do you mind keeping things as they are for now? If you don't, but still would like to see this resolved eventually, please do feel free to create an issue linking to my patch, where I can tell you a bit more about the problems that I see with my patch.

@snowleopard Anything else that you see that I should take care of?

snowleopard · 2018-03-30T17:31:05Z

@alpmestan Thanks! I've added the buildDir/contextDir item to the list of things I'd like to have a look: #531 (comment). After merging this PR, I'll copy this list into a separate issue.

I think I can merge this now. There are some unanswered questions, but discussing them here seems to be counterproductive. I suggest to merge and move on to discussing the leftovers in separate issues.

@alpmestan Waiting for a Good-To-Go signal from you :)

alpmestan · 2018-03-30T18:15:27Z

@alpmestan Waiting for a Good-To-Go signal from you :)

snowleopard · 2018-03-30T18:31:37Z

Done!

@alpmestan @angerman Thank you both for this amazing PR!

#531 removed install rules (`src/Rules/Install.hs`), but left a lot of supporting code that is now unused. Hopefully, this clean up completes the removal of install rules. If they are ever coming back to Hadrian in any shape, some useful code can be recovered here. See #540.

angerman mentioned this pull request Mar 19, 2018

Error: at least ftruncate or _chsize functions are required to build #523

Closed

izgzhen reviewed Mar 19, 2018

View reviewed changes

izgzhen reviewed Mar 20, 2018

View reviewed changes

alpmestan added 4 commits March 20, 2018 15:50

fix documentation rules

8298dad

remove some leftover unrelated, commented-out code

42f5b6f

more documentation fixes, address some feedback

0f85e35

izgzhen reviewed Mar 21, 2018

View reviewed changes

cleanup

e23a3d9

alpmestan force-pushed the bye-ghc-cabal branch from d4b1b1d to e23a3d9 Compare March 21, 2018 06:10

angerman mentioned this pull request Mar 30, 2018

Make rules generic (to reenable -c flag as well). #538

Closed

alpmestan mentioned this pull request Mar 30, 2018

Restore wrapper (and install?) generation rules (conditionally?) #539

Open

update README.md and doc/user-settings.md to reflect configurable bui…

770be1c

…ld root

some more feedback

c569957

snowleopard merged commit 3a68f11 into snowleopard:master Mar 30, 2018

snowleopard mentioned this pull request Mar 30, 2018

Follow-up to #531 #540

Closed

7 tasks

This was referenced Apr 2, 2018

ghc-pkg register failed with "already installed" #543

Closed

WIP: Relocation based build (inplace free) #445

Closed

This was referenced Apr 15, 2018

Fix integer-gmp build #568

Merged

Move touchy from lib/bin to bin #570

Open

This was referenced Apr 24, 2018

Get rid of ghc-cabal and package-data.mk #18

Closed

[ghc-cabal branch] Run the 'configure' command first #170

Closed

$srcdir != $builddir support? #226

Closed

izgzhen mentioned this pull request May 5, 2018

Tracking installation of built artifacts #344

Closed

chitrak7 mentioned this pull request Jun 23, 2018

Remove GhcCabal buider #635

Closed

This was referenced Aug 25, 2018

Clean up remaining bits related to ghc-cabal #658

Merged

Fix defaut top level targets #659

Merged

Clean up remaining bits of the install rule #660

Merged

snowleopard mentioned this pull request Aug 27, 2018

Should Hadrian build ghctags and haddock in Stage1? #661

Closed

Use Cabal directly in place of ghc-cabal + make build root configurable #531

Use Cabal directly in place of ghc-cabal + make build root configurable #531

Conversation

alpmestan commented Mar 19, 2018

Choose a reason for hiding this comment

izgzhen commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izgzhen commented Mar 19, 2018

alpmestan commented Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izgzhen commented Mar 19, 2018

alpmestan commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izgzhen commented Mar 20, 2018

izgzhen commented Mar 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpmestan commented Mar 20, 2018

snowleopard commented Mar 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izgzhen commented Mar 21, 2018

angerman commented Mar 21, 2018

izgzhen commented Mar 21, 2018

angerman commented Mar 21, 2018

angerman commented Mar 21, 2018

Regarding stages

Stage 0 (bootstrap)

Stage 1 (the final stage)

alpmestan commented Mar 29, 2018 • edited Loading

angerman commented Mar 30, 2018

snowleopard commented Mar 30, 2018 • edited Loading

alpmestan commented Mar 30, 2018

alpmestan commented Mar 30, 2018

alpmestan commented Mar 30, 2018

alpmestan commented Mar 30, 2018

snowleopard commented Mar 30, 2018

alpmestan commented Mar 30, 2018

snowleopard commented Mar 30, 2018

alpmestan commented Mar 19, 2018 •

edited

Loading

alpmestan commented Mar 29, 2018 •

edited

Loading

snowleopard commented Mar 30, 2018 •

edited

Loading