Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce qualified constraints in the dependency solver. #4257

Merged
merged 10 commits into from
Feb 3, 2017

Conversation

grayjay
Copy link
Collaborator

@grayjay grayjay commented Jan 22, 2017

This PR is a continuation of #4236. I made two changes to ConstraintScope before enforcing the constraints in the solver:

  • I removed the Namespace from the ScopeQualified constructor. This change doesn't affect the behavior of the solver, because we currently don't use Namespace anywhere.
  • I added a ScopeAnySetupQualifier constructor to allow constraints to apply to all setup scripts. The new constructor is only used internally in this PR.

This PR also adds dependencies with version ranges to the solver DSL. I ended up not needing the version ranges for these unit tests, but I left them because I think they could be useful.

/cc @robjhen @kosmikus

EDIT:

/cc @dcoutts @23Skidoo @ezyang @Ericson2314 @hvr @phadej

Here is a summary of the changes, from the changelog:

  • Added qualified constraints for setup dependencies. For example,
    --constraint="setup.bar == 1.0" constrains all setup dependencies on
    bar, and --constraint="foo:setup.bar == 1.0" constrains foo's setup
    dependency on bar (part of Constraint syntax is not expressive enough. #3502).
  • Non-qualified constraints, such as --constraint="bar == 1.0", now
    only apply to top-level dependencies. They don't constrain setup or
    build-tool dependencies. The new syntax --constraint="any.bar == 1.0"
    constrains all uses of bar.

@mention-bot
Copy link

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

@grayjay grayjay force-pushed the enforce-qualified-constraints branch 2 times, most recently from 93e42ff to df67466 Compare January 22, 2017 23:02
@grayjay
Copy link
Collaborator Author

grayjay commented Jan 27, 2017

Since the release is coming up, I wanted to figure out what changes relating to qualified constraints we should include. I can see three options that wouldn't take long to implement:

  1. Disable the parsing of setup qualified constraints so that no changes go into 2.0. We'll be able to choose the syntax later, possibly after we have component-based solving.
  2. Merge this pull request so that we have these changes in 2.0:
    • new setup qualified constraints, such as foo:setup.bar == 1.0
    • Non-qualified constraints, such as bar == 1.0, only apply to top-level dependencies.
    • There is no longer a way to constrain build tool dependencies, or all setup dependencies, which means that a user can't prevent cabal from installing a particular package.
  3. Merge this pull request and add a new syntax for constraints that apply to all dependencies. The previous PRs added that type of constraint but only used it internally. They are currently displayed as any.bar == 1.0 in the log.

I prefer 1 and 3, because I don't want to take away the ability to constrain any dependency.

@23Skidoo
Copy link
Member

@grayjay

I prefer 1 and 3, because I don't want to take away the ability to constrain any dependency.

+1 from me. If we can implement 3 in time, then let's go with 3, otherwise we can postpone merging this PR.

@ezyang
Copy link
Contributor

ezyang commented Jan 27, 2017

OK, I'm going to mark this as action-required for now. @grayjay, are you on point for (3)?

@grayjay
Copy link
Collaborator Author

grayjay commented Jan 28, 2017

Yes, I'll try to implement (3) this weekend. I'll add it to this PR.

This commit changes the meaning of constraints like 'pkg > 5'. Previously,
'pkg > 5' only applied to the top-level 'pkg' goal without a 'Namespace', but
now it applies to any top-level goal for 'pkg'. However, the 'Namespace' field is
currently only used by --independent-goals, so this change has no effect on
behavior.
I used the new constraint scope to enforce the minimum Cabal version in setup
scripts that lack a 'custom-setup' stanza.  It isn't exposed to users yet.
…dency graph.

For example, --constraint="any.pkg == 5" applies to "pkg" whether it is a
top-level dependency, setup dependency, or build tool dependency.

I also modified the UserConstraint type so that it is more similar to the
PackageConstraint type, now that both types need to express similar
"constraint scopes".
For example, "setup.Cabal installed" forces cabal to use the installed Cabal
library for all setup scripts.
@grayjay grayjay force-pushed the enforce-qualified-constraints branch from df67466 to ff4c548 Compare January 30, 2017 00:26
@grayjay
Copy link
Collaborator Author

grayjay commented Jan 30, 2017

I exposed the constraints that apply to all dependencies, as well as constraints that only apply to setup dependencies. The current syntax is:

--constraint="setup.Cabal >= 1.24" - constrain Cabal in all setup scripts
--constraint="any.Cabal >= 1.24" - constrain all uses of Cabal

@Ericson2314
Copy link
Collaborator

Ericson2314 commented Jan 30, 2017

@grayjay this looks great! But say there is a package called setup or any? 😛 That would cause problems in my full-qualifier-paths future.

edit Er nope, because lib dependencies cannot be constrained, unless we get full private lib dependencies. By that point, I think we'll have full component-base solving too.

@grayjay
Copy link
Collaborator Author

grayjay commented Feb 1, 2017

@Ericson2314 Thanks for reviewing the syntax. Maybe we should reserve the package names "any" and "setup" to allow more flexibility for future changes to constraint syntax.

@Ericson2314
Copy link
Collaborator

@grayjay well I'm thinking we can use my arrow notation with * and ** wildcards which can express everything, and then we won't need anything behind what we currently have as short-hands.

If we can reserve package names at this point, that's cool, though.

@ezyang ezyang merged commit 9aa4b9f into haskell:master Feb 3, 2017
@ezyang
Copy link
Contributor

ezyang commented Feb 3, 2017

I went ahead and merged this. Hopefully we can reserve any/setup in the near future; I don't actually know how to go about doing this.

@23Skidoo
Copy link
Member

23Skidoo commented Feb 3, 2017

Someone who's familiar with hackage-server code needs to step in.

@grayjay
Copy link
Collaborator Author

grayjay commented Feb 5, 2017

Thanks!

@grayjay well I'm thinking we can use my arrow notation with * and ** wildcards which can express everything, and then we won't need anything behind what we currently have as short-hands.

I'm not sure I understand. Are you suggesting using asterisks instead? Then we would have something like *.Cabal or **.Cabal instead of any.Cabal.

@grayjay grayjay deleted the enforce-qualified-constraints branch February 5, 2017 03:07
@Ericson2314
Copy link
Collaborator

Ericson2314 commented Feb 9, 2017

@grayjay err nah I wasn't trying to subtly imply that :).

I was thinking if all single constraint path edges can be written (component -> component), * could be a wildcard within the component, and ** could be a wildcard over 0 or more edges.

  • any.foo = **.foo
  • setup.foo = **(*->*:setup).foo

** does subsume any (I assume any can only be used at the beginning unlike ** though), but any may still be nice to have as a more-discoverable English term that mirrors setup.

@hvr
Copy link
Member

hvr commented Jul 10, 2017

Hopefully we can reserve any/setup in the near future; I don't actually know how to go about doing this.

Someone who's familiar with hackage-server code needs to step in.

@23Skidoo @ezyang Fyi, since yesterday, Hackage rejects package names such as "all", "setup", "any" or "none"

@Ericson2314
Copy link
Collaborator

Great!

@grayjay
Copy link
Collaborator Author

grayjay commented Jul 15, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants