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

Remove 4.03-related ifdefs #95

Closed
wants to merge 1 commit into from

Conversation

emillon
Copy link
Contributor

@emillon emillon commented Mar 19, 2019

According to the opam file, 4.03 is not supported anymore, so this always holds.

According to the opam file, 4.03 is not supported anymore, so this
always holds.
@gasche
Copy link
Contributor

gasche commented Mar 19, 2019

The log shows that the (ocaml >= 4.04) bound was added in 1c82f34 ( #85 ). It's not clear to me that we have given proper thought on what our lower bound should be, it looks like this merely reflects what our CI tests for (which is a good default policy). Can we give a minute of thought on whether we really want to constraint 4.04?

  • Are there any mainstream distributions left alive that provide 4.02 or 4.03 to their users?
  • Do we build properly with older versions than 4.04? (Dune works with 4.02)

@emillon
Copy link
Contributor Author

emillon commented Mar 19, 2019

OK, I thought it had been decided already. I'm fine with closing this otherwise.

@XVilka
Copy link

XVilka commented May 5, 2019

@gasche there are still some distributions who use old OCaml versions: https://repology.org/project/ocaml/versions

@XVilka
Copy link

XVilka commented Jul 1, 2019

Was there any decision on this one?

@XVilka
Copy link

XVilka commented Jul 10, 2019

Seems there are no mainstream distributions left with < 4.04 version in their latest releases. Should be good to merge before the new release, I think.

@XVilka
Copy link

XVilka commented Nov 13, 2019

Ping?

@whitequark
Copy link
Collaborator

@gasche IMO given #95 (comment) dropping 4.03 support should be fine.

@rgrinberg
Copy link
Contributor

rgrinberg commented Nov 16, 2019

Are there any mainstream distributions left alive that provide 4.02 or 4.03 to their users?

I think they should be encouraged to stick to older versions of ppx_deriving_yojson.

Do we build properly with older versions than 4.04? (Dune works with 4.02)

We build properly but we don't work properly. Or at least, the minimum version of ppx_type_conv (moved to ppxlib) that we need to have compatible deriving plugins is indeed 4.04. If we allow users to run on 4.03, then they're going to get quite weird errors in conjunction with ppx_sexp_conv for example.

In any case, the constraint in opam-repository has been 4.04 and no one has complained. I think we should proceed with this PR.

@rgrinberg
Copy link
Contributor

In fact, the latest version that still supports < 4.04 is 3.1. This means that we've dropped support quite a while ago and no one has yet made an issue. That should us give us quite a bit of confidence regarding this change.

@rgrinberg
Copy link
Contributor

#111 supersedes this PR. I just got rid of cppo in one swoop. Ppxlib gives us a fixed version of the AST anyway.

@emillon
Copy link
Contributor Author

emillon commented Nov 19, 2019

the ppxlib solution seems more future proof than this PR, thanks @rgrinberg !

@rgrinberg
Copy link
Contributor

rgrinberg commented Nov 19, 2019 via email

@XVilka
Copy link

XVilka commented Dec 16, 2019

Should be this one closed then?

@XVilka
Copy link

XVilka commented May 7, 2020

Superseded by #118

@emillon
Copy link
Contributor Author

emillon commented May 12, 2020

That's great, thanks everyone! I'm closing this since it won't be necessary.

@emillon emillon closed this May 12, 2020
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.

None yet

5 participants