Distinguish between true package names, and munged package names #4382

Ericson2314 · 2017-03-08T18:39:18Z

There should be little-to-no functional changes with this commit

mention-bot · 2017-03-08T18:39:21Z

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

ezyang · 2017-03-08T19:59:42Z

Cabal/Distribution/Backpack/PreExistingComponent.hs

@@ -46,9 +48,10 @@ ipiToPreExistingComponent ipi =
    PreExistingComponent {
        pc_pkgname = case Installed.sourcePackageName ipi of
            Just n -> n
-            Nothing -> pkgName $ Installed.sourcePackageId ipi,
+            Nothing -> mkPackageName $ unMungedName
+              $ mungedName $ Installed.sourceMungedId ipi,


This deserves a comment, since it is doing something dodgy.

Well hmm, I moved the functionality to Distribution.InstalledPackageInfo, and give a it a quick comment, but perhaps the fact that ghc-pkg can strip this metadata so care must be taken should also be mentioned.

ezyang · 2017-03-08T20:02:09Z

cabal-install/Distribution/Client/Types.hs

@@ -151,6 +162,10 @@ instance Package ConfiguredId where
 instance Package (ConfiguredPackage loc) where
  packageId cpkg = packageId (confPkgSource cpkg)

+instance HasMungedId (ConfiguredPackage loc) where


Has munged id... of what?

Should I everywhere rename MungedId to MungedPackageId? I don't think this is any more or less clear than the type itself.

At your discretion!

ezyang · 2017-03-08T20:04:22Z

Cabal/Distribution/InstalledPackageInfo.hs

-        -- 'sourcePackageId' actually stores a *munged* version of the
-        -- package identifier that also incorporates the component name.
-        -- The /real/ package name is stored in 'sourcePackageName'.
-        sourcePackageId   :: PackageId,


At the very least, we need a compatibility definition of sourcePackageId from this module. You can DEPRECATE it if you like.

Ah, that would solve most problems. Should it recover original name or just be straight synonym?

Well, it depends on whether or not sourcePackageId returns a PackageId or a MungedId ;)

ezyang · 2017-03-08T20:08:34Z

Cabal/Distribution/Simple/Build/Macros.hs

@@ -73,7 +76,8 @@ generate :: PackageDescription -> LocalBuildInfo -> ComponentLocalBuildInfo -> S
 generate pkg_descr lbi clbi =
  "/* DO NOT EDIT: This file is automatically generated by Cabal */\n\n" ++
  generatePackageVersionMacros
-    (package pkg_descr : map snd (componentPackageDeps clbi)) ++
+    (computeCompatPackageId (package pkg_descr) CLibName


This is a no-op right, because CLibName always is the real package name.

Yeah, it's a no-op. I like to convert in this way this as a warning sign that the code might not be very component-aware.

ezyang · 2017-03-08T20:12:36Z

Cabal/Distribution/Simple/GHC/IPIConvert.hs

-data PackageIdentifier = PackageIdentifier {
-    pkgName    :: String,
-    pkgVersion :: Version
+data MungedId = MungedId {


This isn't right. The reason why PackageIdentifier is redefined here is because the conversion function needs the Read instance (see below) to be compatible with the output of a Show instance from an old version of Cabal. If you change the data structure as you've done here, you break the Read instance!

Can we change the type name as long as the constructor stays the same? I'm trying that.

Not only does the constructor have to be, but each record selector has to be the same.

ezyang@sabre:~$ runghc T.hs MkT {f = 2} ezyang@sabre:~$ cat T.hs data T = MkT {f :: Int } deriving (Show) main = print (MkT 2)

ezyang · 2017-03-08T20:12:56Z

Cabal/Distribution/Types/MungedId.hs

+{-# LANGUAGE DeriveDataTypeable #-}
+{-# LANGUAGE DeriveGeneric #-}
+module Distribution.Types.MungedId
+  ( MungedId(..)


Let's make this abstract please.

~~Oh oops that was never intended.~~

ezyang · 2017-03-08T20:13:50Z

There are also general BC considerations which I've mentioned to @Ericson2314 on IRC.

Ericson2314 · 2017-03-09T02:31:19Z

Ok fixed everything but the opacity. Whatever is done there I think is best to do in a second commit (though in this PR is fine.)

To put some things into context, I just thought of an example why we hypothetically might want to change PackageId and MungedId in practice: to disambiguate between packages' origin in the context of new-build projects using local and git overrides. It's unclear how these other package sources interact with qualified goals---perhaps packages with the same name and version from different origins can exist in the same plan.

I guess this would be an argument for opacity if we can default new fields in a back-compat way (for construction). Perhaps they both should be opaque? The consistency would make me feel better about it, I think.

There should be little-to-no functional changes with this commit

ezyang · 2017-03-09T05:58:52Z

I guess this would be an argument for opacity if we can default new fields in a back-compat way (for construction).

It depends on how you setup the smart constructor. One reasonably forwards compatible scheme is to have the constructor take package name, library name, and version number. Then you can switch representation between (MungedName, Version) versus (PackageName, LibraryName, Version).

Perhaps they both should be opaque? The consistency would make me feel better about it, I think.

The difference is making PackageId opaque is a (gasp) BC-breaking change ;)

new patchset

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

This reverts commit 4774fb6.

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

This reverts commit 4774fb6.

Distinguish between true package names, and munged package names

Ericson2314 force-pushed the munge branch from e497bbf to 3b155e7 Compare March 8, 2017 19:57

ezyang previously requested changes Mar 8, 2017

View reviewed changes

Ericson2314 force-pushed the munge branch from 3b155e7 to 2862313 Compare March 9, 2017 02:19

Distinguish between true package names, and munged package names

1512cf3

There should be little-to-no functional changes with this commit

Ericson2314 force-pushed the munge branch from 2862313 to 1512cf3 Compare March 9, 2017 02:42

Ericson2314 mentioned this pull request Mar 9, 2017

Fix #2066: buildDepends is just an accessor function, not a field #4383

Merged

ezyang merged commit 332d809 into haskell:master Mar 10, 2017

Ericson2314 deleted the munge branch March 10, 2017 16:51

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

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

4774fb6

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

ezyang mentioned this pull request Mar 14, 2017

Fix Backpack tests #4396

Closed

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

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

4dc0f30

This reverts commit 4774fb6.

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

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

13c1437

This reverts commit 4774fb6.

ezyang mentioned this pull request Mar 14, 2017

Resurrect MungedId patch, and a pile of bugfixes #4397

Merged

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

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

b1b9d3b

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

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

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

b0e6c31

This reverts commit 4774fb6.

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

Merge pull request haskell#4382 from Ericson2314/munge

2c7abac

Distinguish between true package names, and munged package names

Distinguish between true package names, and munged package names #4382

Distinguish between true package names, and munged package names #4382

Uh oh!

Conversation

Ericson2314 commented Mar 8, 2017

Uh oh!

mention-bot commented Mar 8, 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!

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezyang commented Mar 8, 2017

Uh oh!

Ericson2314 commented Mar 9, 2017

Uh oh!

ezyang commented Mar 9, 2017

Uh oh!

Uh oh!

Ericson2314 Mar 8, 2017 •

edited

Loading