Make more dependency types #4067
Conversation
|
@Ericson2314, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dcoutts, @phadej and @ezyang to be potential reviewers. |
|
CI failures are real. If you try compiling with -Werror that should help you catch the warnings (CI will fail unless you are warnings-free.) |
|
CC @phadej especially on b729cff ; @Ericson2314 spotted an inconsistency with the Parsec parser and the old parser: whether or not quoted strings in build-tools are allowed. |
| @@ -19,6 +19,7 @@ | |||
| -- This module is meant to be local-only to Distribution... | |||
|
|
|||
| {-# OPTIONS_HADDOCK hide #-} | |||
| {-# LANGUAGE Rank2Types #-} | |||
There was a problem hiding this comment.
OK, excellent use of rank 2 types :)
| @@ -138,6 +141,39 @@ unPackageName = unUnqualComponentName . packageNameToUnqualComponentName | |||
| mkPackageName :: String -> PackageName | |||
| mkPackageName = PackageName . mkUnqualComponentName | |||
|
|
|||
| -- | A pkgconfig library name | |||
There was a problem hiding this comment.
But what is a pkgconfig library name?! Answer: "a valid argument to the pkg-config utility".
| @@ -317,19 +353,24 @@ mkLegacyUnitId = newSimpleUnitId . mkComponentId . display | |||
| data Dependency = Dependency PackageName VersionRange | |||
| deriving (Generic, Read, Show, Eq, Typeable, Data) | |||
|
|
|||
| instance Binary Dependency | |||
| -- | Describes a legacy `build-tools`-style dependency on an executable | |||
There was a problem hiding this comment.
It is "legacy" because we do not know if the build-tool referred to, refers to a pkg-config executable, or an internal executable (thus it is stringly typed.)
| -- | Describes a legacy `build-tools`-style dependency on an executable | ||
| -- | ||
| data LegacyExeDependency = LegacyExeDependency | ||
| String -- ^ name of one of the hardcoded understood tools |
There was a problem hiding this comment.
Not always a hardcoded tool, right?
| @@ -0,0 +1,56 @@ | |||
| -- | *Dependency Text instances moved from Distribution.Package | |||
| {-# OPTIONS_GHC -fno-warn-orphans #-} | |||
There was a problem hiding this comment.
OK for now. Let's refactor it away later.
|
Ah to be I opened up a separate PR for the parser commit so @phadej can ignore this. |
Is this an intentional inconsistency with the old parser?
Ericson2314
force-pushed
the
dep-types
branch
4 times, most recently
from
59b9d20 to
4cad7b0
Compare
There's a haddock error but not sure what's wrong. Would |
|
I guess so. Add it and see? |
|
Ah, need a record for per-field documentation. https://www.haskell.org/haddock/doc/html/ch03s02.html |
Plain `Dependency` should strictly refer to a Haskell Package, usually the
library component of that package.
Ericson2314
force-pushed
the
dep-types
branch
from
4cad7b0 to
b63ebe1
Compare
|
No point documenting per-variant if there's only one, so I removed the |
|
@ezyang also despite the not-invalidated diffs aboved I think I fixed everything you mentioned if you want to toggle your review :). |
Ericson2314
changed the title
This is a follow-up to #4057, so until that is dealt with this diff will also be humongous and unreviewable.
This PR makes separate types for different types of dependencies:
Dependencyis now just for regular (usually library) dependencies on haskell packagesLegacyExeDependecyfor legacybuild-toolsdependenciesPkgconfigDependecyfor pkg-config dependenciesAdditionally, there is a newtype
PkgconfigNamefor pkg-config library names. For Cabal's uses alone this wouldn't be worth it, but cabal-install already distinguishes these libraries widely withPackageName. Continuing to usePackageNameseemed inconsistent with the newly-rigorous use ofDependecy, while going back toStringseemed like a step backwards.Plain
Stringis used for legacy built tool names, as those can mean quite different things, and very little code was usingPackageNameand notStringfor them.Note I had to make some orphan instances (module-wise, not package-wise) in order to avoid a
Distribution.ParserUtils<->Distribution.Packagecyclic dep. This can go away once the old parser is removed.