Resurrect MungedId patch, and a pile of bugfixes #4397

ezyang · 2017-03-14T10:44:06Z

I recently was running Backpack tests manually, and noticed that they didn't work! As it turned out, our CI setup for testing with ghc-head had been broken, and so the tests weren't actually being run at all. Ooops.

When I turned the tests back on, I noticed a lot of failures. These were due to #4382, which did not correctly handle picking up internal libraries when --dependency is not explicitly specified (the proximate problem was that PackageIndex was indexed on un-munged PackageName, but we really do need an index from PackageName + LibName when we're testing if package names are defined or not.) The most expedient thing was to undo the change for now, hoping to merge it back once this problem is fixed.

There were some miscellaneous small test drift, but the rest of it didn't have any major problems.

That was the first five patches. THEN, I resurrected @Ericson2314's patch, with a huge pile of refactoring to make everything work.

The big ideas:

There's a new AnnotatedId type, which is any identifier (like
ComponentId), but also with a PackageId and ComponentName.
It's a bit like ConfiguredId, but ConfiguredId is specialized
for ComponentId. There's a conversion function
annotatedIdToConfiguredId.
We adopt a totally new strategy for handling MungedPackageId in
InstalledPackageInfo. Rather than store the munged package
identifier in IPI, we NEVER store it, instead computing it
from the package name and library name (which are stored.)
There is now code to PARSE a munged package name into these
two components, so that when we read 'name' we get the
correct info. The resulting code is splendidly clear.
Some places where we took a ComponentName (notably
computeCompatPackageName) we now take 'Maybe UnqualComponentName'.
This is more accurate, because compatibility package names are
only computed for libraries, not other components. Some code
in Distribution.Backpack.ReadyComponent is partial now,
but the invariants should hold up.
A number of places where MungedId was applied, I undid them.
Most notable are macro generation (that should be PACKAGES,
not components) and mkLegacyUnitId (since REALLY old style
unit identifiers, we won't support internal libraries anyway.)
Many functions in PackageIndex are monomorphized to
InstalledPackageInfo. Fortunately cabal-install's usage
still works.
Distribution/Client/SolverPlanIndex.hs, not used by anyone,
got the axe.
Dependency solving has a hack to solve the problem of how to
interpret installed internal libraries in the package database.
The basic idea is that, although in most cases where we used
a MungedId, we say it explicitly, in the SOLVER, we munge
installed package names only, and keep the type PackageName.
It's a hack, but the alternative is to write a lot more code.
Note that is MORALLY PN is indeed a PackageName, since we really
are solving for honest to goodness packages, not components!
See Note [Index conversion with internal libraries] for more
details.
ConfiguredId now records the ComponentName. This is quite important,
because we need to use it to figure out how we are going to phrase
--dependency. In fact, it is a miracle that this worked at all
(and it only worked because of a very skeevy update to the PackageId
in the creation of ConfiguredComponent). Irritatingly, this must
be a Maybe ComponentName, because a ConfiguredId MIGHT refer to
a setup component. Very distressing.

There's some final extra bugfixes at the end as I was bludgeoning the test suite into passing.

mention-bot · 2017-03-14T10:44:09Z

@ezyang, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dcoutts, @Ericson2314 and @23Skidoo to be potential reviewers.

Ericson2314 · 2017-03-14T19:05:31Z

Cabal/Distribution/Backpack/MixLink.hs

+  pp_pn =
+    -- NB: This syntax isn't quite the source syntax, but it
+    -- should be clear enough.  To do source syntax, we'd
+    -- need to know what the package we're linking is.


This will become the source syntax with my changes, right? :)

Ericson2314 · 2017-03-14T19:08:35Z

Cabal/Distribution/Backpack/ReadyComponent.hs


 instance Package ReadyComponent where
    packageId = rc_pkgid

-instance HasMungedPackageId ReadyComponent where


Ah good to avoid non-totality in instances 👍

Ericson2314 · 2017-03-14T19:55:50Z

Cabal/Distribution/Simple/PackageIndex.hs

