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

ocamlPackages.ppx_deriving_rpc: init at 5.9.0 #68958

Merged
merged 1 commit into from Sep 20, 2019

Conversation

vyorkin
Copy link
Member

@vyorkin vyorkin commented Sep 17, 2019

Motivation for this change

Adds ppx_deriving_rpc -- Ppx deriver for ocaml-rpc.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

So I've just inherited the src and version attributes since this seems a right thing to do in such cases.

@vbgl
Copy link
Contributor

vbgl commented Sep 18, 2019

The build inputs you’ve listed look strange. Here is a possible patch:

--- a/pkgs/development/ocaml-modules/ppx_deriving_rpc/default.nix
+++ b/pkgs/development/ocaml-modules/ppx_deriving_rpc/default.nix
@@ -1,15 +1,15 @@
-{ stdenv, buildDunePackage, rpclib, rresult, result, ppxfind, ppx_tools, ppx_deriving, cppo }:
+{ lib, buildDunePackage, rpclib, ppxfind, ppx_deriving, cppo }:
 
 buildDunePackage rec {
   pname = "ppx_deriving_rpc";
 
   inherit (rpclib) version src;
 
-  buildInputs = [ rpclib ppx_tools ppx_deriving ppxfind cppo ];
+  buildInputs = [ ppxfind cppo ];
 
-  propagatedBuildInputs = [ result rresult ];
+  propagatedBuildInputs = [ rpclib ppx_deriving ];
 
-  meta = with stdenv.lib; {
+  meta = with lib; {
     homepage = "https://github.com/mirage/ocaml-rpc";
     description = "Ppx deriver for ocaml-rpc";
     license = licenses.isc;

@vyorkin
Copy link
Member Author

vyorkin commented Sep 18, 2019

Thank you @vbgl! Just to clarify (because I'm going to contribute more derivations of OCaml packages for sure): I've taken the build inputs from here https://github.com/mirage/ocaml-rpc/blob/2525190d7c95c4ad19f90e0d90796a59fa320847/ppx_deriving_rpc.opam#L14, is this a right thing to do? For example, in the suggested patch there is no ppx_tools, is that correct? (I've updated the PR)

@vbgl
Copy link
Contributor

vbgl commented Sep 19, 2019

@GrahamcOfBorg build ocamlPackages.ppx_deriving_rpc

@vbgl vbgl merged commit 0cf81af into NixOS:master Sep 20, 2019
@vbgl
Copy link
Contributor

vbgl commented Sep 20, 2019

Here ppx_tools is implicitly brought by ppx_deriving. Adding it explicitly to the buildInputs of this package is up to you. I don’t have a strong opinion about this: I tend to prefer minimality.

@vyorkin
Copy link
Member Author

vyorkin commented Sep 20, 2019

Thank you for the explanation! Yeah, I agree about the minimality

@vyorkin vyorkin deleted the ocaml-ppx_deriving_rpc-5.9.0 branch December 5, 2019 10:39
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.

None yet

2 participants