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

[ghost]

In this PR:

• Non-upgradeable package constraints now use ScopeAnyQualifier.
• Added documentation for qualified constraints. I've also refactored the documentation a bit as there was duplication of information between the description of the --constraint command line option and the constraints field of cabal.project (let me know if any objections to my moving things around).
• Added unit tests for parsing qualified constraints.

@grayjay You asked me before if there is anything else you'll need to do aside from enforcing the constraints in Distribution.Solver.Modular.Preference. Mainly just the following:

• I would advise briefly checking all instances where PackageConstraint / UserConstraint values are constructed and pattern-matched (you can just grep for these identifiers). Where they are constructed, check that the correct ConstraintScope has been applied (I have currently set them all to scopeTopLevel except for the non-upgradeable packages one that is set to ScopeAnyQualifier). Where they are pattern-matched, check if you need to handle the qualifier in any way (currently qualifiers are just being ignored at these places). Two cases of pattern matching that are particularly worth inspecting are packageVersionConstraintMap in Distribution.Client.Dependency and pcName in Distribution.Solver.Modular.
• If that's all okay, I'll leave it to you to add the changelog entry once the solver part is done!

[mention-bot]

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

[ezyang]

Thanks, we should definitely merge this.

The docs made it a lot clearer to me about what kind of syntax bikeshedding we might want to do for constraints. I think the current syntax is close; in particular, foo:setup.bar == 2.0 seems fine. So it's just the executable syntax that seems bad. The big problem is that the syntax is not parallel: foo:setup says, "the setup component of the foo package" (this is true both with new-build syntax and here), but foo:bar:exe says, "the bar build-tools dependency of ALL components in foo". So, if x.y means, "the y package referenced by entity x", then to say something like "the baz referenced by the executable bar referenced by the build-tools of foo", we actually want a syntax like foo.barpkg:exe:bar.baz. Except I don't entirely like this because there is this new barpkg thing and maybe if you build-tools-depend on multiple executables from the same package you want to end up picking the same version (but maybe not?!)

In part, this is a bit related to the underlying model of how much flexibility the solver should have in choosing the dependencies for executables. One way to design the system is to be as flexible as possible: then, it should be possible to single out how we are depsolving a particular executable from a particular build-tools-dependency of a particular package. But maybe that's too much and a user who is actually wanting to exert some control here only cares about, "well, I just want to change what dep alex is using." In that case, the syntax should simplify to handle that case.

Does this make sense?

[ezyang]

So, let me give a concrete suggestion. It should be possible to say exe:alex.alexdep >= 2.0, or even pkg:exe:subexe.subexedep, to apply a constraint to ALL versions of alex or subexe (from pkg) which are in our dependency plan. And maybe that should be the only user visible knob available, for now.

[grayjay]

LGTM. Thanks!

I checked the use sites of the new types, and I think that the only ones that need to be updated are Distribution.Solver.Modular.Preference and Distribution.Client.CmdFreeze.projectFreezeConstraints.

Two cases of pattern matching that are particularly worth inspecting are packageVersionConstraintMap in Distribution.Client.Dependency and pcName in Distribution.Solver.Modular.

packageVersionConstraintMap should be able to ignore constraint qualifiers because resolveWithoutDependencies doesn't use qualifiers when choosing packages. pcName is used for indexing the constraints used by the solver. I think it is okay for pcName to throw out the qualifier and just use the PackageName as a key, because there probably won't be a large number of constraints for each package name.

If that's all okay, I'll leave it to you to add the changelog entry once the solver part is done!

Yes, the changelog can wait.

[grayjay]

This needs another newline after the "::" to form a code block.

[grayjay]

to build

[grayjay]

I like the idea of trying to make the syntax more similar to new-build's targets. We may also want to make it consistent with the implication constraints from @dcoutts's package-collections branch: 657448b#diff-2a65a1064ba8104a25e44b665f4aabf5R193

So, let me give a concrete suggestion. It should be possible to say exe:alex.alexdep >= 2.0, or even pkg:exe:subexe.subexedep, to apply a constraint to ALL versions of alex or subexe (from pkg) which are in our dependency plan. And maybe that should be the only user visible knob available, for now.

So this syntax would apply to all non-lib components? It seems similar to independent goals qualifiers, which allow different targets to have different dependency versions.

[ghost]

@ezyang @grayjay If/when you need to change the syntax, it should be easy to do. Just update the following four sections of code:

1. Pretty-printing: dispQualifier in Distribution.Solver.Types.PackagePath (and potentially also dispConstraintScope in Distribution.Solver.Types.PackageConstraint, although that doesn't currently affect user constraints).
2. Parsing: Text instance of UserConstraint in Distribution.Client.Targets.
3. Parsing unit test: exampleConstraints in UnitTests.Distribution.Client.Targets.
4. Documentation: one paragraph on qualified constraints in the --constraint option section of doc/installing-packages.rst.

The syntax that we've got currently was never intended as final, and I agree it could be improved, particularly the exe qualifier as you pointed out.

[ezyang]

@grayjay How do the independent goal qualifiers look like? :) Oh, those are the like, goal1.blah things? Yes, something like that, even though internally we have something more flexible.

[grayjay]

@grayjay How do the independent goal qualifiers look like? :) Oh, those are the like, goal1.blah things? Yes, something like that, even though internally we have something more flexible.

Yes, the independent goals are numbered internally, but I imagine they would look similar to your suggested syntax if we ever exposed them.