@@ -138,7 +143,7 @@ data PackageIndex a = PackageIndex {
  --
  -- FIXME: Clarify what "preference order" means. Check that this invariant is
  -- preserved. See #1463 for discussion.
-  packageIdIndex :: !(Map PackageName (Map Version [a]))
+  packageIdIndex :: !(Map (PackageName, Maybe UnqualComponentName) (Map Version [a]))


What about Map PackageName (Map (Maybe UnqualComponentName) (Map Version [a]))? This matches what you ended up on for Backpack.ConfiguredComponent (and what I did in my branch :D).

I can push that (if I haven't already) if that would help.

I think that variant is OK too, but it turned out that there weren't really any cases where you wanted to find all components of a PackageName; and this version required less code changes. Notice that in all the functions below, we generally default to looking for the public library.

Actually, in your other PR (still open, but probably a little obsolete now), you did the Map as Map PackageName (Map Version (Map (Maybe UnqualComponentName) [a]))). I tried using this version first, but it's actually not the right way to go about doing things, because in a lot of ops you just want the public library...

Maybe a three level map makes sense if we decide to handle the solver more correctly than the hack I have right now, but it still doesn't make too much sense, because you still need to work out the relationships between components with the same package identifier.

Oh wait that's what I meant write. My goal was eventually that prefiltering of the index that I mentioned on IRC, but that can be done with this too.

Ericson2314 · 2017-03-14T19:58:53Z

Cabal/Distribution/Simple/PackageIndex.hs

@@ -435,12 +464,42 @@ lookupPackageName index name =
 -- We get back any number of versions of the specified package name, all
 -- satisfying the version range constraint.
 --
+-- This does NOT work for internal dependencies, DO NOT use this
+-- function on those; use 'lookupInternalDependency' instead.
+--
 -- INVARIANT: List of eligible 'IPI.InstalledPackageInfo' is non-empty.
 --
 lookupDependency :: InstalledPackageIndex -> Dependency
                 -> [(Version, [IPI.InstalledPackageInfo])]
 lookupDependency index (Dependency name versionRange) =


Can't this be a tiny shim around lookupInternalDependency with Nothing?

Yes you're right.

Also perhaps that can mean using only the internal on in configure to save a little code as I did before. I bring it up because with LibDependency it will work that way anyways.

Well, this will all get rewritten when LibDependency lands.

ezyang · 2017-03-15T01:48:36Z

CI failures are legit but I probably won't have time to debug until after my defense.

Ericson2314 · 2017-03-15T02:34:49Z

Oh! When is that?

Ericson2314 · 2017-03-15T06:15:35Z

Oh, I guess as a maintainer I can push to this?

ezyang · 2017-03-15T06:18:34Z

Thursday! And yes you can!

Ericson2314 · 2017-03-15T06:19:06Z

Good luck!

23Skidoo · 2017-03-15T08:17:48Z

@ezyang Good luck with your defense!

grayjay · 2017-03-16T05:21:36Z

cabal-install/Distribution/Solver/Modular/IndexConversion.hs

+-- with anyone else (except other instances of itself).  But
+-- yet, we ought NOT to say that PNs in the solver are munged
+-- package names, because they're not; for source packages,
+-- we really will never see source package names.


I don't really understand this sentence. Should it say "never see munged source package names"?

Yes, it should say munged.

Signed-off-by: Edward Z. Yang <[email protected]>

This reverts commit 332d809, reversing changes made to 0c72bc8.

Signed-off-by: Edward Z. Yang <[email protected]>

This reverts commit 4774fb6.

This makes the necessary changes to 4dc0f30 to handle component configuring correctly. It probably is a good step towards pkg:lib style dependencies. The big ideas: * There's a new AnnotatedId type, which is any identifier (like ComponentId), but also with a PackageId and ComponentName. It's a bit like ConfiguredId, but ConfiguredId is specialized for ComponentId. There's a conversion function annotatedIdToConfiguredId. * We adopt a totally new strategy for handling MungedPackageId in InstalledPackageInfo. Rather than store the munged package identifier in IPI, we NEVER store it, instead computing it from the package name and library name (which are stored.) There is now code to PARSE a munged package name into these two components, so that when we read 'name' we get the correct info. The resulting code is splendidly clear. * Some places where we took a ComponentName (notably computeCompatPackageName) we now take 'Maybe UnqualComponentName'. This is more accurate, because compatibility package names are only computed for libraries, not other components. Some code in Distribution.Backpack.ReadyComponent is partial now, but the invariants should hold up. * A number of places where MungedId was applied, I undid them. Most notable are macro generation (that should be PACKAGES, not components) and mkLegacyUnitId (since REALLY old style unit identifiers, we won't support internal libraries anyway.) * Many functions in PackageIndex are monomorphized to InstalledPackageInfo. Fortunately cabal-install's usage still works. * Distribution/Client/SolverPlanIndex.hs, not used by anyone, got the axe. * Dependency solving has a hack to solve the problem of how to interpret installed internal libraries in the package database. The basic idea is that, although in most cases where we used a MungedId, we say it explicitly, in the SOLVER, we munge *installed package names* only, and keep the type PackageName. It's a hack, but the alternative is to write a lot more code. Note that is MORALLY PN is indeed a PackageName, since we really are solving for honest to goodness packages, not components! See Note [Index conversion with internal libraries] for more details. * ConfiguredId now records the ComponentName. This is quite important, because we need to use it to figure out how we are going to phrase --dependency. In fact, it is a miracle that this worked at all (and it only worked because of a very skeevy update to the PackageId in the creation of ConfiguredComponent). Irritatingly, this must be a Maybe ComponentName, because a ConfiguredId MIGHT refer to a setup component. Very distressing. Signed-off-by: Edward Z. Yang <[email protected]>

