Switch to lang dune 2.0 and bump minimum OCaml to 4.03.0 #4770
Conversation
lang dune 2.x has stricter rules on the naming of files in a select stanza.
There was a problem hiding this comment.
This PR requires a rebase to resolve the conflicts.
The Cygwin CI also fails with:
/cygdrive/c/projects/opam/src_ext/dune-local/dune.exe build --profile=dev --root . --promote-install-files -- opam-installer.install opam.install
File "src/stubs/libacl/dune", line 11, characters 2-77:
11 | (c_flags (:standard
12 | (:include ../c-flags.sexp)))
Error: 'c_flags' was deleted in version 2.0 of the dune language. Use the
(foreign_stubs ...) field instead.
make: *** [Makefile:127: build-opam] Error 1
and mingw one fails with:
/cygdrive/c/projects/opam/src_ext/dune-local/dune.exe build --profile=dev --root . --promote-install-files -- opam-installer.install opam.install
File "src/manifest/dune", line 4, characters 2-25:
4 | (c_names dummy)
^^^^^^^^^^^^^^^^^^^^^^^
Error: 'c_names' was deleted in version 2.0 of the dune language. Use the
(foreign_stubs ...) field instead.
make: *** [Makefile:127: build-opam] Error 1
Apart from my reservations about (implicit_transitive_deps false) this looks all good.
dune-project
Outdated
| (name opam) | ||
|
|
||
| (implicit_transitive_deps false) |
There was a problem hiding this comment.
I'm not entirely sure this is such a good idea given we'd have to be careful not to depend on something non-abstract that we re-export the type of (e.g. Result.t when depending on the result library).
This is a good idea in theory but in practice this is a bit finicky to handle correctly.
There was a problem hiding this comment.
That's what I'm after, though - if we suddenly started using Result, then I'd prefer CI to fail immediately on 4.07 and reveal that the libraries are lacking an explicit dependency on the result library, rather than it "magically" working because it picked up the Result module via a dependency which depends on the result package (and which could choose in the future to stop depending on it, creating a very strange failure case).
As far as I can tell (from having just checked) the implicit_transitive_deps setting is not exported by the project. i.e. us adopting implicit_transitive_deps does not affect any users of the opam libraries.
We could have it set as true in the committed file and override in CI as a test, but I'm not (yet) convinced that the potential behaviour you're worried about would be a bad thing?
There was a problem hiding this comment.
As far as I can tell (from having just checked) the implicit_transitive_deps setting is not exported by the project. i.e. us adopting implicit_transitive_deps does not affect any users of the opam libraries.
Are you sure? The setting is exported in the dune-package file.
We could have it set as true in the committed file and override in CI as a test, but I'm not (yet) convinced that the potential behaviour you're worried about would be a bad thing?
That could be a sensible middle point. I remember some compiler devs horrified about (implicit_transitive_deps false) who basically said this should be a static check in dune instead, not a change to the compilation pass.
I like (implicit_transitive_deps false) on paper but i saw it bite back in opam-repo-ci a couple of times so I understand what they were saying.
That's what I'm after, though - if we suddenly started using Result, then I'd prefer CI to fail immediately on 4.07 and reveal that the libraries are lacking an explicit dependency on the result library, rather than it "magically" working because it picked up the Result module via a dependency which depends on the result package (and which could choose in the future to stop depending on it, creating a very strange failure case).
Yes but the opposite is true for users. Let's say we have an .mli file with val f : unit -> (string, string) result and for whatever reason we switch the result type to Result.t (while depending on the result library) then the users who were using this function but not the result library will now fail if they tried to use it like this for instance:
match OpamModule.f () with
| Ok () -> fmt "yay"
| Error _ -> fmt "nay"
because now the type returned by f is abstract.
There was a problem hiding this comment.
mmh, sorry I tested again and it's not a problem for the opam library users but it's still a potential issue for the libraries we are using. We can always add more dependencies but for example in the case of result it would be nice to not depend on it when the library is not using it anymore.
master_changes.md
Outdated
| @@ -76,6 +76,9 @@ Prefixes used to help generate release notes, changes, and blog posts: | |||
|
|
|||
| ## Build | |||
| * Bump src_exts and fix build compat with Dune 2.9.0 [#4752 @dra27] | |||
| * Change minimum required OCaml to 4.03.0 [#4770 @dra27] | |||
| * Change minimum required Dune to 2.0 [#4770 @dra27] | |||
| * Use (implicit_transitive_deps true) [#4770 @dra27] | |||
There was a problem hiding this comment.
| * Use (implicit_transitive_deps true) [#4770 @dra27] | |
| * Use (implicit_transitive_deps false) [#4770 @dra27] |
| @@ -200,7 +201,6 @@ jobs: | |||
| fail-fast: false | |||
| env: | |||
| SOLVER: ${{ matrix.solver }} | |||
| OPAM_REPO_SHA: 02ead5a557 | |||
There was a problem hiding this comment.
Yes, it was to have a more up to date repo for 0install, not updated the general on because we needed it only here and not rebuild caches. lgtm!
|
Cygwin and mingw-w64 hopefully fixed... if only I had a Windows box to them on 🙈 |
I've completed the changes required for 4.08 as a general minimum, with opam-core, opam-format and opam-installer at 4.03, but along with the CI changes required, the PR has become too large for practical reviewing, so here's the first part which drops 4.02 support and raises the Dune requirement to 2.0.
The new solver jobs had a hard-coded SHA (possibly while developing, @rjbou, to keep the old caches?) but it seems better to use the same ones throughout. While testing something else, I also wanted to switch opam-repository to my fork, so I factorised out ocaml/opam-repository to
$OPAM_REPO. I haven't included that in the cache keys, because the assumption is that if you switch to a fork for some reason you'll also have changed the SHA!