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

[release] rpc metapackage (5.9.0, 6.0.0, 6.1.0, 7.0.0) #15879

Merged
merged 8 commits into from
Mar 3, 2020

Conversation

psafont
Copy link
Contributor

@psafont psafont commented Feb 20, 2020

Without the metapackage being available it's difficult to develop on the package while using this repo as the first one in the active opam switch, see mirage/ocaml-rpc#150

Changes:
7.0.0

  • Add basic support for JSON-RPC notifications (@vycastor)

6.1.0

  • opam: updated bounds on a more conservative basis
  • travis: tests more compilers
  • tests: disable useless-object-inheritance on pylint checks
  • pythongen: generate python2-3 compatible bindings
  • Add ISC license
  • Incremented the upper bound for async's version.
  • Added lower bound for js_of_ocaml in related .opam file.
  • Fixed compilation issue with js_of_ocaml 3.5.0 and 3.5.1.
  • opam: remove the 'build' directive on dune dependency
  • opam: remove unnecessary flag
  • port to dune

6.0.0

  • Switch to ppxlib from ppx_deriving

5.9.0

  • Optional named params fix

@camelus
Copy link
Contributor

camelus commented Feb 20, 2020

Commit: 887d960

Hello @psafont! I believe this is your first contribution here. Please be nice, reviewers!

☀️ All lint checks passed 887d960
  • These packages passed lint tests: rpc.5.9.0, rpc.6.0.0, rpc.6.1.0, rpc.7.0.0

☀️ Installability check (+4)
  • new installable packages (4): rpc.5.9.0 rpc.6.0.0 rpc.6.1.0 rpc.7.0.0

7.0.0
- Add basic support for JSON-RPC notifications (@vycastor)

6.1.0
- opam: updated bounds on a more conservative basis
- travis: tests more compilers
- tests: disable useless-object-inheritance on pylint checks
- pythongen: generate python2-3 compatible bindings
- Add ISC license
- Incremented the upper bound for async's version.
- Added lower bound for js_of_ocaml in related .opam file.
- Fixed compilation issue with js_of_ocaml 3.5.0 and 3.5.1.
- opam: remove the 'build' directive on dune dependency
- opam: remove unnecessary flag
- port to dune

6.0.0
- Switch to ppxlib from ppx_deriving

5.9.0
- Optional named params fix
@psafont
Copy link
Contributor Author

psafont commented Feb 21, 2020

@mseri travis is failing for 5.9.0 and 6.0.0 because dependencies are not compatible with the ocaml versions used for the tests, this is not surprising to me, but it'd be nice to have confirmation from you.

The tests for 6.1.0 are failing because pylint is missing, not much that can be done unless the environment is changed to include it.

@mseri
Copy link
Member

mseri commented Feb 21, 2020

Thanks for the PR. Your analysis is correct. I think we can simply disable the tests in all of them. They have been tested already after all

@kit-ty-kate
Copy link
Member

@psafont the tests require pylint. We can add a new conf-pylint package so that you can require it during tests. I'll do that now

@mseri
Copy link
Member

mseri commented Feb 22, 2020

It is not that easy I think. Some tests require the python2 version and the more recent ones the pyhon3 if I remember correctly

@kit-ty-kate kit-ty-kate mentioned this pull request Feb 22, 2020
@kit-ty-kate
Copy link
Member

then I could add two packages instead of one (python2-pylint and python3-pylint), do you think that would fix the issue?

@mseri
Copy link
Member

mseri commented Feb 22, 2020

Modulo distributions that only have one of the two, I believe it would. I am not sure it is worth the hassle, but let’s try

@kit-ty-kate
Copy link
Member

The build command was wrong and the packages didn't install anything. Could these fixes be returned upstream?

I also removed the tests as it seems too much of a hassle. This should be good to go once CI is done with it

@mseri
Copy link
Member

mseri commented Mar 2, 2020

They were already correct upstream https://github.com/mirage/ocaml-rpc/blob/master/rpc.opam I've just added the missing odoc dependency. Thanks again. I agree with the decision of turning off the tests, python dependencies are a bit of a hell still with the 2 to 3 transition

@kit-ty-kate
Copy link
Member

mh, it seems that md2mld is also needed all the time, not just for doc generation. Is this normal?

#=== ERROR while compiling rpc.6.1.0 ==========================================#
# context              2.0.6 | linux/x86_64 | ocaml-base-compiler.4.09.0 | pinned(https://github.com/mirage/ocaml-rpc/archive/v6.1.0.tar.gz)
# path                 ~/.opam/4.09/.opam-switch/build/rpc.6.1.0
# command              ~/.opam/4.09/bin/dune build -p rpc -j 72
# exit-code            1
# env-file             ~/.opam/log/rpc-23-2f89d0.env
# output-file          ~/.opam/log/rpc-23-2f89d0.out
### output ###
# File "docs/dune", line 7, characters 10-21:
# 7 |    (run %{bin:md2mld} -min-header 3 %{deps})
#               ^^^^^^^^^^^
# Error: Program md2mld not found in the tree or in PATH
#  (context: default)

@mseri
Copy link
Member

mseri commented Mar 2, 2020

That's weird... it may be changed recently, it used to be called only to generate the documentation

@mseri
Copy link
Member

mseri commented Mar 2, 2020

It is explicitly marked for documentation: https://github.com/mirage/ocaml-rpc/blob/master/docs/dune#L12

@kit-ty-kate
Copy link
Member

weird indeed. cc @rgrinberg @diml?

@rgrinberg
Copy link
Member

.mld files are installed as part of the package (see the .install file generated), so it's expected they're included here. I'm afraid you will need to add a dependency on the package that provides this binary or simply promote the generated artifact.

@rgrinberg
Copy link
Member

rgrinberg commented Mar 2, 2020

@avsm @diml this issue reminds me of our discussion at the retreat regarding the correct way to split packages so that they're as usable in as many contexts as possible. There was a remark that packages should be split along the lines laid out by Linux package managers, e.g foo to provide the binaries, foo-dev to provide the libraries. The issue here would be solved if documentation was packaged separately as well: foo-doc. Additionally, a foo-test package would help for cases when the test dependencies require additional dependencies.

I recall that in the retreat, we concluded that the user should factor their packages manually. Perhaps it's the wrong way after all as it will quickly get tedious and upstream package maintainers will have to repeat this work. Maybe dune should be a little smarter do this factoring automatically to generate multiple packages?

I realize that this is in conflict with opam's with-test and with-doc features, but these have caused quite a few problems for me and other users. They seem to be anti features like optional dependencies at this point. Anyway, this is just something to think about. I don't think we should take any action here.

@kit-ty-kate
Copy link
Member

Looks good now. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants