snowleopard
commented
5 years ago @alpmestan Thank you for the amazing investigation!
I don't understand why simply adding -no-global-package-db to all invocations of GHC by Hadrian is not a good solution. You say:
However, we don't want to force our users to do this when using our ghc executables!
But they won't need to! We will install GHC and the package library in completely separate directory, outside the _build tree, where there will be no way confuse different stages.
I would really prefer to keep everything built in Stage0 in the _build/stage0 directory. It feels the Right Thing to do, because it's easy to explain. Introducing the complexity into the build directory structure just as a workaround for what I believe is a plain bug in compareByPreference sounds wrong.
alpmestan
bgamari
angerman
... and I figured out why while investigating the CI failures from #701. It's not something there's one natural fix for that stands out. We're at a crossroads and we need to pick a solution. Everyone's free to give some input, but it'd be great to in particular hear from @bgamari @snowleopard and @angerman here.
Here's the problem statement. When we build with
--integer-simple, stage 1 GHC gets built fine (under_build/stage0/bin/ghc, since it is built by the boot -- stage 0 -- compiler). The libraries that were needed for this got either built or copied from the boot compiler's package db and reside underbuild/stage0/lib. Then the stage 1 GHC starts building the few packages it needs for building the stage 2 compiler, and while it's doing this, we get errors about mismatched interface files that have to do withIntegerand in particular GHC seems to be reaching for integer-gmp interface files. Stage 1 GHC has built integer-simple at that point (under_build/stage1/lib) and should therefore be using it, but instead it gets confused and goes for integer-gmp. This only happens with a GHC three that has this commit.So what's going on? Everything
Integerbelong to a fictional packageinteger-wired-inwhich gets resolved to some version of integer-gmp or integer-simple depending on the circumstances. Our stage 1 GHC lives under_build/stage0/bin/ghc, and with the current heuristics, the global ("system") package db is determined to be under$topdir/package.conf.d. By default,$topdiris assumed to be at../libcompared to where the executable lives. So the global package db for our stage 1 ghc is therefore taken to be_build/stage0/lib/package.conf.d, even though those are things built by the boot compiler! You can see how this is going to be problematic...In
initPackages, we end up looking at two package databases, the global one mentionned above, because that's something we always look at by default, unless told not to (-no-global-package-db), as well as the one given on the command line (the right one,_build/stage1/lib/package.conf.d). Here's their contents:And in
findWiredInPackages, when comes the time to decide whatinteger-wired-inshould resolve to for everything that this ghc session will do, we're faced with a choice betweenstage0/lib'sinteger-gmp-1.0.2.0andstage1/lib'sinteger-simple-0.1.1.1. As you can probably guess from the error, GHC ends up pickinginteger-gmpeven though it was built with the boot compiler and not stage 1, and later down the road, whenever we touch something that refers to any entity frominteger-wired-in, boom.Now you may wonder, why on earth do we pick integer-gmp? Well, the "comparison" is done using a function named
compareByPreference, whose code is below:The precedence index thingie is just a map that gives (in our case) higher precedence to packages from
_build/stage1/lib/package.conf.d(because it's given on the command line) then to those from_build/stage0/lib/package.conf.d(which is the global one for our stage 1 GHC). So in that precedence index,integer-gmpis given a precedence of 1 andinteger-simplehas 2, which should in theory make it "win". But look at the code... It compares package versions before anything. It assumes the package names are identical, I guess. And it turns out,integer-gmpversion is strictly greater thaninteger-simple's... so it gets picked. I can get the entire build to succeed by simply changingcompareByPreferenceto the following implementation:Now, you can see this is not ideal. But it still makes more sense than the existing implementation, which completely ignores the possibility that the packages might be different. However, this situation shouldn't happen to begin with, our stage 1 ghc's global package database should be
_build/stage/lib/package.conf.d. So we've got two problems:compareByPrecedencemakes an assumption that bad situations can break, whereas my definition can be a lot more helpful in those cases, especially if the situation is bad enough to make it land in theerrorbranch. And alternative implementation would be to error out ifpackageName pkg /= packageName pkg'instead of looking at precedences. Regardless, there's an improvement to be made there I think. (1)_build/stage<N>/bin/ghc's package database lives at_buid/stage<N+1>/lib/package.conf.dbut GHC doesn't know about such layouts so_build/stage<N>/bin/ghcends up thinking its package db is_build/stage<N>/lib/package.conf.d. We override the-package-db` whenever we invoke ghc in hadrian, but that's not going to be the case of people running the ghc executables manually. (2)For (1), a GHC patch that implements either of my suggestions is enough to fix it. It is however more of a "improve behaviour/problem reporting under buggy circumstances" patch, and is not the core of our problem. The real problem is (2). Note that the build failure can also be addressed by specifying
-no-global-package-dbin every_build/stage<N>/bin/ghccommand, that'd make integer-gmp (and the whole stage0 package db) disappear from the listing above, and there would be no choice left to make, we'd just go for integer-simple. However, we don't want to force our users to do this when using our ghc executables!What are our options? A few came up in my mind or in discussions with others. But here's the one I would likely suggest we go for.
Let's move
_build/stage<N>/bin/ghcto_build/stage<N+1>/bin/ghc. That way, it would sit right next to the libraries that it builds (as opposed to next to the other things that were built by the same compiler that built_build/stage<N>/bin/ghc, like now), the$topdirwould correctly resolve to_build/stage<N+1>/lib, the default/global package would correctly resolve to_build/stage<N+1>/lib/package.conf.dand everything would go well. We would have:The alternatives all worse, from what I can tell. We could require the generation and use of wrapper scripts that point to the right topdir with
-B. We do want to support wrappers and have some code for that, but requiring them when we can avoid it feels a bit silly. We could teach GHC about thosestage<N>/stage<N+1>dirs, but that would be a gross hack. All in all, I think the aforementionned split would be best.Thoughts on both problems and the solutions that I suggest?