tweag / opam-nix

Turn opam-based OCaml projects into Nix derivations
MIT License
111 stars 33 forks source link

ppx_expect isn't compiled correctly #22

Closed rgrinberg closed 2 years ago

rgrinberg commented 2 years ago

Describe the bug

ppx_expect isn't being compiled correctly.

To Reproduce

To reproduce, we must clone the dune repository first.

$ git clone https://github.com/ocaml/dune.git
$ cd dune
$ nix develop
$ make dev
$ ./dune.exe runtest test/expect-tests/
File "_none_", line 1:
Error: Files /nix/store/ff70rp932zf00qfw6bsdld6v53z5pl68-ppx_expect-v0.15.0/lib/ocaml/4.14.0/site-lib/ppx_expect/matcher/expect_test_matcher.cmxa
       and /nix/store/sl3k9vw69knlgnbjgsqlbpykihhkf2ld-re-1.10.4/lib/ocaml/4.14.0/site-lib/re/re.cmxa
       make inconsistent assumptions over interface Re__

Expected behavior

The tests should compile and link run.

Environment

balsoft commented 2 years ago

What Nix code are you using? Can you provide the flake.nix file?

rgrinberg commented 2 years ago

It's in the dune repository. I've reproduced it here for you:

{
  inputs = {
    nixpkgs.follows = "opam-nix/nixpkgs";
    opam-nix.url = "github:tweag/opam-nix";
    flake-utils.url = "github:numtide/flake-utils";
    ocamllsp.url = "git+https://www.github.com/ocaml/ocaml-lsp?submodules=1";
    ocamllsp.inputs.opam-nix.follows = "opam-nix";
    ocamllsp.inputs.nixpkgs.follows = "nixpkgs";
  };
  outputs = { self, flake-utils, opam-nix, nixpkgs, ocamllsp }@inputs:
    let package = "dune";
    in flake-utils.lib.eachDefaultSystem (system:
      let
        devPackages = {
          menhir = "*";
          lwt = "*";
          csexp = "*";
          core_bench = "*";
          bisect_ppx = "*";
          js_of_ocaml = "*";
          js_of_ocaml-compiler = "*";
          mdx = "*";
          merlin = "*";
          odoc = "*";
          ppx_expect = "*";
          ppxlib = "*";
          ctypes = "*";
          utop = "*";
          cinaps = "*";
          ocamlfind = "1.9.2";
        };
      in
      {
        packages =
          let
            scope = opam-nix.lib.${system}.buildOpamProject' { } ./.
              (devPackages // { ocaml-base-compiler = "4.14.0"; });
          in
          scope // { default = self.packages.${system}.${package}; };

        devShell =
          let
            pkgs = nixpkgs.legacyPackages.${system};
          in
          pkgs.mkShell {
            buildInputs = (with pkgs;
              [
                # dev tools
                ocamlformat_0_21_0
                opam
                coq_8_16
                nodejs-slim
                pkg-config
              ]) ++ [ ocamllsp.outputs.defaultPackage.${system} ] ++ (builtins.map (s: builtins.getAttr s self.packages.${system})
              (builtins.attrNames devPackages));
            inputsFrom = [ self.packages.${system}.default ];
          };
      });
}
balsoft commented 2 years ago

Oh wow, didn't realise dune had a flake.nix. Will take a look.

balsoft commented 2 years ago

I think this is because of a mismatch between the ocaml libraries in ocamllsp and dune. The proper fix is to set doNixSupport = false in the ocamllsp derivation, the easy fix is to just get ocamllsp from opam-repository instead of the flake (add it to devPackages and remove the ocamllsp.outputs.defaultPackage.${system} from the buildInputs).

As a couple more notes, the opam-nix Scope is not suited for packages output of a flake, since technically it must be an attribute set of derivations, and Scope contains some non-derivation things (e.g. overrideScope'). legacyPackages is more suited for this. Also, it is probably a good idea to take all the ocaml deps from opam-nix, including ocamlformat, opam, and coq.

balsoft commented 2 years ago

This is the flake.nix I would suggest:

{
  inputs = {
    nixpkgs.follows = "opam-nix/nixpkgs";
    opam-nix.url = "github:tweag/opam-nix";
    flake-utils.url = "github:numtide/flake-utils";
  };
  outputs = { self, flake-utils, opam-nix, nixpkgs }@inputs:
    let package = "dune";
    in flake-utils.lib.eachDefaultSystem (system:
      let
        devPackages = {
          menhir = "*";
          lwt = "*";
          csexp = "*";
          core_bench = "*";
          bisect_ppx = "*";
          js_of_ocaml = "*";
          js_of_ocaml-compiler = "*";
          mdx = "*";
          merlin = "*";
          odoc = "*";
          ppx_expect = "*";
          ppxlib = "*";
          ctypes = "*";
          utop = "*";
          cinaps = "*";
          ocamlfind = "1.9.2";
          ocaml-lsp-server = "*";
          ocamlformat = "*";
          coq = "*";
        };
      in {
        legacyPackages = opam-nix.lib.${system}.buildOpamProject' { } ./.
          (devPackages // { ocaml-base-compiler = "4.14.0"; });

        packages.default = self.legacyPackages.${system}.${package};

        devShell = let pkgs = nixpkgs.legacyPackages.${system};
        in pkgs.mkShell {
          buildInputs = (with pkgs; [
            # dev tools
            nodejs-slim
            pkg-config
          ]) ++ (builtins.map
            (s: builtins.getAttr s self.legacyPackages.${system})
            (builtins.attrNames devPackages));

          inputsFrom = [ self.packages.${system}.default ];
        };
      });
}

But it would also be good to fix the one in ocamllsp to not have an incorrect share/nix-support.

balsoft commented 2 years ago

The tests pass with the above flake.nix.

rgrinberg commented 2 years ago

I think this is because of a mismatch between the ocaml libraries in ocamllsp and dune. The proper fix is to set doNixSupport = false in the ocamllsp derivation, the easy fix is to just get ocamllsp from opam-repository instead of the flake (add it to devPackages and remove the ocamllsp.outputs.defaultPackage.${system} from the buildInputs).

It would mean that I would need an overlay to use the latest ocamllsp. Which requires annoying manual fiddling with deps. It's an acceptable workaround though. Isn't ocamllsp being built in a separate scope? Why does it leak its test dependencies into dune's build environment?

Also, it is probably a good idea to take all the ocaml deps from opam-nix, including ocamlformat, opam, and coq.

My goal was to make use of nix's binary caching by pulling these binary only packages from nixpkgs. None of them really need to be built with dune. Is there a downside to do doing that?

Thanks for the help.

balsoft commented 2 years ago

Isn't ocamllsp being built in a separate scope? Why does it leak its test dependencies into dune's build environment?

It provides a share/nix-support file containing the hooks to get its ocaml dependencies available in the build environment. This is the way opam-nix makes dependencies transitive. You can tell opam-nix to not create a share/nix-support by passing doNixSupport = false as a derivation attribute. Something like this in the ocaml-lsp flake:

        legacyPackages = let
          scope =
            on.buildOpamProject { resolveArgs = { with-test = true; }; } package
            ./. (allPackages);
            overlay = final: prev: {
              git-subrepo =
                prev.git-subrepo.overrideAttrs (old: {
                  src = inputs.git-subrepo-src;
                });
              ${package} = prev.${package}.overrideAttrs (_: {
                # Do not add share/nix-support, so that dependencies from the scope don't leak into dependent derivations
                doNixSupport = false;
              });
            };
        in scope.overrideScope' overlay;
balsoft commented 2 years ago

My goal was to make use of nix's binary caching by pulling these binary only packages from nixpkgs. None of them really need to be built with dune. Is there a downside to do doing that?

As long as they are only used as executables, it should be fine. But you should be careful to not accidentally use any of the library components from them, or you'll get errors similar to the one in the original issue.

rgrinberg commented 2 years ago

Is there an equivalent of doNixSupport for the packages from nixpkgs to avoid this issue?

balsoft commented 2 years ago

I'm afraid the best way is to do postFixup = "rm share/nix-support -rf"; on them. Note that it will cause a rebuild, nullifying any point in using them in the first place.

balsoft commented 2 years ago

I don't think this is an issue with opam-nix, feel free to reopen if you feel otherwise.