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 #183 (pre-ppxlib only): CI testing for our reverse dependencies (again!) #227

Merged

Conversation

thierry-martinez
Copy link
Collaborator

This is a new try for testing rev-deps, because Travis stops to work on #226 .

…dencies

This should implement @gasche suggestion:
ocaml-ppx#183

We should do that for `master` as well (I expect there will be
more failures there!) but I begin here.
@gasche
Copy link
Contributor

gasche commented May 24, 2020

I don't see a Travis build here either. Could this be related to the multi-line string syntax? (The configuration file might not parse, which would prevent the build from running?)

@gasche
Copy link
Contributor

gasche commented May 24, 2020

Ah, indeed: https://travis-ci.org/github/ocaml-ppx/ppx_deriving/requests seems to suggest a parse error. (I didn't know of this page before.)

@thierry-martinez
Copy link
Collaborator Author

@gasche Thank you for having found this! I will try to fix that...

@thierry-martinez
Copy link
Collaborator Author

Fixed! The problem was the comment about 4.02 in the middle of the node.

@thierry-martinez
Copy link
Collaborator Author

Thanks!

@gasche
Copy link
Contributor

gasche commented May 24, 2020

It looks like this is working; only the 4.02 build completed, but it has these interesting bits in the log:

Skipping uninstallable REVDEP ppx_deriving_morphism
[...]
1/2 REVDEPS installable
[...]
<><> Gathering sources ><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
[ppx_deriving.4.4.1] synchronised from file:///home/travis/build/ocaml-ppx/ppx_deriving
[visitors.20200210] downloaded from https://gitlab.inria.fr/fpottier/visitors/repository/20200210/archive.tar.gz

<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
∗ installed ppx_deriving.4.4.1
∗ installed visitors.20200210

@gasche
Copy link
Contributor

gasche commented May 24, 2020

@thierry-martinez we might want to set TESTS=true to also run the testsuite of those packages. (But it may turn out that their tests are too fragile, slow, etc.)

@thierry-martinez
Copy link
Collaborator Author

The previous commit (476b1e0) was already working, except for 4.11 (the most interesting one!). I am still surprised by the "installability" check. For instance, ppx_deriving_morphism is said to be never installable (here for 4.05: https://travis-ci.org/github/ocaml-ppx/ppx_deriving/jobs/690663979 ) whereas I managed to install it on my machine without any difficulties (at least on 4.05).

@gasche
Copy link
Contributor

gasche commented May 24, 2020

Well sometimes we have to accept that not everything is nice :-)

Suggested by Gabriel Scherer.
I think that CI results on recent versions are often those that are
the most interesting, so it is better to compute them first.
thierry-martinez added a commit to thierry-martinez/ocaml-ci-scripts that referenced this pull request May 24, 2020
There does not seem to be a way to run reverse-dependencies tests
currently. We would like to do that for ppx_deriving CI (see
ocaml-ppx/ppx_deriving#227).

In this commit, I propose to run tests on reverse-dependencies when
`TESTS=true` (the default) and `REVDEPS` is `true` or lists some
reverse dependencies to test (no by default).
It is the default.
Here is a pull-request to allow reverse-dependencies to be tested.
ocaml/ocaml-ci-scripts#338
@thierry-martinez
Copy link
Collaborator Author

Actually, TESTS=true is the default, and I don't see how to run tests on reverse-dependencies. I just opened a pull-request for that: ocaml/ocaml-ci-scripts#338.

@thierry-martinez
Copy link
Collaborator Author

All is green now! @gasche, are you ok if I merge this, even if revdeps testing is not available yet?

@gasche
Copy link
Contributor

gasche commented May 25, 2020

Yes, please feel free to merge. Thanks!

@thierry-martinez thierry-martinez merged commit 0f3809a into ocaml-ppx:pre-ppxlib May 25, 2020
@thierry-martinez
Copy link
Collaborator Author

Merged! Thanks.

@gasche
Copy link
Contributor

gasche commented May 25, 2020

Why do we have a v4.5 tag in the main repository now? See the releases page. We should only push the tag at the time where we do an actual release. (Are we sure that the current pre-ppxlib branch will not change when we decide to update the main plugins for 4.11 as well?)

(This is a nitpick, but there is a lot of commit noise in the present PR, which is now in the branch history. I think it would be better to rebase to erase the CI golfing commits before merging, so that it is easier to review what is and isn't each branch through git log.)

@thierry-martinez
Copy link
Collaborator Author

Oops, there is a misunderstanding: I thought we were ready for release, now that rev-dependencies were tested (both with Travis and opam-repository CI) and change-log updated. I can stop the process if you prefer: what is left to do before the release?

@gasche
Copy link
Contributor

gasche commented May 25, 2020

Actually the misunderstanding may be on my side: it may be that we can release now but I haven't realized it.

Ideally, a good release of ppx_deriving is such that plugins are compatible with the new release, and (in the case of new-ocaml-version support) can be easily adapted to the new OCaml version as well. If we realize when we start porting plugins that we would have needed a couple helpers, or different definitions, in ppx_deriving itself, we probably want those in the new release. (Or we released already and we do a patch-level release.)

My understanding was that today:

  • we know ppx_deriving itself builds with 4.11
  • we know (thanks to the present PR) that a good set of plugins builds with ppx_deriving with older versions of the compiler

I was assuming that we still have work left to do to port the plugins on 4.11. But actually, looking back at the revdeps result from the 4.11 CI run of the present PR, it looks like this is good.

This suggests that when we merged the ppx_deriving_yojson fixes yesterday, it was the only thing left before we were release-ready. Can you confirm? I had not realized that.

@thierry-martinez
Copy link
Collaborator Author

There is still ocaml-ppx/ppx_deriving_protobuf#36 but it seems it does not involve a change in ppx_deriving itself. I think therefore that we should be ready for the release!

@gasche
Copy link
Contributor

gasche commented May 25, 2020

Precisely, here is what we know about our revdeps from the CI testing log for the pre-ppxlib branch:

Skipping uninstallable REVDEP ppx_deriving_crowbar
Skipping uninstallable REVDEP ppx_deriving_rpc
Skipping uninstallable REVDEP ppx_deriving_cmdliner
Skipping uninstallable REVDEP ppx_deriving_morphism
∗ installed visitors.20200210
∗ installed ppx_deriving_protobuf.2.7
  from git+https://github.com/thierry-martinez/ppx_deriving_protobuf.git#411
∗ installed ppx_deriving_argparse.0.0.5
∗ installed ppx_deriving_protocol.0.8.1
  Note: the logs suggest that recent versions of this package do *not* depend on ppx_deriving anymore (it seems to be based on ppxlib's type_conv instead), so we could remove it from the revdeps.
∗ installed ppx_deriving_madcast.0.1
∗ installed ppx_deriving_yojson.3.5.2
  from git+https://github.com/thierry-martinez/ppx_deriving_yojson.git#411_lightning
∗ installed ppx_deriving_hardcaml.v0.13.0

This looks good, and I agree this was/is ready for a ppx_deriving release. Thanks!

@gasche
Copy link
Contributor

gasche commented May 25, 2020

Are you also planning to take care of the opam-repository submission? I'm happy to take care of it otherwise.

@thierry-martinez
Copy link
Collaborator Author

thierry-martinez added a commit to thierry-martinez/ppx_deriving that referenced this pull request May 25, 2020
Gabriel Scherer reported that ppx_deriving_protocol does not depend
on ppx_deriving.

ocaml-ppx#227 (comment)
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.

2 participants