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

Enable (implicit_transitive_deps false) only if OCaml version supports -H? #11212

Open
mbarbin opened this issue Dec 16, 2024 · 3 comments
Open

Comments

@mbarbin
Copy link
Contributor

mbarbin commented Dec 16, 2024

Desired Behavior

I recently started using the new (implicit_transitive_deps false) with dune 3.17 and OCaml 5 (is -H from 5.1 or 5.2?), and I like it a lot. Thanks!

One issue I did not foresee, and discovered in CI results is that, for certain repos, the CI matrix checks the build with older ocaml versions, such as 4.14, and because it uses the same dune config files there, things do not work well (this effectively reverts to a world where -H is not well supported).

Dune files have the ability to restrict stanza based on the OCaml version, via the enabled_if construct. I wonder if something similar could be made available in dune-project files.

Example

Something like this, which I would put in my dune-project file maybe?

(implicit_transitive_deps (< %{ocaml_version} 5.2))

Currently doesn't work:

File "dune-project", line 21, characters 1-26:
21 |  (< %{ocaml_version} 5.2))
      ^^^^^^^^^^^^^^^^^^^^^^^^^
Error: Atom expected

Workaround

Maybe I can add a line in the CI script to edit that construct and replace the false by true for older ocaml version. I haven't tried this yet.

@mbarbin
Copy link
Contributor Author

mbarbin commented Dec 16, 2024

For context I'm linking an actual repo with my attempt at the workaround approach mbarbin/loc#8

@nojb
Copy link
Collaborator

nojb commented Dec 16, 2024

Now that OCaml supports -H (since 5.2), there is no longer a reason to support both modes in Dune (other than backwards compatibility), and (implicit_transitive_deps false) should become the default (and only) setting of Dune. Is there a reason to support (implicit_transitive_deps true) with OCaml >= 5.2?

If we do this, then (implicit_transitive_deps ...) could be understood to modify the behaviour only for versions of OCaml < 5.2, which do not support -H.

Just an idea.

cc @rgrinberg

@mbarbin
Copy link
Contributor Author

mbarbin commented Dec 16, 2024

Thanks for commenting @nojb

Is there a reason to support (implicit_transitive_deps true) with OCaml >= 5.2?

I don't know if some people find it convenient not to have to list transitive dependencies. Imo the new behavior seems desirable and I am happy to forget about the old one. There is though a breaking change to cross when you need to add all the deps that were permitted to be missing, the first time this is enabled. I don't know if this fits with dune policy about breaking changes in general. Perhaps this default can be changed when reaching dune version 4.x?

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

No branches or pull requests

2 participants