Previously, configCompilerEx was setting Haddock as known, but not actually configuring it; thus, later when we required haddockProgram, we configured the WRONG program. By configuring known programs, we can fix this problem. Signed-off-by: Edward Z. Yang <[email protected]>

* Add missing record selector. * Correctly mkPackageIndex * Skip T4375 on GHC 8.2, it doesn't have a Cabal to actually use! Signed-off-by: Edward Z. Yang <[email protected]>

* Propagate ComponentName to ModuleSource, allowing us to accurately specify both package and library name of references. (Arguably, should just stuff a ComponentInclude in there.) Signed-off-by: Edward Z. Yang <[email protected]>

Signed-off-by: Edward Z. Yang <[email protected]>

Resurrect MungedId patch, and a pile of bugfixes

ezyang changed the title ~~Pr/omnibus refactor~~ Resurrect MungedId patch, and a pile of bugfixes Mar 14, 2017

ezyang mentioned this pull request Mar 14, 2017

Fix Backpack tests #4396

Closed

Ericson2314 reviewed Mar 14, 2017

View reviewed changes

ezyang force-pushed the pr/omnibus-refactor branch from 53862ec to 853750d Compare March 14, 2017 20:44

grayjay reviewed Mar 16, 2017

View reviewed changes

ezyang and others added 15 commits March 16, 2017 23:55

Support --with-haddock.

8b1ff6d

Signed-off-by: Edward Z. Yang <[email protected]>

Revert "Merge pull request haskell#4382 from Ericson2314/munge"

b1b9d3b

This reverts commit 332d809, reversing changes made to 0c72bc8.

Fix Backpack test drift (since it's not CI'd)

e663b52

Signed-off-by: Edward Z. Yang <[email protected]>

Fix CI testing of old versions of GHC.

c6e3c23

Signed-off-by: Edward Z. Yang <[email protected]>

Add non-Backpack per-component configure test.

300c4b5

Signed-off-by: Edward Z. Yang <[email protected]>

Revert "Revert "Merge pull request haskell#4382 from Ericson2314/munge""

b0e6c31

This reverts commit 4774fb6.

Bugfixes.

6326e53

* Add missing record selector. * Correctly mkPackageIndex * Skip T4375 on GHC 8.2, it doesn't have a Cabal to actually use! Signed-off-by: Edward Z. Yang <[email protected]>

Bugfixes for Backpack error reporting.

8c5511e

* Propagate ComponentName to ModuleSource, allowing us to accurately specify both package and library name of references. (Arguably, should just stuff a ComponentInclude in there.) Signed-off-by: Edward Z. Yang <[email protected]>

Fix a warning.

2d0223d

Signed-off-by: Edward Z. Yang <[email protected]>

Reuse lookupInternalDependency to implement lookupDependency.

30ed755

Signed-off-by: Edward Z. Yang <[email protected]>

Fix comment typo.

3ef451c

Signed-off-by: Edward Z. Yang <[email protected]>

Fix setup-no-tix to be hermetic, solves test failures.

94a9c33

Signed-off-by: Edward Z. Yang <[email protected]>

Fix warning on 7.10

c7cd985

ezyang force-pushed the pr/omnibus-refactor branch from 5dd8617 to c7cd985 Compare March 17, 2017 06:56

ezyang merged commit 20de0bf into haskell:master Mar 17, 2017

ezyang added a commit to ezyang/cabal that referenced this pull request Mar 21, 2017

Merge pull request haskell#4397 from ezyang/pr/omnibus-refactor

6dfa2f0

Resurrect MungedId patch, and a pile of bugfixes

Resurrect MungedId patch, and a pile of bugfixes #4397

Resurrect MungedId patch, and a pile of bugfixes #4397

Uh oh!

Conversation

ezyang commented Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Mar 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezyang commented Mar 15, 2017

Uh oh!

Ericson2314 commented Mar 15, 2017

Uh oh!

Ericson2314 commented Mar 15, 2017

Uh oh!

ezyang commented Mar 15, 2017

Uh oh!

Ericson2314 commented Mar 15, 2017

Uh oh!

23Skidoo commented Mar 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ezyang commented Mar 14, 2017 •

edited

Loading