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

Allow specifying dependencies as a hash #193

Closed
wants to merge 4 commits into from

Conversation

tfausak
Copy link
Collaborator

@tfausak tfausak commented Aug 30, 2017

This pull request fixes #186. It changes the dependencies to allow hash values specifying the package as the key and the constraint as the value. Here are some allowable dependencies:

# As before, a single string is allowed.
dependencies: base ==4.10.0.0

# Also as before, a list of strings is allowed.
dependencies:
  - base ==4.10.0.0

# This changed! Now a hash is interpreted as "<package>: <constraint>".
dependencies:
  base: ==4.10.0.0

# Values aren't required.
dependencies:
  base:
  aeson: null

# Values can be hashes.
dependencies:
  local:
    path: the/path-to/local
  remote:
    github: user/repo
    ref: master

# Be careful with constraints that collide with YAML syntax.
dependencies:
  # base: >=4.10 # Error!
  base: '>=4.10'

# When there are duplicate keys, later keys win.
dependencies:
  base: ignored
  base: ==4.10.0.0

In #186 I originally suggested that the == constraint be implied if no constraints were given. In other words, base: 4.10.0.0 would mean base ==4.10.0.0. I chose not to implement that because it would require parsing constraints, which is hard.

@sol
Copy link
Owner

sol commented Sep 11, 2017

@tfausak Hey, sorry for the late reply (I was offline for a while).

In #186 I originally suggested that the == constraint be implied if no constraints were given. In other words, base: 4.10.0.0 would mean base ==4.10.0.0. I chose not to implement that because it would require parsing constraints, which is hard.

This syntax is actually what convinced me in favor of adding this feature. In addition, I was hopping that it will be possible to tighten (or loosen) constraints for specific dependencies, e.g.:

dependencies:
  base: 4.*

tests:
  ...
  dependencies:
    base: '>= 4.3'

What exactly do you mean with "hard" when it comes to parsing constraints? My assumption was that depending on Cabal will allow us to do this (not that I'm excited to depend on Cabal, but that's probably what I would do...).

@tfausak
Copy link
Collaborator Author

tfausak commented Sep 11, 2017

I said that parsing constraints would be hard because right now the dependency strings are simply passed through to the Cabal file. Parsing the constraints will be a little bit bigger of a change, but it's definitely doable.

Would later constraints be "and"ed together with earlier ones? In your example, that would end up as 4.* && >=4.3. It would be hard to loosen constraints that way. You could optionally put || before the constraint, but Cabal might not like parsing that.

sol added a commit that referenced this pull request Sep 23, 2017
sol added a commit that referenced this pull request Sep 23, 2017
sol added a commit that referenced this pull request Sep 23, 2017
sol added a commit that referenced this pull request Sep 23, 2017
@sol
Copy link
Owner

sol commented Sep 23, 2017

Would later constraints be "and"ed together with earlier ones? In your example, that would end up as 4.* && >=4.3.

Cabal solves dependencies for a package as a whole (with intersection-semantics for constraints from different sections, which is the same as &&). For that reason I think it is fine to override global constraints with section specific ones by just merging the Maps and leaving the intersecting to the dependency solver.

This will give us the exact same semantics in most cases. The only exception I see is when you override a global dependency in all sections of a package, e.g.:

dependencies:
  base: '>= 4'

library:
  dependencies:
    base: '< 5'
  ...

tests:
  spec:
    dependencies:
      base: '>= 4.3'

which is not particularly DRY in the first place and better expressed as

dependencies:
  base: 4.*

tests:
  spec:
    dependencies:
      base: '>= 4.3'

(As a side note, as I understand it, it's never useful to specify different constraints for library and executables, as Cabal will always solve them together anyway. In the same vane it does not make sense to loosen constraints for tests/benchmarks, as again Cabal will always solve them together with constraints from library/executables. So the only thing that I ever wanted to do is tighten constraints for tests/benchmarks. Please correct me if you think I'm missing something.)

It would be hard to loosen constraints that way. You could optionally put || before the constraint, but Cabal might not like parsing that.

Can you come up with a use case?

@tfausak big sorry again for the delay! I implemented constraint parsing and changed the internal representation from a list to a Map in #196. Can you migrate your changes on top of that?

@sol
Copy link
Owner

sol commented Sep 23, 2017

Once we are through with this, I'm also happy to revisit #91 and see what we can cherry-pick from npm/bundler.

@tfausak
Copy link
Collaborator Author

tfausak commented Sep 24, 2017

I'll take another look at this today. Your changes look great and lay a nice foundation for what I want to do here. I'll probably open another PR to avoid gnarly merge conflicts.

@tfausak
Copy link
Collaborator Author

tfausak commented Sep 24, 2017

Closing in favor of #198.

@tfausak tfausak closed this Sep 24, 2017
@tfausak tfausak deleted the gh-186-dependencies-hash branch September 24, 2017 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow dependencies to be a hash?
2 participants