Target package names #4889

dcoutts · 2017-11-14T00:42:08Z

Much improved support for named packages in the target selector syntax and resolution. Instead of just supporting syntax of a single unadorned package name, all the existing syntax that allow package names are extended to support named packages.

This includes packages from the extra-packages but also indirect deps and packages from hackage. The resolution for extra-packages and indirect deps should just work, while currently packages outside the project are reported as an error (but we're in a good position now to resolve to packages from hackage if we want).

So this should be a better basis for the target handling for the new install code, since any command should now be able to refer to packages outside of the project (or at least we're a short step from tha now).

This passes the existing target selector integration tests. New tests need to be added to cover the extra-packages and indirect deps within the project, and packages outside the project.

This does extend what the existing syntax can refer to so, docs & changelog will need to be updated.

BTW, this PR is designed to be read patch-by-patch, not all in one go. Each change is hopefully comprehensible on its own.

Please include the following checklist in your PR:

Patches conform to the coding conventions.
Any changes that could be relevant to users have been recorded in the changelog.
The documentation has been updated, if necessary.
If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

Instead of two constructors MatchExact and MatchInexact we now have one Match constructor with a field of data MatchClass = Inexact | Exact. This factoring helps simplify several other functions where we treat them uniformly or according to the Ord instance. The real ulterior motive however is that we'd like to introduce a third Match class value between Inexact and Exact. This refactoring is a prerequisite to that, to avoid a combinatorial explosion of cases.

Bundle up what we had been passing as separate args into a single KnownTargets type, and separate out some utils for constructing it.

To KnownPackage and KnownComponent. This is in preparation for extending KnownPackage by adding more constructors for known package names.

Previously the TargetSelector type had a type param for the type of the package that it referred to. In particular we used it with types like: type Matcher = ... -> Match (TargetSelector KnownPackage) type Renderer = TargetSelector PackageId -> ... However we are about to extend the TargetSelector so that it does not just refer to one form of package (e.g. KnownPackage) but can refer to packages via various different forms and partial information. So it no longer makes sense to have TargetSelector be paramaterised by the different states of the one kind of package it refers to, as there are now many kinds. So in preparation for that we simplify it so that it is equivalent to always using TargetSelector PackageId, and we remove the type paramater.

It is unnecessary now that we do not put the whole KnownPackage into the TargetSelector. It not being recursive also makes it possible to have a Show instance, which is handy.

That is, the TargetPackage instead of having a single PackageId contains a [PackageId]. Ultimately this will allow us to support multiple .cabal files in a single directory. But the real reason to do this generalisation now is that it helps with the TargetImplicitCwd case. For the implicit CWD case we need to be able to parse the target whether or not there is a package in the CWD. So the simplest solution is to pass in all the local CWD packages (though typically only 0 or 1) and put all of them in. Then at the end we can check if in fact there were 0 and fail. When we do want to support multiple .cabal files in a dir, we'll also need to adjust the project config code, and extend the syntax slightly so that we render as the package location for the case of multiple packages.

We are about to generalise KnownPackage in such a way that it can no longer be an instance of Package. The extra constructor will only hold a PackageName, not a full PackageId. In addition, whenever we get a KnownPackage, we do case analysis and record pattern matching to select the bits we need. This is in preparation for adding the second constructor. We switch to record pattern matching rather than field names as functions since those functions will become partial once we add the second constructor.

We'll need display of component kind both for component names (as now) and also directly for ComponentKind. Rename so the naming is sane.

Add TargetPackageNamed, like TargetPackage but for known packages within the project that are only specified by name. This includes the extra-packages from the @cabal.project@ file. It does not include indirect deps or other packages from hackage. That will be covered by a separate constructor. This replaces the previous TargetPackageName constructor which was part of a much more limited implementation of the same general idea.

Via the new KnownPackage and TargetPackageNamed stuff.

Easier to understand and document when it's a top level function.

The PackageId and ComponentName are already part of the existing AvailableTarget record argument. We need to eliminate this redundancy because for new kinds of target selectors we will not have the PackageId and ComponentName from the selector, only from the AvailableTarget selected.

Improve clarity and reduce code duplication a bit.

For local packages we have the full .cabal file so we know exactly what components are available. For packages named in extra-packages we do not know the components until after solving, which is after the target parsing stage. So this adds partial support parsing targets that refer to components of extra-packages, even though we do not yet know if the component exists.

This allows for referring to packages outside of the project, e.g. for cabal (new-)install to refer to packages from hackage. At the moment it still reports an error message that the package is not known, but this error is reported at an appropriate point where we could resolve the package name to a package from some wider context such as a hackage archive.

23Skidoo · 2017-11-21T16:46:04Z

Travis still red... looks like a failure in the CmdRun test.

Trivial change, but actually a wording improvement.

fgaz · 2017-11-22T08:34:18Z

The failing one is #4644 again

dcoutts · 2017-12-24T18:38:48Z

Is this a prior failing test? Is this one I need to fix?

23Skidoo · 2017-12-24T18:40:18Z

CI seems to be green now.

23Skidoo

LGTM modulo some minor comments.

23Skidoo · 2017-12-25T15:32:41Z

cabal-install/Distribution/Client/TargetSelector.hs

-data Match a = NoMatch      Confidence [MatchError]
-             | ExactMatch   Confidence [a]
-             | InexactMatch Confidence [a]
+data Match a = NoMatch           !Confidence [MatchError]


BTW, I noticed that we have exactly the same thingy in Cabal/Distribution/Simple/BuildTarget.hs, perhaps these two can be merged?

23Skidoo · 2017-12-25T15:58:02Z

cabal-install/Distribution/Client/TargetSelector.hs

-data MaybeAmbiguous a = None [MatchError] | Unambiguous a | Ambiguous Bool [a]
+  NoMatch _ msgs -> None msgs
+  Match _ _  [x] -> Unambiguous x
+  Match m d   [] -> error $ "findMatch: impossible: " ++ show match'


May be worth using Data.List.NonEmpty here, though we'll have to Distribution.Compat. that module. Or we can just add a TODO.

I'd vote for a TODO; this won't be the only place benefiting from NonEmpty

23Skidoo · 2017-12-25T16:20:58Z

cabal-install/Distribution/Client/TargetSelector.hs

@@ -1560,6 +1548,15 @@ dispM = display
 -- Package and component info
 --

+data KnownTargets = KnownTargets {


Would be nice with a Haddock comment here.

23Skidoo · 2017-12-25T16:26:12Z

cabal-install/Distribution/Client/TargetSelector.hs

-syntaxForms :: [PackageInfo] -> [PackageInfo] -> Syntax
-syntaxForms ppinfo opinfo =
+syntaxForms :: KnownTargets -> Syntax
+syntaxForms KnownTargets {


Since we're using RecordWildcards, this can be rewritten as KnownTargets {..} = ... knownPackagesAll ....

23Skidoo · 2017-12-25T16:53:01Z

cabal-install/Distribution/Client/TargetSelector.hs

-          Left ((m, ms):_) -> Left (MatchingInternalError targetStr
-                                       (fmap packageId m)
-                                       (map (fmap (map (fmap packageId))) ms))
+          Right targets'   -> Left (TargetSelectorAmbiguous targetStr targets')


23Skidoo · 2017-12-25T21:23:03Z

cabal-install/Distribution/Client/ProjectOrchestration.hs

@@ -476,6 +476,12 @@ resolveTargets selectPackageTargets selectComponentTarget liftProblem
      | otherwise
      = Left (liftProblem (TargetProblemNoSuchPackage pkgid))

+    checkTarget (TargetPackage _ _ _)
+      = error "TODO: add support for multiple packages in a directory"


I wonder how this use case will play together with the --cabal-file option of configure which allows using multiple .cabal files for a single package.

23Skidoo · 2017-12-25T21:33:32Z

cabal-install/Distribution/Client/TargetSelector.hs

-      orNoThingIn "component" (cinfoStrName c) $ do
-        (filepath,_) <- matchComponentFile [c] str7
-        return (TargetComponent (packageId p) (cinfoName c) (FileTarget filepath))
+    case p of


Why not use KnownPackage{pinfoId, pinfoComponents} <- matchPackage ...? That'd make the diff a bit smaller.

OK, I see, because of the addition of KnownPackageName in a later commit.

23Skidoo · 2017-12-25T21:49:16Z

cabal-install/Distribution/Client/TargetSelector.hs

@@ -149,19 +148,19 @@ data TargetSelector =
     --
     TargetPackage TargetImplicitCwd [PackageId] (Maybe ComponentKindFilter)

+     -- | A package within the project speciied by name. This includes the


s/speciied/specified/

23Skidoo · 2017-12-25T22:12:33Z

cabal-install/Distribution/Client/TargetSelector.hs

@@ -1669,13 +1668,15 @@ getKnownTargets dirActions@DirActions{..} pkgs = do


 collectKnownPackageInfo :: (Applicative m, Monad m) => DirActions m
-                        -> SourcePackage (PackageLocation a)
+                        -> PackageSpecifier (SourcePackage (PackageLocation a))


Re: commit message: TargetSpecifier is not a thing, you meant either PackageSpecifier or TargetSelector.

23Skidoo · 2017-12-25T23:18:20Z

cabal-install/Distribution/Client/TargetSelector.hs

@@ -1104,10 +1116,14 @@ syntaxForm2PackageComponent ps =
          return (TargetComponent pinfoId (cinfoName c) WholeComponent)
        --TODO: the error here ought to say there's no component by that name in


Is this TODO still valid?

23Skidoo · 2017-12-25T23:50:55Z

@dcoutts Do you want to merge this as-is or do you want to write some additional tests before merging? The latter probably makes more sense once the feature is fully implemented.

23Skidoo · 2018-01-03T14:53:25Z

I'm merging this now, because the PR looks quite good on the whole and there's a chance it will start bitrotting.

sboosali · 2019-03-09T07:29:13Z

@dcoutts I got here via @hvr 's comment on #5444 about an "enhancement to its [extra-package's] syntax". could you clarify if you have a moment? (I want to port over a few Nix-based and Stack-based projects, which a few different ways of defining additional vendored packages.)

dcoutts added 16 commits November 5, 2017 18:06

Refactor: add KnownTargets type to target selection code

271d3ab

Bundle up what we had been passing as separate args into a single KnownTargets type, and separate out some utils for constructing it.

Rename PackageInfo and ComponentInfo

9724c46

To KnownPackage and KnownComponent. This is in preparation for extending KnownPackage by adding more constructors for known package names.

Make the KnownPackage/KnownComponent construction non-recursive

0cdaeaa

It is unnecessary now that we do not put the whole KnownPackage into the TargetSelector. It not being recursive also makes it possible to have a Show instance, which is handy.

Add and rename a utility

5ebae3e

We'll need display of component kind both for component names (as now) and also directly for ComponentKind. Rename so the naming is sane.

Plumb extra-packages through to TargetSpecifier

493f5fd

Via the new KnownPackage and TargetPackageNamed stuff.

Refactor: split a util out of resolveTargets

2f8abd7

Easier to understand and document when it's a top level function.

A bit of refactoring in resolveTargets

ca5d7f4

Improve clarity and reduce code duplication a bit.

Remove a couple done TODOs for target selection

4760f24

dcoutts requested review from fgaz, hvr, ezyang and 23Skidoo November 14, 2017 00:42

Fix compat for various ghc versions

7996ed5

Update the test CLI output in one case

8ffed56

Trivial change, but actually a wording improvement.

23Skidoo approved these changes Dec 25, 2017

View reviewed changes

23Skidoo mentioned this pull request Jan 1, 2018

Duplicate build-tools-depends/build-depends chokes new-build #4986

Closed

23Skidoo merged commit b5f9914 into master Jan 3, 2018

23Skidoo deleted the target-package-names branch January 3, 2018 14:53

hvr mentioned this pull request Jul 19, 2018

source-repository-package not optional #5444

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Target package names #4889

Target package names #4889

dcoutts commented Nov 14, 2017 •

edited

Loading

23Skidoo commented Nov 21, 2017

fgaz commented Nov 22, 2017

dcoutts commented Dec 24, 2017

23Skidoo commented Dec 24, 2017

23Skidoo left a comment

23Skidoo Dec 25, 2017

23Skidoo Dec 25, 2017

hvr Dec 27, 2017

23Skidoo Dec 25, 2017

23Skidoo Dec 25, 2017

23Skidoo Dec 25, 2017

23Skidoo Dec 25, 2017

23Skidoo Dec 25, 2017

23Skidoo Dec 25, 2017

23Skidoo Dec 25, 2017

23Skidoo Dec 25, 2017

23Skidoo Dec 25, 2017

23Skidoo commented Dec 25, 2017

23Skidoo commented Jan 3, 2018

sboosali commented Mar 9, 2019

		@@ -1104,10 +1116,14 @@ syntaxForm2PackageComponent ps =
		return (TargetComponent pinfoId (cinfoName c) WholeComponent)
		--TODO: the error here ought to say there's no component by that name in

Target package names #4889

Target package names #4889

Conversation

dcoutts commented Nov 14, 2017 • edited Loading

23Skidoo commented Nov 21, 2017

fgaz commented Nov 22, 2017

dcoutts commented Dec 24, 2017

23Skidoo commented Dec 24, 2017

23Skidoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

23Skidoo commented Dec 25, 2017

23Skidoo commented Jan 3, 2018

sboosali commented Mar 9, 2019

dcoutts commented Nov 14, 2017 •

edited

Loading