Qualified constraints (issue #3502) #4219

ghost · 2017-01-07T20:36:01Z

The main changes are:

Updated the syntax of qualifiers in PackagePath.hs to use colon as a separator rather than hyphen.
Changed PackageName to QPN in PackageConstraint.
Added 'UserQualifier' into UserConstraint.

The only difference to the overall behaviour of cabal due to this patch is that qualified constraints will be parsed on the command line. Currently the solver etc. just ignores these qualifiers, however everything should be all set now to make the solver act on them.

One last thing: I'm going to add some new unit tests for the UserConstraint parser, but if it's okay I'll leave that till the next submission to stop this one getting too big.

I modified the pretty-printing code so that it uses the 'Text.PrettyPrint' system rather than raw strings. I updated the syntax of pretty-printed qualifiers to use colons as separators rather than hyphens to fix an ambiguity problem (since hyphens can occur in package names). See issue 3502.

Refactored PackageConstraint in two ways: 1) split it into a package name and a 'PackageProperty' to make the code a bit cleaner; 2) changed PackageName to 'Qualified PackageName'. Added a Binary instance for Qualifier in PackagePath.hs (needed for PackageConstraint). Added pretty-printing code for PackageConstraint. For now, all the code that creates a PackageConstraint just sets the qualifier to 'unqualified', so this commit will not change the external behaviour of cabal-install.

Changed PackageConstraint to PackageProperty in both cases, since the name in the PackageConstraint was redundant.

This eliminates some boilerplate code.

Amended parsing and pretty-printing code of UserConstraint to handle qualifiers. Qualified constraints are now accepted on the command line, but the solver and other subsystems currently just ignore the qualifiers and don't do anything differently from before.

mention-bot · 2017-01-07T20:36:03Z

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

23Skidoo · 2017-01-07T20:49:29Z

/cc @kosmikus

23Skidoo · 2017-01-07T21:02:57Z

AppVeyor failure seems to be genuine:

tests\UnitTests\Distribution\Client\ProjectConfig.hs:567:10: error:
    Duplicate instance declarations:
      instance Arbitrary UserConstraint
        -- Defined at tests\UnitTests\Distribution\Client\ProjectConfig.hs:567:10
      instance Arbitrary UserConstraint
        -- Defined at tests\UnitTests\Distribution\Client\ProjectConfig.hs:577:10

grayjay

This is looking great! Thanks for working on this feature and refactoring the surrounding code. Is there anything else that needs to be done, besides enforcing the constraints in Distribution.Solver.Modular.Preference?

grayjay · 2017-01-08T20:32:58Z

cabal-install/Distribution/Client/Dependency.hs

@@ -346,7 +348,7 @@ dontUpgradeNonUpgradeablePackages params =
  where
    extraConstraints =
      [ LabeledPackageConstraint
-        (PackageConstraintInstalled pkgname)
+        (PackageConstraint (unqualified pkgname) PackagePropertyInstalled)


These constraints should still apply to dependencies with any qualifier. I would expand PackageConstraint so that it can take either a qualified or unqualified PackageName, because there will probably be other situations where it is useful to constrain a package everywhere it appears as a dependency.

It would also be nice to allow users to specify unqualified constraints, but I don't think that's necessary for this set of PRs. I think that --constraint pkg==2 should always apply to the top-level pkg dependency, as implemented in this PR. Once the solver part is done, it will fix an issue with bootstrapping packages such as time: #4154. (See the comment at #4154 (comment).)

Yes I see your point. This may require a little thought, so I'll make a new PR after this one. Would it make sense instead of expanding PackageConstraint to expand Qualifier by adding a new constructor? I see two obvious choices:

Let 'Unqualified' mean top-level dependency and add a new constructor called e.g. Any.

Let 'Unqualified' mean applies everywhere and add a new constructor Toplevel.

Option 1 seems somewhat confusing semantically, so maybe option 2 is better - what do you think?

On second thoughts, having looked at how Qualifier is used in the solver, I think my last suggestion was incorrect. So, if I understand you, I should modify PackageConstraint to support a wildcard or 'any' qualifier as an extra alternative, but leave the Qualifier and QPN datatypes as they are?

So, if I understand you, I should modify PackageConstraint to support a wildcard or 'any' qualifier as an extra alternative, but leave the Qualifier and QPN datatypes as they are?

Yes, the solver still needs QPN to refer to a single instance of a package in many places. It might be worth adding a new type for the scope of the constraint, since we'll probably need other scopes later, such as "all setup dependencies". For example:

data PackageConstraint = PackageConstraint ConstraintScope PackageProperty data ConstraintScope = Qualified QPN | AnyQualifier PackageName | AnySetupQualifier PackageName

Let 'Unqualified' mean applies everywhere and add a new constructor Toplevel.

I think Toplevel is a lot clearer than the current use of Unqualified. (I'm not suggesting a change for these PRs, though.)

grayjay · 2017-01-09T00:19:56Z

cabal-install/Distribution/Client/Targets.hs

     --
-     NamedPackage PackageName [PackageConstraint]
+     NamedPackage PackageName [PackageProperty]


grayjay · 2017-01-09T00:47:35Z

cabal-install/Distribution/Solver/Types/PackageConstraint.hs

+
+-- | A package constraint consists of a package plus a property
+-- that must hold for that package.
+data PackageConstraint = PackageConstraint QPN PackageProperty
  deriving (Eq, Show, Generic)

 instance Binary PackageConstraint


I think that your changes to PackageSpecifier removed the need for this instance and most of the new Binary instances.

Yes, I think you're right - I'll look into that.

grayjay · 2017-01-09T02:58:39Z

cabal-install/Distribution/Solver/Types/PackagePath.hs

+instance Binary Namespace
+
+-- | Pretty-prints a namespace. The result is either empty or
+-- ends in a period, so it can be prepended onto a package name.


Should this say "qualifier" instead of "package name"?

grayjay · 2017-01-09T03:26:28Z

cabal-install/Distribution/Client/Targets.hs

@@ -744,35 +733,42 @@ readUserConstraint str =
      ++ "either a version range, 'installed', 'source' or flags"


This message should probably mention that the package name is qualified.

ghost · 2017-01-09T11:08:09Z

Thanks very much! I'm going to merge this PR now, and then I'll create a new PR addressing the points you've made.

Regarding your question is there anything else that needs to be done, I think the top priority is to add in the ability to distinguish between 'any' and 'toplevel' qualifiers, as you pointed out. Probably best not to modify the solver until the any vs. toplevel distinction is done. After that I'll see if I can think of anything else that we may have missed.

23Skidoo · 2017-01-09T11:25:46Z

Do we need any documentation for this or is this purely an internal thing?

ezyang · 2017-01-09T14:06:41Z

Documentation for the new syntax would be very helpful. I am a little worried that it will look similar but be subtly different from the current component qualification syntax in new-build.

23Skidoo · 2017-01-09T14:14:18Z

We can still change the syntax, the feature is not in any released version.

ghost · 2017-01-09T14:18:25Z

@23Skidoo @ezyang Would you mind giving me a link to the code or info concerning the component qualification syntax, as I am not familiar with that feature. I'd like to compare it with the UserConstraint syntax. Yes you are right, once this is finalized we will need documentation for qualified constraints on the command line.

ezyang · 2017-01-09T14:26:19Z

http://cabal.readthedocs.io/en/latest/nix-local-build.html#cabal-new-build

ghost · 2017-01-09T18:38:16Z

Thanks.

Also, just a quick question to check I've understood the semantics of Exe qualifiers. Consider the following qualified package name:

Q (PackagePath DefaultNamespace
               (Exe (mkPackageName "foo") (mkPackageName "happy")))
  (mkPackageName "containers")

In the current provisional syntax this is pretty-printed as:

foo:happy:exe.containers

As I understand it, this means the following:

I want to build some package foo.
The happy build tool is needed to build foo.
The containers library is a dependency of happy.

So, for example, specifying the following constraint:

foo:happy:exe.containers ==0.4

means that when building foo, you must use a happy executable that has been linked against version 0.4 of the containers package.

My question is: have I understood this correctly?

ezyang · 2017-01-09T18:44:51Z

Yep.

grayjay · 2017-01-10T08:10:01Z

Do we need any documentation for this or is this purely an internal thing?

The changelog should also mention that the meaning of constraints that use the old syntax has changed. They applied to all dependencies in 1.24, but now they only apply to top-level dependencies.

ghost · 2017-01-12T21:37:49Z

See PR #4228.

@grayjay I've added a 'ConstraintScope' data type as you suggested. I've also renamed 'Unqualified' to 'Toplevel'.

hvr · 2017-01-16T09:24:36Z

Looking at #4219 (comment), can somebody explain to me the meaning of the .s and :s symbols in the path

foo:happy:exe.containers ==0.4

?

Is happy the executable name or is it the package name? In the former case it looks a bit confusing to me that :exe is postfixed to the executable name, as in the goal syntax it's the other way round (i.e. happy:exe:happy denotes the executable component; and the first happy occurence before the :exe is the package name there)

When do I use : delimiter, and when do I use a . delimiter?

ezyang · 2017-01-16T15:58:49Z

@hvr We are having discussion about this in #4236. I think the syntax definitely needs to change.

…ifiers. This commit comments out the part of haskell#4219 that parses build tool dependency qualifiers, to disable the feature until we finalize the syntax. It also comments out the part of haskell#4236 that tests the parsing.

…ifiers. This commit comments out the part of #4219 that parses build tool dependency qualifiers, to disable the feature until we finalize the syntax. It also comments out the part of #4236 that tests the parsing.

Robert Henderson added 5 commits January 7, 2017 11:13

Code cleanup: refactored 'PackageSpecifier' and 'PackageTarget'.

ee86697

Changed PackageConstraint to PackageProperty in both cases, since the name in the PackageConstraint was redundant.

Code cleanup: added Traversable instance for PackageTarget.

d2bf16e

This eliminates some boilerplate code.

Robert Henderson added 2 commits January 8, 2017 15:50

Corrected minor error (forgot to delete old instance).

b992fae

Corrected missing parentheses and added comment.

14b5e9d

ghost mentioned this pull request Jan 8, 2017

Constraint syntax is not expressive enough. #3502

Open

grayjay self-assigned this Jan 8, 2017

grayjay reviewed Jan 9, 2017

View reviewed changes

ghost merged commit 2a8e853 into haskell:master Jan 9, 2017

ghost deleted the issue-3502-part2 branch January 9, 2017 11:09

ghost mentioned this pull request Jan 12, 2017

Qualified constraints (issue #3502) part 2 #4228

Merged

grayjay mentioned this pull request Jan 21, 2017

Temporarily disallow constraints that have build tool dependency qualifiers. #4252

Merged

grayjay mentioned this pull request Jan 27, 2017

Qualified constraints: documentation and unit tests (issue #3502) #4236

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Qualified constraints (issue #3502) #4219

Qualified constraints (issue #3502) #4219

ghost commented Jan 7, 2017

mention-bot commented Jan 7, 2017

23Skidoo commented Jan 7, 2017

23Skidoo commented Jan 7, 2017

grayjay left a comment

grayjay Jan 8, 2017

ghost Jan 9, 2017

ghost Jan 9, 2017

grayjay Jan 10, 2017

grayjay Jan 9, 2017

grayjay Jan 9, 2017

ghost Jan 9, 2017

grayjay Jan 9, 2017

ghost Jan 9, 2017

grayjay Jan 9, 2017

ghost Jan 9, 2017

ghost commented Jan 9, 2017

23Skidoo commented Jan 9, 2017

ezyang commented Jan 9, 2017

23Skidoo commented Jan 9, 2017

ghost commented Jan 9, 2017

ezyang commented Jan 9, 2017

ghost commented Jan 9, 2017

ezyang commented Jan 9, 2017

grayjay commented Jan 10, 2017

ghost commented Jan 12, 2017

hvr commented Jan 16, 2017

ezyang commented Jan 16, 2017

		@@ -744,35 +733,42 @@ readUserConstraint str =
		++ "either a version range, 'installed', 'source' or flags"

Qualified constraints (issue #3502) #4219

Qualified constraints (issue #3502) #4219

Conversation

ghost commented Jan 7, 2017

mention-bot commented Jan 7, 2017

23Skidoo commented Jan 7, 2017

23Skidoo commented Jan 7, 2017

grayjay 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

ghost commented Jan 9, 2017

23Skidoo commented Jan 9, 2017

ezyang commented Jan 9, 2017

23Skidoo commented Jan 9, 2017

ghost commented Jan 9, 2017

ezyang commented Jan 9, 2017

ghost commented Jan 9, 2017

ezyang commented Jan 9, 2017

grayjay commented Jan 10, 2017

ghost commented Jan 12, 2017

hvr commented Jan 16, 2017

ezyang commented Jan 16, 2017