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

3.8.0 regression: Error: Library "ppx_deriving_runtime" not found. #7875

Closed
hannesm opened this issue Jun 4, 2023 · 22 comments · Fixed by #7887 or ocaml/opam-repository#23884
Closed
Assignees

Comments

@hannesm
Copy link
Member

hannesm commented Jun 4, 2023

Expected Behavior

Compilation succeeds, exit code should be 0, and dist/caldav.hvt should exist.

Actual Behavior

Compilation failure:

#=== ERROR while compiling mirage-unikernel-caldav-hvt.0.0.2 ==================#
# context              2.1.5 | freebsd/x86_64 |  | git+https://git.robur.coop/robur/unikernel-repo.git
# path                 /tmp/orb-build
# command              /bin/sh -exc cd mirage/ && mirage build
# exit-code            1
# env-file             /tmp/opam-hannes-12430/mirage-unikernel-caldav-hvt-12430-857f4b.env
# output-file          /tmp/opam-hannes-12430/mirage-unikernel-caldav-hvt-12430-857f4b.out
### output ###
# + cd mirage/
# + mirage build
# File "duniverse/ppx_deriving/src/api/dune", line 7, characters 24-44:
# 7 |  (ppx_runtime_libraries ppx_deriving_runtime)
#                             ^^^^^^^^^^^^^^^^^^^^
# Error: Library "ppx_deriving_runtime" not found.
# -> required by library "ppx_deriving.api" in
#    _build/default/duniverse/ppx_deriving/src/api
# -> required by library "caldav" in _build/solo5/duniverse/caldav/src
# -> required by executable main in dune.build:7
# -> required by _build/solo5/main.exe
# -> required by alias all (context solo5)
# -> required by alias default (context solo5)
# Generating static_caldavzap.ml
# Generating static_caldavzap.mli
# Generating static_tls.ml
# Generating static_tls.mli
# Generating static_caldavzap.ml
# Generating static_caldavzap.mli
# Generating static_tls.ml
# Generating static_tls.mli

No artifacts are produced, exit code is non-zero.

Reproduction

I'm sorry it's likely this can be minimized, but I fail to understand how to best minimize the reproduction case.
Start with an empty switch (OCaml 4.14.1 here), and follow the steps:

$ opam install mirage ocaml-solo5
$ git clone https://github.com/roburio/caldav.git
$ cd caldav/mirage
$ mirage configure -t hvt
$ make lock pull build

Now the above error appears. Doing the very same with a dune 3.7.1 succeeds. I'm sure you've a good idea what changed internally that suddenly a library is no longer found (in both cases, a grep -r ppx_deriving_runtime duniverse/ returns the exact same results).

Specifications

  • Version of dune (output of dune --version): 3.8.0
  • Version of ocaml (output of ocamlc --version) 4.14.1
  • Operating system (distribution and version): FreeBSD 13.1/amd64
@ejgallego
Copy link
Collaborator

@hannesm there were some change in how ppx_runtime_libraries are found when cross-compiling, I think it is documented in the CHANGES.md file, and very likely to be a good lead for this problem.

@hannesm
Copy link
Member Author

hannesm commented Jun 4, 2023

Thanks for your quick reply. Indeed, there's Resolve ppx_runtime_libraries in the target context when cross compiling (#7450, fixes #2794, @anmonteiro) -- but I've no clue, even from looking into the PR, what is needed to cope with this "fix". Any hints?

@ejgallego
Copy link
Collaborator

ejgallego commented Jun 4, 2023

I think that ppx_deriving_runtime needs to be installed in your target context now, instead of on your host context as before.

That seems like the sensible behavior to me, but I'm not expert tho.

Do you have a pointer to the place the target / host contexts are setup in the build process?

@samoht
Copy link
Member

samoht commented Jun 5, 2023

think that ppx_deriving_runtime needs to be installed in your target context now, instead of on your host context as before.

