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.lwt_ppx: fix dependency propagation #73401

Merged
merged 1 commit into from Nov 16, 2019
Merged

Conversation

@wizeman
Copy link
Member

wizeman commented Nov 14, 2019

Motivation for this change

dune was showing this error when using lwt_ppx:

File "/nix/store/sz4cg32ph84lapgs50xv73s3a0baqq2s-ocaml4.08.1-lwt_ppx-4.2.1/lib/ocaml/4.08.1/site-lib/lwt_ppx/dune-package", line 11, characters 56-75:
11 |  (requires compiler-libs.common ocaml-migrate-parsetree ppx_tools_versioned)
                                                             ^^^^^^^^^^^^^^^^^^^
Error: Library "ppx_tools_versioned" not found.
-> required by library "lwt_ppx" in
   /nix/store/sz4cg32ph84lapgs50xv73s3a0baqq2s-ocaml4.08.1-lwt_ppx-4.2.1/lib/ocaml/4.08.1/site-lib/lwt_ppx
-> required by executable wizytests in dune:6
Hint: try: dune external-lib-deps --missing @@default
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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.
Notify maintainers

cc @vbgl

dune was showing this error when using lwt_ppx:

File "/nix/store/sz4cg32ph84lapgs50xv73s3a0baqq2s-ocaml4.08.1-lwt_ppx-4.2.1/lib/ocaml/4.08.1/site-lib/lwt_ppx/dune-package", line 11, characters 56-75:
11 |  (requires compiler-libs.common ocaml-migrate-parsetree ppx_tools_versioned)
                                                             ^^^^^^^^^^^^^^^^^^^
Error: Library "ppx_tools_versioned" not found.
-> required by library "lwt_ppx" in
   /nix/store/sz4cg32ph84lapgs50xv73s3a0baqq2s-ocaml4.08.1-lwt_ppx-4.2.1/lib/ocaml/4.08.1/site-lib/lwt_ppx
-> required by executable wizytests in dune:6
Hint: try: dune external-lib-deps --missing @@default
@wizeman wizeman force-pushed the wizeman:u/fix-lwt-ppx branch from 8157446 to a4ebc4f Nov 14, 2019
@wizeman wizeman requested a review from Zimmi48 Nov 14, 2019
Copy link
Member

Zimmi48 left a comment

That's typical. Sorry about that.

@vbgl

This comment has been minimized.

Copy link
Contributor

vbgl commented Nov 15, 2019

I found that PPX dependencies are tricky.

Consider for instance the eliom package: it propagates lwt_ppx but does not propagate ppx_tools_versioned. The ocsigen-toolkit package, that has eliom as input, needs the lwt_ppx library but does not use the ppx_tools_versioned library.

The ocsigen-toolkit examples shows that it is not necessary that lwt_ppx propagate ppx_tools_versioned (since you may use the first without the last).

However, it may be convenient and less surprising for the end user to do the propagation.

I have no strong opinion on this particular case.

@vbgl vbgl added the 6.topic: ocaml label Nov 15, 2019
@wizeman

This comment has been minimized.

Copy link
Member Author

wizeman commented Nov 15, 2019

@Zimmi48 No problem!
@vbgl I don't understand how that's possible.

Note that I was also using lwt_ppx but I wasn't using ppx_tools_versioned (as far as I know, since I don't even know what that is for), just like ocsigen-toolkit, so why did I get the error above but ocsigen-toolkit builds fine?

I see that ocsigen-toolkit doesn't use dune, is that the reason? I mean, do I get the error because dune needs ppx_tools_versioned when I add lwt_ppx to my dune file as a library dependency?

If you see the error above, you will see that the error happens because there is a requires (...) ppx_tools_versioned stanza which comes from a file called dune-package which belongs to the lwt_ppx derivation. So this must be the reason why dune fails if I don't propagate ppx_tools_versioned through the Nix dependency mechanism, as I'm not really using ppx_tools_versioned.

In other words, I'm guessing that ocsigen-toolkit builds fine with lwt_ppx but without ppx_tools_versioned simply because it doesn't use dune, but if anyone tries to add lwt_ppx as a dependency to a project that uses dune as a build tool, it will also fail with the error above once lwt_ppx is added to the dune and .nix files, assuming that ppx_tools_versioned is not being propagated or added to the .nix file, simply because dune picks up that stanza from lwt_ppx's dune-package file.

@wizeman

This comment has been minimized.

Copy link
Member Author

wizeman commented Nov 15, 2019

In the same vein, I've just added cryptokit as a dependency (after removing base and stdio) and now I get this failure:

/nix/store/q354712mnkw3ky8b5crj7ir7dyv29ylj-binutils-2.31.1/bin/ld: cannot find -lz
collect2: error: ld returned 1 exit status
File "caml_startup", line 1:
Error: Error during linking

It seems the zlib dependency is needed by cryptokit itself (as my project doesn't use zlib directly). Should I make another PR to propagate this dependency?

It's strange that other packages that use cryptokit (such as ocsigen-server) don't have this problem. Perhaps zlib is being propagated for them from some other dependency?

For example, I just noticed that ocsigen-server depends on ocamlzip, which does propagate zlib, so perhaps this is why this problem wasn't noticed before?

@vbgl vbgl merged commit d5053d1 into NixOS:master Nov 16, 2019
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@vbgl

This comment has been minimized.

Copy link
Contributor

vbgl commented Nov 16, 2019

In the “dune-package” file you mention, the (requires … ppx_tools_versioned) line is followed by (ppx_runtime_deps lwt). Similarly, in the “META” file, there are a requires(ppx_driver) = "… ppx_tools_versioned" clause and a requires(-ppx_driver) = "lwt". So this is not related to dune.

This package has different sets of dependencies depending on how you use it. I thought that these two sets could roughly correspond to the buildInputs and the propagatedBuildInputs. But it’s probably more complex than that.

Regarding zlib and cryptokit, can it be the case that it is an upstream bug? If you ask pkg-config: “how to link to zlib”? (aka “pkg-config --libs zlib”), you’ll get an answer like this: -L/nix/store/m…0-zlib-1.2.11/lib -lz. However, the build system of cryptokit has only partial information: https://github.com/xavierleroy/cryptokit/blob/8895c61c75e30b95ed9b475e4f7bb0151d8d89d8/_oasis#L83

@Zimmi48

This comment has been minimized.

Copy link
Member

Zimmi48 commented Nov 25, 2019

Dune 2.0 introduces the following:

Add support for dependencies that are re-exported. Such dependencies are marked with re_export and will be automatically provided to users of a library (ocaml/dune#2605, @rgrinberg)

It looks like this could be the solution followed by package authors in the future. We could stop propagating libraries ourselves and consider it an upstream bug when a package fails because of a missing transitive dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.