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

feat: let extensions generate opam constraints #10751

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Jul 19, 2024

This adds a new feature where extensions can generate opam dependencies. For
example, (using mdx x.y) will include mdx as an opam dependency.

TODO:

  • document interface
  • update menhir and link to relevant issue
  • update ctypes
  • check if other extensions need to be updated
  • changelog

@emillon emillon marked this pull request as ready for review July 22, 2024 14:59
@emillon emillon requested a review from rgrinberg July 22, 2024 14:59
@emillon
Copy link
Collaborator Author

emillon commented Jul 22, 2024

@rgrinberg, before I go further, does that approach look sensible?

@rgrinberg
Copy link
Member

The idea itself seems fine. However, I'm wondering if the implementation is flexible enough. In particular, what if the extension is used only for the test suite? How would I make sure the generated constraint has with-test.

@emillon
Copy link
Collaborator Author

emillon commented Jul 22, 2024

The approach of the current implementation is to back off if the package appears in the package's dependencies in dune-project. The idea is that we try to provide good defaults (mdx would be with-test; ctypes and menhir would not), but we trust that the user will have the last word.

An alternative would be to add more precise knobs when parsing the (using) directive (there's already a hook for this used in fmt): (using mdx (opam with-test)) to add the constraint, (using mdx (opam :dont-generate)) to force skipping the generation, etc, but I think it will be fairly heavy syntax-wise.

@emillon
Copy link
Collaborator Author

emillon commented Jul 22, 2024

(I'm open to suggestions someone has an idea for a better interface)

@rgrinberg
Copy link
Member

Okay, well we can always add such an option later if there's demand.

@Leonidas-from-XIV
Copy link
Collaborator

I think backing off if it is already detected in the dependencies is a good approach to allow the user to override it without additional syntax that the user would need to be aware of. I like it.

And yes, it would be great to generate the Menhir dependency with the right lower-bound as I sometimes see packages fail the lower-bound check on opam-repository, having dune generate the depemendency with >= 20180530 will avoid this in the future. Or maybe even >= 20180905 to get the fixes for --explain (though we don't use --strict so it might not even be relevant to Dune).

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.

3 participants