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

Fix compat with cmdliner 1.1+ #429

Merged
merged 3 commits into from
Feb 24, 2022

Conversation

NathanReb
Copy link
Contributor

No description provided.

Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Copy link
Member

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. We should however also bump the minimum OCaml version to 4.08 since that's what Cmdliner requires.

Co-authored-by: Marek Kubica <marek@xivilization.net>
@NathanReb
Copy link
Contributor Author

We should however also bump the minimum OCaml version to 4.08 since that's what Cmdliner requires.

You 're right that we should probably do it but I don't think that's the reason why. If we do, we should do it in a separate PR at the very least and make it an occasion to use new syntax such as letop!

@Leonidas-from-XIV
Copy link
Member

I get your point but at the moment the CI just doesn't run on 4.06 and 4.07 so if we merge it we basically have no guarantees that the code we write actually works on anything before 4.08 (say, if a future cmdliner 1.1.1 decides to support 4.06 again), which is not a good place to be in.

Nevertheless I think it would be nice to convert to use the let operators and e.g. remove any stdlib polyfills we might have, but I suggest that as a separate PR.

@NathanReb NathanReb mentioned this pull request Feb 14, 2022
@gpetiot
Copy link
Contributor

gpetiot commented Feb 22, 2022

Could we cut a minor release with this compat? This is needed to release projects that already upgraded cmdliner (like ocamlformat)

The issue is that given cmdliner >= 1.1.0 forces 4.08 we have no
possibility to test that the code we write going forward will be
compatible with 4.06 or 4.07 since it can't be installed so it makes
more sense to admit that we have to abandon support for < 4.08.
Copy link
Member

@pitag-ha pitag-ha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks all good to me!

It's a bit of a pity to drop support for some compilers because of this, but to be fair there are several workarounds that still allow folks to use dune-release even if they really want to use an old compiler: they could have their project on a local switch and dune-release on the default global switch with compatible compiler version, or they could compile dune-release on a different switch and move the binary, or similar.

@Leonidas-from-XIV Leonidas-from-XIV merged commit 0c3a6ff into tarides:main Feb 24, 2022
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Feb 24, 2022
CHANGES:

### Fixed

- Fix compatibility with Cmdliner 1.1.0. This also unfortunately means that the
  minimum OCaml version is 4.08 now. (tarides/dune-release#429, @NathanReb)
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.

4 participants