In part, this is a bit related to the underlying model of how much flexibility the solver should have in choosing the dependencies for executables. One way to design the system is to be as flexible as possible: then, it should be possible to single out how we are depsolving a particular executable from a particular build-tools-dependency of a particular package.

I meant to add that the current syntax for build tool dependencies, pkg1:pkg2:exe.pkg3, captures everything that the solver uses internally now, but with per-component solving, that will change. I think that the solver should have maximum flexibility when choosing dependencies for separate executables. Internally, the solver will probably need a component (of any type) instead of pkg1, and an executable component instead of pkg2. I think that your idea would fit well with that design. We don't want users to always have to specify everything, like pkg1:ctype1:comp1-pkg2:exe:comp2.pkg3. We can just expose the second half of the internal qualifier until there is a use case that requires more targeted constraints. We could also simplify it further, like you suggested, i.e., pkg2:exe.pkg3 or exe:comp2.pkg3.

[Ericson2314]

Hmm, found this. I wrote up a plan for cross compiling involving changing the qualifier type to a list: #1493 (comment) . I figure this is quite relevant to bike shedding syntax.

[Ericson2314]

Ok, so if understand this stuff right, qualifying comes from "encapulating" edges in the dependency DAG. If we think about solving per-package, these encapuslating edges are the single connection between between otherwise-disconnected subgraphs.

Solving per-component is weirder as each non-setup component would have and edge to the setup component coming from the same package, though technically those exes could be configured separately so not the same node restoring single connection property? Let's assume that for the following so we have the single-connection property.

Today we do some "pruning" (I talk about this in the linked comment), but a qualification is a chain of these encapsulating edges that must be traversed to reach the package in question. [Formally, N is the set of nodes, D is the set of dependency edges, and E is the subset of encapsulating edges, then N' = N / ((D \ E)≡) (N quotiented by conflating any two nodes that can be reached via non-encapsulating edges) is a set of almost-disconnected components, and nodes of a tree whose edges are isomorphic to E.] The pruning would be quotienting the space of qualifications in some fashion so the search space for the solver is less big

Now if we do want to be less flexible, or the single-edge property doesn't hold, that's means we don't have a tree, so qualifications do not uniquely reach a node. I guess we have to whittle down the space of connections then---which incidentally this pruning might do.

Anyways, back to the syntax, it might make sense to again forget about the is pruning, and except that syntactically different constraints may in fact conflict. Maybe I'm wrong and actually that's a horrible design, but for the sake of argument let me try to give a grammar for the full unadulterated paths-as-qualifiers for components.

Component ::= 'exe' ':' UnqualComponentName
              -- or, like elsewhere, `$pkg:lib:$pkg` is the default lib?
           |  'lib' ':' UnqualComponentName?
              -- Allow an optional trailing semicolon for consistency with `$pkgs:lib:` ?
           |  'setup'

PackageComponent ::= Package ':' Component

Edge ::= PackageComponent '->' Package ':' 'exe' ':' UnqualComponentName
      |  PackageComponent '->' Package ':' 'setup'
      |  'base' '->' Package ':' 'lib' ':' UnqualComponentName -- not exposed

Bound ::= -- ...Just like today

Qualifier ::= ('(' Edge ')' '.')*

Constraint ::= Qualifier Package Bound

[grayjay]

@Ericson2314 The solver previously allowed unlimited qualifiers, but they were removed in #3220 because of a problem with cyclic dependencies. If we need more qualifiers, there might be other ways to prevent the list from growing too long, such as a fixed bound on the length.

Edge ::= PackageComponent '->' Package ':' 'exe' ':' UnqualComponentName
| PackageComponent '->' Package ':' 'setup'
| PackageComponent '->' 'base' -- do we even want to expose this?

I like the use of arrows here.

[grayjay]

I made a pull request (#4252) to disallow the exe qualifier until we decide on syntax. I left setup, because, even if we decide to handle setup scripts more similarly to other build tools, I think it would be nice to also be able to use the shorter syntax. For example, pkg1:setup.pkg2 could mean both pkg1:lib->pkg1:setup.pkg2 and pkg1:exe:exe1->pkg1:setup.pkg2 if pkg1 had a library and an executable. What do you think? Is there any reason to also wait to expose setup?

[hvr]

Btw, why do we use this arbitrary > 1 && < 1 contradiction instead of the more canonical < 0 constraint?

[ezyang]

Patches accepted :) But -none is literally implemented this way:

noVersion :: VersionRange
noVersion = IntersectVersionRanges (LaterVersion v) (EarlierVersion v)
  where v = mkVersion [1]

We could change this as long as it is actually impossible to have negative version numbers :)

[ezyang]

Wow, did we seriously apply this to top-level only before? Worth a comment!

[grayjay]

scopeTopLevel was only added in #4219 (originally unqualified). Before that, every constraint applied to all occurrences of the package, so the constraints above always prevented base, etc. from being installed.

[ezyang]

Yeah, I have to say, if I saw this in a source code file, I would definitely have a hard time telling what the bar is supposed to mean.

[ezyang]

I think this is still useful advice to reiterate here, since it's not obvious from the command line flag discussion.

[ezyang]

Patches accepted :) But -none is literally implemented this way:

noVersion :: VersionRange
noVersion = IntersectVersionRanges (LaterVersion v) (EarlierVersion v)
  where v = mkVersion [1]

We could change this as long as it is actually impossible to have negative version numbers :)