That's what the mirage tool is doing - it install ppx_derivering in a duniverse directory. I'm not sure what is happening there.

@dinosaure
Copy link

dinosaure commented Jun 5, 2023

I think that ppx_deriving_runtime needs to be installed in your target context now, instead of on your host context as before.

Do you know the reason for this choice? It seems to me, rather naively, that elements generating OCaml code (and therefore running on the host) have more interest in being compiled and available on the host system.

This is the main reason why we haven't notified (or been notified of) any changes for this latest release, considering the initial behaviour to be valid (and ultimately requiring no change).

EDIT: for the record, @TheLortex raised this issue initialy for our context: #4155
EDIT2: it seems that definition about host and target is a bit different that what I have in my mind according to this comment (and probably invalidate my previous comment). But yeah, as @samoht said, ppx_deriving (the PPX and the runtime) is actually vendored according to the mirage workflow.

@rgrinberg
Copy link
Member

@anmonteiro could you look at this?

@ejgallego
Copy link
Collaborator

@dinosaure precisely ppx_runtime_libs are libraries to be used by the code generated by the ppx, not the ppx itself.

@samoht
Copy link
Member

samoht commented Jun 5, 2023

I'm building a small repro just to make sure we all understand host and target the same way :-)

@hannesm
Copy link
Member Author

hannesm commented Jun 5, 2023

Do you have a pointer to the place the target / host contexts are setup in the build process?

So, from what I can tell, we have:

dune:

;; Generated by mirage.v4.3.6

(include dune.build)

dune.build:

;; Generated by mirage.v4.3.6

