Remove Cabal dependency from GHC
AbandonedPublic

Press ? to show keyboard shortcuts.
Author
hvr
Reviewers
ezyang
austin
duncan
Repository
rGHC Glasgow Haskell Compiler
Lint
Lint Warnings
Excusedon't trust auto-fixes
compiler/main/PackageConfig.hs
Autofix(TXT6) Trailing Whitespace at line 110
compiler/main/Packages.lhs
Warning(TXT3) Line Too Long at line 1366
Show Full Lint Results (2 Details)
Unit
No Unit Test Coverage
Branch
wip/remove-cabal-dep
Apply Patch
arc patch D172
Arcanist Project
Restricted Arcanist Project
Trac Issues
#8244
Subscribers
simonpj, simonmar, ezyang, carter
Projects
None
Build Status
Build 960: GHC Patch Validation (amd64/Linux)
Build 628: GHC Patch Validation (amd64/Linux)
Summary

(Uploaded on behalf of Duncan)

We can serialise directly, without having to convert some fields to
string first.

(Part of preparitory work for removing the compiler's dep on Cabal)

Drop support for single-file style package databases

Historically the package db format was a single text file in Read/Show
format containing [InstalledPackageInfo]. For several years now the
default format has been a directory with one file per package, plus a
binary cache.

The old format cannot be supported under the new scheme where the
compiler will not depend on the Cabal library (because it will not
have access to the InstalledPackageInfo type) so we must drop support.
It would still technically be possible to support a single text file
style db (but containing a different type), but there does not seem to
be any compelling reason to do so.

(Part of preparitory work for removing the compiler's dep on Cabal)

Improve the ghc-pkg warnings for missing and out of date package cache files

In particular, report when it's missing, and also report it for ghc-pkg check.
Also make the warning message more explicit, that ghc will not be able to
read these dbs, even though ghc-pkg may be able to.

Introduce new file format for the package database binary cache

The purpose of the new format is to make it possible for the compiler
to not depend on the Cabal library. The new cache file format contains
more or less the same information duplicated in two different sections
using different representations.

One section is basically the same as what the package db contains now,
a list of packages using the types defined in the Cabal library. This
section is read back by ghc-pkg, and used for things like ghc-pkg dump
which have to produce output using the Cabal InstalledPackageInfo text
representation.

The other section is a ghc-local type which contains a subset of the
information from the Cabal InstalledPackageInfo -- just the bits that
the compiler cares about.

The trick is that the compiler can read this second section without
needing to know the representation (or types) of the first part. The
ghc-pkg tool knows about both representations and writes both.

This patch introduces the new cache file format but does not yet use it
properly. More patches to follow. (As of this patch, the compiler reads
the part intended for ghc-pkg so it still depends on Cabal and the
ghc-local package type is not yet fully defined.)

Use ghc-local types for packages, rather than Cabal types

Also start using the new package db file format properly, by using the
ghc-specific section.

This is the main patch in the series for removing the compiler's dep
on the Cabal lib.

Move Cabal Binary instances from bin-package-db to ghc-pkg itself

The ghc-pkg program of course still depends on Cabal, it's just the
bin-package-db library (shared between ghc and ghc-pkg) that does not.

Drop ghc library dep on Cabal

Make binary a boot package

Since ghc-pkg needs a relatively recent version.

Fix warnings arising from the package db refactoring

Fix more warnings, remove now-dead code

Also remove more of the old file style ghc-pkg dbs that I missed previously.

Test Plan

validate

ezyang created this revision.Via ConduitAug 22 2014, 10:44 AM
ezyang added reviewers: austin, hvr.
ezyang updated the Trac tickets for this revision.
ezyang updated the summary for this revision. (Show Details)Via WebAug 22 2014, 10:46 AM
ezyang added a comment.Via WebAug 22 2014, 11:11 AM

Some minor comments, but looks good! Ship it!

compiler/main/Finder.lhs
618 ↗(On Diff #424)

Hm, a type signature on this function would be handy.

compiler/main/PackageConfig.hs
50 ↗(On Diff #424)

Maybe a remark about why it's OK to represent these data types as strings, as opposed to ModuleName and PackageKey (namely, because the equality comparisons we do for them are not on the hotpath)

67 ↗(On Diff #424)

Do we seriously not have a bytestring to faststring conversion function?

compiler/main/Packages.lhs
386 ↗(On Diff #424)

Isn't this TODO done?

565 ↗(On Diff #424)

I think it might be better to convert the string to a FastString than vice versa.

ghc.mk
386 ↗(On Diff #424)

Oh, do we really assume binary isn't recent enough?

libraries/bin-package-db/GHC/PackageDb.hs
154 ↗(On Diff #424)

It's worth remarking that the instance here is not defined in this package!

228 ↗(On Diff #424)

I added a PR to put this in binary proper, maybe mention that.

utils/ghc-pkg/Main.hs
596 ↗(On Diff #424)

Wait, but the conditional doesn't look and see if the command is check/modify?

696 ↗(On Diff #424)

Can we refactor out this condition?

Harbormaster failed to build B589: Diff 424!Via DaemonsAug 22 2014, 11:15 AM
ezyang retitled this revision from "Simplify conversion in binary serialisation of ghc-pkg db" to "Remove Cabal dependency from GHC".Via WebAug 24 2014, 5:51 PM
ezyang updated this revision to Diff 438.Via ConduitAug 24 2014, 5:52 PM
  • Drop support for single-file style package databases
  • Improve the ghc-pkg warnings for missing and out of date package cache files
  • Introduce new file format for the package database binary cache
  • Use ghc-local types for packages, rather than Cabal types
  • Move Cabal Binary instances from bin-package-db to ghc-pkg itself
  • Drop ghc library dep on Cabal
  • Make binary a boot package
  • Fix warnings arising from the package db refactoring
  • Fix long lines and trailing whitespace
  • Remove a TODO that is now done
  • Add a ghc -show-packages mode to display ghc's view of the package env
  • Make mkFastStringByteString pure and fix up uses
  • Switch the package id types to use FastString (rather than String)
  • Fix string conversions in ghc-pkg to be correct w.r.t. Unicode
  • Address a number of Edward's code review comments
ezyang added a comment.Via WebAug 24 2014, 5:59 PM

LGTM.

compiler/utils/FastString.lhs
383 ↗(On Diff #438)

Why did this get inlinePerformIO'ified?

carter added a comment.Via WebAug 24 2014, 6:06 PM

Question thats related: is Duncan's WIP new binary major version out yet? (or is that unrelated to the use of Binary in ghc?)

Harbormaster failed to build B605: Diff 438!Via DaemonsAug 24 2014, 6:09 PM
duncan added a reviewer: duncan.Via WebAug 27 2014, 8:30 AM
duncan removed a subscriber: duncan.
duncan added a subscriber: duncan.
duncan added a comment.Via WebAug 27 2014, 8:33 AM
In D172#12, @carter wrote:

Question thats related: is Duncan's WIP new binary major version out yet? (or is that unrelated to the use of Binary in ghc?)

Unrelated.

compiler/utils/FastString.lhs
383 ↗(On Diff #438)

as opposed to unsafePerformIO? Because that's what FastString uses for all of these (though I suspect it's unwise). For example see the appendFS which is using inlinePerformIO.

If you mean, why do we need it to be pure? Because we need to use it in a pure context elsewhere. It's being used in a pure context already (appendFS), and it is morally pure so it's the right way round.

As an independent thing, I think all the uses of inlinePerformIO throughout the FastString module should be audited. I would avoid allocating inside an inlinePerformIO.

ezyang added inline comments.Via WebAug 27 2014, 8:38 AM
compiler/utils/FastString.lhs
383 ↗(On Diff #438)

OK.

duncan commandeered this revision.Via WebAug 27 2014, 10:44 AM
duncan edited reviewers, added: ezyang; removed: duncan.

Now regained my logon credentials :-)

duncan updated this revision to Diff 448.Via ConduitAug 27 2014, 10:51 AM

Hopefully add a few extra patches...

No idea if I'm using arc correctly. We'll see...
Add patches for the testsuite, haddock and Cabal updates
The ./validate now passes, except for a couple unrelated test failures.

duncan updated this revision to Diff 449.Via ConduitAug 27 2014, 10:56 AM

Ok, that didn't work, lets try again...

ezyang accepted this revision.Via WebAug 27 2014, 11:07 AM

LGTM.

Harbormaster failed to build B626: Diff 449!Via DaemonsAug 27 2014, 11:40 AM
Harbormaster failed to build B625: Diff 448!
austin requested changes to this revision.Via WebAug 27 2014, 12:31 PM

Generally LGTM, although please kill the dead code/trailing comments.

Also, should the behavioral change of ghc-pkg now requiring ghc-pkg init (e.g. in the testsuite) be documented? I am not sure if anyone other than us relied on it, but perhaps noting it if worthy (in the release notes).

I think the change should at least also include a release note modification saying the compiler no longer depends on the Cabal library.

compiler/main/PackageConfig.hs
110

Kill this trailing whitespace.

(NB, regarding the linting excuse: 'auto fixes' are only suggested in the case the lint rule actively suggests a replacement for the given line. Not every linting rule does this: for example, the 'trailing whitespace' suggestion can autofix - the suggested line is trivial - but 80-column violations can not be reliably autofixed by the generic linter. In general it should be quite safe to let Arcanist fix things like this if you miss them.)

compiler/main/Packages.lhs
1366

Fixing this would be nice, JFYI. :)

compiler/utils/Binary.hs
684

Is there any reason for this change? If it improves things or fixes a leak (somehow) it should be noted, at least (aside: unfortunately we can't use <$!> or anything here yet as an alternative.)

utils/ghc-pkg/Main.hs
60

Kill this line.

This revision now requires changes to proceed.Via WebAug 27 2014, 12:31 PM
simonpj added a subscriber: simonpj.Via WebAug 27 2014, 3:29 PM

I can't comment on the details, but I'm very supportive of the direction of travel.

Would it be possible to describe the new architecture -- the pieces and how they fit together -- on a GHC Wiki page? That would be super-helpful.

Simon

duncan added a comment.Via WebAug 27 2014, 11:19 PM
In D172#33, @simonpj wrote:

I can't comment on the details, but I'm very supportive of the direction of travel.

Cheers.

Would it be possible to describe the new architecture -- the pieces and how they fit together -- on a GHC Wiki page? That would be super-helpful.

https://ghc.haskell.org/trac/ghc/wiki/CabalDependency

compiler/utils/Binary.hs
684

It's really not a change. Previously it was in IO and so ran strictly. Now that it's pure, the equivalent is to evaluate it strictly here.

I didn't know if it was important or not that it be evaluated strictly here, so I kept it strict rather than change it to be lazy.

duncan added a comment.EditedVia WebAug 27 2014, 11:58 PM
In D172#30, @austin wrote:

Generally LGTM,

Thanks for the review.

although please kill the dead code/trailing comments.

Done.

Also, should the behavioral change of ghc-pkg now requiring ghc-pkg init (e.g. in the testsuite) be documented? I am not sure if anyone other than us relied on it, but perhaps noting it if worthy (in the release notes).

Documented in the release notes.

I think the change should at least also include a release note modification saying the compiler no longer depends on the Cabal library.

Done.

All the changes are in the wip/remove-cabal-dep branch.

So as far as I know, we're good to go. I did have a couple test failures, but I'm fairly sure they're nothing to do with these changes and were pre-existing.

hvr commandeered this revision.Via WebSep 23 2014, 12:02 PM
hvr edited reviewers, added: duncan; removed: hvr.
duncan added a comment.Via WebSep 23 2014, 12:03 PM

This has been committed. We used the wip/ branch rather than this diff since it was a whole series of patches that we didn't want to squash into one.

I can't abandon the revision however apparently as I'm not the owner (though I am the author -- clearly something I don't understand)

hvr abandoned this revision.Via WebSep 23 2014, 12:03 PM

This was landed manually, see commits 6d8c70c1262a0f8b02ee685905f469f94d742af2 and earlier

Revision Update History

DiffIDBaseDescriptionCreatedLintUnit
BaseBase
Diff 14242719526Aug 22 2014, 10:44 AM
Diff 24382719526 - Drop support for single-file style package databasesAug 24 2014, 5:52 PM
Diff 344897d95b1Hopefully add a few extra patches...Aug 27 2014, 10:51 AM
Diff 44492719526Ok, that didn't work, lets try again...Aug 27 2014, 10:56 AM

Local Commits

CommitTreeParentsAuthorSummaryDate
7273bf071d1bc2579d66d4d997d95b14acadDuncan Coutts
Change testsuite to not use old-style file package databases (Show More…)
Aug 27 2014, 10:33 AM
97d95b14acadd21898aea2e6ecdf5363d7e8Duncan Coutts
Update Cabal and haddock to follow the Canal-dep removal changes (Show More…)
Aug 27 2014, 7:57 AM
ecdf5363d7e8276323fb4acfe1d9fcdef97dDuncan Coutts
Fix validation error in Linker arising from package rep changes
Aug 27 2014, 7:26 AM
e1d9fcdef97d5431dbcd962585f8f20ea7e1Duncan Coutts
Address a number of Edward's code review comments (Show More…)
Aug 24 2014, 5:43 PM
85f8f20ea7e13efd6b79998c575a9e81c8e3Duncan Coutts
Fix string conversions in ghc-pkg to be correct w.r.t. Unicode (Show More…)
Aug 24 2014, 4:11 PM
575a9e81c8e3278e50f6900d025f626309d9Duncan Coutts
Switch the package id types to use FastString (rather than String) (Show More…)
Aug 24 2014, 3:59 PM
025f626309d9a73d058cdce122b2cf5b12fcDuncan Coutts
Make mkFastStringByteString pure and fix up uses (Show More…)
Aug 24 2014, 3:46 PM
22b2cf5b12fc3ec2035ca090cd4edf2b20d6Duncan Coutts
Add a ghc -show-packages mode to display ghc's view of the package env (Show More…)
Aug 23 2014, 9:38 PM
cd4edf2b20d667924042ec9f4f4dd1c312c1Duncan Coutts
Remove a TODO that is now done
Aug 23 2014, 9:41 PM
4f4dd1c312c10fa8dfd556b87925a43240e7Duncan Coutts
Fix long lines and trailing whitespace (Show More…)
Aug 23 2014, 7:12 AM
7925a43240e7a608c10360f2692116f32408Duncan Coutts
Fix warnings arising from the package db refactoring
Aug 22 2014, 9:57 AM
692116f32408c2e7b8b12da8b87a05e3130fDuncan Coutts
Make binary a boot package (Show More…)
Aug 22 2014, 9:10 AM
b87a05e3130f614317186a9900816eef3ac6Duncan Coutts
Drop ghc library dep on Cabal
Aug 22 2014, 9:09 AM
00816eef3ac6a5273c9a9a518e3fdcb6e406Duncan Coutts
Move Cabal Binary instances from bin-package-db to ghc-pkg itself (Show More…)
Aug 22 2014, 9:08 AM
8e3fdcb6e40698d6aab9cc8ca54d9a90c572Duncan Coutts
Use ghc-local types for packages, rather than Cabal types (Show More…)
Aug 22 2014, 8:38 AM
a54d9a90c57204a44d96cdf1819e28fad20cDuncan Coutts
Introduce new file format for the package database binary cache (Show More…)
Aug 19 2014, 2:33 PM
819e28fad20c89987f08ce6b1b7645564c04Duncan Coutts
Improve the ghc-pkg warnings for missing and out of date package cache files (Show More…)
Aug 19 2014, 10:10 AM
1b7645564c045e3ee98d6ed04feb9902dbc7Duncan Coutts
Drop support for single-file style package databases (Show More…)
Aug 19 2014, 7:23 AM
4feb9902dbc78bd355fdd29927195265e2a9Duncan Coutts
Simplify conversion in binary serialisation of ghc-pkg db (Show More…)
Aug 18 2014, 7:00 PM

Similar Open Revisions
Recently updated open revisions affecting the same files.

Diff 449

compiler/basicTypes/Module.lhs

Loading...

compiler/deSugar/MatchLit.lhs

Loading...

compiler/ghc.cabal.in

Loading...

compiler/ghci/Linker.lhs

Loading...

compiler/main/Finder.lhs

Loading...

compiler/main/PackageConfig.hs

Loading...

compiler/main/Packages.lhs

Loading...

compiler/utils/Binary.hs

Loading...

compiler/utils/FastString.lhs

Loading...

ghc.mk

Loading...

ghc/Main.hs

Loading...

libraries/Cabal

Loading...

libraries/bin-package-db/Distribution/InstalledPackageInfo/Binary.hs

Loading...

libraries/bin-package-db/GHC/PackageDb.hs

Loading...

libraries/bin-package-db/bin-package-db.cabal

Loading...

testsuite/tests/driver/T1372/Makefile

Loading...

testsuite/tests/driver/T3007/Makefile

Loading...

testsuite/tests/ghci/linking/Makefile

Loading...

testsuite/tests/plugins/simple-plugin/Makefile

Loading...

testsuite/tests/rename/prog006/Makefile

Loading...

testsuite/tests/simplCore/should_compile/T7702plugin/Makefile

Loading...

testsuite/tests/typecheck/bug1465/Makefile

Loading...

utils/ghc-pkg/Main.hs

Loading...

utils/haddock

Loading...

Add Comment