Skip to content

Define a vendored Compat library to be compatible with base.v0.14.0#1396

Merged
gpetiot merged 5 commits intoocaml-ppx:masterfrom
gpetiot:vendor-utils-lib
Jun 29, 2020
Merged

Define a vendored Compat library to be compatible with base.v0.14.0#1396
gpetiot merged 5 commits intoocaml-ppx:masterfrom
gpetiot:vendor-utils-lib

Conversation

@gpetiot
Copy link
Copy Markdown
Collaborator

@gpetiot gpetiot commented Jun 25, 2020

We could also put the library extensions of Fpath and Cmdliner that are defined in lib/import/Import.ml in utils as well
edit: renamed as Compat, so it doesn't make as much sense

I removed the copyright headers since the code is vendored

@gpetiot gpetiot added the no changelog set this to bypass the CI check for changelog entries label Jun 25, 2020
@gpetiot gpetiot requested review from Julow, emillon and jberdine June 25, 2020 10:11
@jberdine
Copy link
Copy Markdown
Collaborator

I realize that this is a small patch, but I have to say that I don't think that the basic approach is a good long-term solution. In particular, I do not think that we should complicate the code (other than a clearly designated compatibility module/library). Things like needing to remember which List functions need to be prefixed with Utils. is really not great. It is also not great to end up reimplementing everything that changes across versions of dependencies. And once there are incompatibilities in type definitions, things get much worse. Instead, I think that there are two options:

  1. Admit that current versions of ocamlformat need current versions of dependencies.

  2. Define a compatibility shim library/module, Compat say, that is opened in all source files by adding -open Compat to the build flags. (Or just make Compat part of Import.) In that module we can override the items that are version-sensitive. The hope would be to use conditional compilation here, to avoid reimplementing things that change in dependencies. We should be implementing the old interfaces in terms of the new ones, not the other way around. We want a situation where from the perspective of the users of the Compat module, they see the current dependency interfaces and get the actual current implementations. In cases where older dependencies are used, non-standard and non-passthrough implementations can be used. We need to have this set up in such a way that when there are problems, we can just drop old version support by deleting some code. We do not want to be in the situation where we need to reimplement things to drop old version support. At a high level, the current version's interface and implementation should be primary, and the compatibility shims should only adapt to the older versions, not the other way around.

@gpetiot gpetiot changed the title Define a vendored utils lib to be compatible with base.v0.14.0 Define a vendored Compat library to be compatible with base.v0.14.0 Jun 25, 2020
@gpetiot gpetiot requested a review from emillon June 25, 2020 13:24
@vbmithr
Copy link
Copy Markdown

vbmithr commented Jun 26, 2020

Why not avoid depending on base/stdio?

@gpetiot
Copy link
Copy Markdown
Collaborator Author

gpetiot commented Jun 26, 2020

Because we already are deeply depending on this (kinda too late), removing it would require a huge amount of work that would better be directed towards fixing bugs and improving the integration of ocamlformat with the rest of the ocaml tools.

@gpetiot gpetiot merged commit 09d139f into ocaml-ppx:master Jun 29, 2020
@gpetiot gpetiot deleted the vendor-utils-lib branch June 29, 2020 10:13
jberdine pushed a commit to emillon/ocamlformat that referenced this pull request Jun 30, 2020
@gpetiot gpetiot mentioned this pull request Jun 30, 2020
@gpetiot gpetiot mentioned this pull request Jul 17, 2020
emillon added a commit to emillon/opam-repository that referenced this pull request Jul 22, 2020
CHANGES:

#### Changes

  + No functional changes from 0.14.2. The goal of this release is to be
    compatible with base and stdio v0.14.0.

  + Backport the following PRs:
    - ocaml-ppx/ocamlformat#1386 - Update opam metadata
    - ocaml-ppx/ocamlformat#1396 - Add compatibility with base.v0.14.0
    - ocaml-ppx/ocamlformat#1399 - Allow stdio.v0.14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no changelog set this to bypass the CI check for changelog entries

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants