compareByPreference: handle the case where the two packages have a different name
ClosedPublic

Authored by alpmestan on Oct 26 2018, 5:53 AM.

Details

Summary

Currently, it assumes the package names are identical and this breaks in the
case where integer-gmp is in one package db and integer-simple in another.
This became a problem with the commit: fc2ff6dd7496a33bf68165b28f37f40b7d647418

Instead of following the precedence information, leading to the right choice,
the current code would compare the integer-gmp and integer-simple versions and
pick integer-gmp because it happened to have a greater version, despite having
a lower precedence. See https://github.com/snowleopard/hadrian/issues/702 for
a comprehensive report about the problem.

This effectively un-breaks integer-simple builds with hadrian.

Test Plan

hadrian/build.sh --integer-simple

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
alpmestan created this revision.Oct 26 2018, 5:53 AM
snowleopard added inline comments.Oct 26 2018, 6:53 AM
compiler/main/Packages.hs
925

To me this whole branch seems very strange. How can we choose between differently named packages? I am tempted to panic right here.

Is integer-wired-in the only case when we reach this point or are there any other twin packages that can replace each other? This feel a bit like Backpack actually.

alpmestan added inline comments.Oct 26 2018, 7:25 AM
compiler/main/Packages.hs
925

Yes, it's in spirit similar to filling in a backpack "interface" (mixin? I don't remember the name) with a concrete instance of the said interface.

We cannot just panic right there though, as this is the branch we end up taking now in our -gmp/-simple situation. Note that the existing comment mentions the possibility of just following the precedence. I didn't go for the whole-sale precedence-based approach to minimise the difference in behaviour with the current implementation. This would however fix our problem too.

snowleopard resigned from this revision.Oct 26 2018, 9:12 AM

@alpmestan I'm happy this fixes Hadrian, but I feel unqualified to review this patch from the GHC perspective. In my opinion, the way compareByPreference and integer-wired-in interact is questionable and error-prone (both before and after this patch), which should probaby be discussed in a separate GHC ticket.

I'll let @bgamari decide whether we can merge this as is.

I'll admit, I somewhat agree with @snowleopard; it seems very odd to have comparisons between distinct packages. Perhaps we could at least limit the special case to cases where one of the packages is integer-wired-in and throw an error otherwise?

alpmestan planned changes to this revision.Oct 28 2018, 1:30 PM

@bgamari This sounds perfectly reasonable and will address our problem. I'll tweak my patch accordingly ASAP.

I answered a bit too quickly, yesterday. integer-wired-in doesn't get passed to compareByPreference. I assumed you meant "integer-wired-in replacements", so to speak. The reason why we end up comparing two different packages is that just a little earlier in the pipeline, we open that possibility by saying that there are two packages that can match integerUnitId. So I'll proceed accordingly, by introducing a clause not for the general case of two different packages, but specifically for the case where one is integer-gmp and the other is integer-simple.

alpmestan updated this revision to Diff 18520.Oct 29 2018, 5:31 AM
  • only handle different packages when they're integer-{gmp, simple}
bgamari requested changes to this revision.Nov 5 2018, 1:34 PM

Looks good. Just one more note.

compiler/main/Packages.hs
899

One last request: Add a reference to Note [The integer library] in PrelNames.

This revision now requires changes to proceed.Nov 5 2018, 1:34 PM
alpmestan updated this revision to Diff 18596.Nov 5 2018, 1:41 PM
  • add reference to integer library note in PrelNames
alpmestan marked an inline comment as done.Nov 5 2018, 1:41 PM

Done.

bgamari accepted this revision.Nov 12 2018, 8:29 AM

Good good.

This revision is now accepted and ready to land.Nov 12 2018, 8:29 AM
This revision was automatically updated to reflect the committed changes.