(copy_files ./mirage/*)

(executable
 (enabled_if (= %{context_name} "solo5"))
 (name main)
 (modes (native exe))
 (libraries arp.mirage caldav cohttp-mirage conduit-mirage dns-client-mirage
   ethernet fmt git-kv git-mirage.http git-mirage.ssh git-mirage.tcp
   happy-eyeballs-mirage icalendar lwt mimic mimic-happy-eyeballs
   mirage-bootvar-solo5 mirage-clock-solo5 mirage-crypto-rng-mirage
   mirage-kv-mem mirage-logs mirage-net-solo5 mirage-random mirage-runtime
   mirage-solo5 mirage-time tcpip.icmpv4 tcpip.ipv4 tcpip.ipv6
   tcpip.stack-direct tcpip.tcp tcpip.udp tls-mirage uri)
 (link_flags :standard -w -70 -color always -cclib "-z solo5-abi=hvt")
 (modules (:standard \ config manifest))
 (foreign_stubs (language c) (names manifest))
)

(rule
 (targets manifest.c)
 (deps manifest.json)
 (action
  (run solo5-elftool gen-manifest manifest.json manifest.c)))

(rule
 (target caldav.hvt)
 (enabled_if (= %{context_name} "solo5"))
 (deps main.exe)
 (action
  (copy main.exe %{target})))

(rule
  (targets static_tls.ml static_tls.mli)
  (deps (source_tree tls))
  (action
    (run ocaml-crunch -o static_tls.ml tls)))

(rule
  (targets static_caldavzap.ml static_caldavzap.mli)
  (deps (source_tree caldavzap))
  (action
    (run ocaml-crunch -o static_caldavzap.ml caldavzap)))

dune.config:

(data_only_dirs duniverse)

;; Generated by mirage.v4.3.6


(executable
 (name config)
 (modules config)
 (libraries mirage))

dune-workspace:

(lang dune 2.0)

(context (default))

(profile release)

(context (default
  (name solo5)
  (host default)
  (toolchain solo5)
  (merlin)
  (disable_dynamically_linked_foreign_archives true)
  ))

;; Generated by mirage.v4.3.6

and, as expressed earlier, opam-monorepo was used to unpack the ppx_deriving tarball into duniverse (including the src/runtime/dune with (name ppx_deriving_runtime))

@samoht
Copy link
Member

samoht commented Jun 5, 2023

Here is a smaller repro: https://gist.github.com/samoht/53babb7821a5d492196407a1777a91b9. You just need to setup a x-compilation context and use a package that requires ppx_deriving to trigger the issue.

I'm wondering if there is something to fix in icalendar instead. Looking at this now. That seems like a bug in 3.8.0 to me.

@samoht
Copy link
Member

samoht commented Jun 5, 2023

I confirm that a simpler repro just involves setting up a x-compilation setup + depending on the ppx_deriving library:

; dune
(library
  (name        repro)
  (public_name repro)
  (preprocess (pps ppx_deriving.std)))

and

; dune-workspace
(lang dune 2.0)

(context (default))

(context (default
  (name x)
  (host default)))

This gives:

File "duniverse/ppx_deriving/src/api/dune", line 7, characters 24-44:
7 |  (ppx_runtime_libraries ppx_deriving_runtime)
                            ^^^^^^^^^^^^^^^^^^^^
Error: Library "ppx_deriving_runtime" not found.
-> required by library "ppx_deriving.api" in
   _build/default/duniverse/ppx_deriving/src/api
-> required by library "repro" in _build/x
-> required by _build/x/META.repro
-> required by alias all (context x)
-> required by alias default (context x)

@Alizter
Copy link
Collaborator

Alizter commented Jun 5, 2023

So the bug here is that ppx_deriving_runtime should come from x not the default context right?

@hannesm
Copy link
Member Author

hannesm commented Jun 5, 2023

Thanks @samoht for working on minimizing the reproduction case.

@emillon emillon self-assigned this Jun 5, 2023
@emillon
Copy link
Collaborator

emillon commented Jun 5, 2023

I'll have a go at this.

@emillon
Copy link
Collaborator

emillon commented Jun 5, 2023

I bisected that down to #7415; the plan is to revert it in a point release together with #7450. I'll send you a branch to test on mirage to confirm it solves the issue.

@emillon
Copy link
Collaborator

emillon commented Jun 5, 2023

Can you try #7887 with mirage and let me know if this fixes the issue?

@samoht
Copy link
Member

samoht commented Jun 5, 2023

I managed to compile @hannesm repro with your branch, so that seems to fix it. Thanks!

@emillon
Copy link
Collaborator

emillon commented Jun 5, 2023

Thanks, I'm merging and backporting this.

emillon added a commit to emillon/opam-repository that referenced this issue Jun 5, 2023
CHANGES:

- Fix a crash when using a version of Coq < 8.13 due to the native compiler
  config variable being missing. We now explicitly default to `(mode vo)` for
  these older versions of Coq. (ocaml/dune#7847, fixes ocaml/dune#7846, @Alizter)

- Duplicate installed Coq theories are now allowed with the first appearing in
  COQPATH being preferred. This is inline with Coq's loadpath semantics. This
  fixes an issue with install layouts based on COQPATH such as those found in
  nixpkgs. (ocaml/dune#7790, @Alizter)

- Revert ocaml/dune#7415 and ocaml/dune#7450 (Resolve `ppx_runtime_libraries` in the target context when
  cross compiling) (ocaml/dune#7887, fixes ocaml/dune#7875, @emillon)
@hannesm
Copy link
Member Author

hannesm commented Jun 5, 2023

Thanks for your fast work on this, and cutting a release. I've a question, though, which is, wouldn't the reproduction case @samoht posted above be a nice thing to add to the test suite (to document the expected behaviour, and ensure that no future PR will break that)?

@rgrinberg
Copy link
Member

Already being done: #7896

@anmonteiro
Copy link
Collaborator

wouldn't the reproduction case @samoht posted above be a nice thing to add to the test suite (to document the expected behaviour, and ensure that no future PR will break that)?

Indeed, and I'd even go further: we want to fix it :)

@hannesm
Copy link
Member Author

hannesm commented Jun 5, 2023

Thanks for your quick replies @rgrinberg @anmonteiro. I only missed the tagging of this issue in the PR you prepared.

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