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

[Coq] Dune crashes on version 3.8 #7846

Closed
LasseBlaauwbroek opened this issue May 31, 2023 · 17 comments · Fixed by #7847 or ocaml/opam-repository#23884
Closed

[Coq] Dune crashes on version 3.8 #7846

LasseBlaauwbroek opened this issue May 31, 2023 · 17 comments · Fixed by #7847 or ocaml/opam-repository#23884
Labels
Milestone

Comments

@LasseBlaauwbroek
Copy link
Contributor

Reproduction

  1. Make sure you are on Dune 3.8.
  2. git clone git@github.com:coq-tactician/coq-tactician.git
  3. opam install . --deps-only
  4. dune build

Crashes with:

Error: exception Dune_rules__coq_config.Vars.E(_)
Raised at file "src/dune_rules/coq/coq_config.ml", line 37, characters 21-49
Called from file "src/dune_rules/coq/coq_config.ml", line 194, characters
  10-53
Called from file "otherlibs/fiber/src/core.ml", line 250, characters 36-41
Called from file "otherlibs/fiber/src/scheduler.ml", line 73, characters 8-11
-> required by ("<unnamed>", ())
-> required by ("<unnamed>", ())
-> required by ("<unnamed>", ())
-> required by ("<unnamed>", ())
-> required by ("gen-rules", In_build_dir "default/tests")
-> required by ("load-dir", In_build_dir "default/tests")
-> required by ("<unnamed>", ())
-> required by
   ("build-alias", { dir = In_build_dir "default"; name = "default" })
-> required by ("top-level", ())

I must not crash.  Uncertainty is the mind-killer. Exceptions are the
little-death that brings total obliteration.  I will fully express my cases. 
Execution will pass over me and through me.  And when it has gone past, I
will unwind the stack along its path.  Where the cases are handled there will
be nothing.  Only I will remain.

CC @ejgallego @Alizter

Specifications

  • Version of dune (output of dune --version): 3.8.0
  • Version of ocaml (output of ocamlc --version): 4.09.1
  • Operating system (distribution and version): Arch linux
@Alizter
Copy link
Collaborator

Alizter commented May 31, 2023

Do you only have coq-core installed by chance? This should go away if you install the stdlib. The issue is that coqc --config fails without the stdlib.

@LasseBlaauwbroek
Copy link
Contributor Author

No. This is running on coq 8.11.

@Alizter
Copy link
Collaborator

Alizter commented May 31, 2023

@LasseBlaauwbroek OK in that case this is new, will have a look ASAP.

@Alizter Alizter added the coq label May 31, 2023
@Alizter Alizter added this to the 3.8.1 milestone May 31, 2023
@Alizter
Copy link
Collaborator

Alizter commented May 31, 2023

OK so the issue is that Dune is looking for COQ_NATIVE_COMPILER_DEFAULT however this is only available since 8.13. I'm not exactly sure how it should be treated when it isn't present, so I will assume "no" for the time being.

@LasseBlaauwbroek
Copy link
Contributor Author

Thanks. Since 3.8.0 is already released, can we add a conflicts clause for Coq < 8.13 to the package?

@Alizter
Copy link
Collaborator

Alizter commented May 31, 2023

Sure that will be up to @emillon to do. I will definitely fix this for a 3.8.1 point release however.

@Alizter
Copy link
Collaborator

Alizter commented May 31, 2023

I've submitted #7847. If you could opam pin that PR branch and test it that would be appreciated. It worked (didn't crash) for me locally.

@emillon
Copy link
Collaborator

emillon commented Jun 1, 2023

Thanks. Since 3.8.0 is already released, can we add a conflicts clause for Coq < 8.13 to the package?

coq 8.13.0 builds fine with dune 3.8.0, so it looks like it's a more precise combination that should be rejected.

Agree to cut a point release with a fix.

@emillon
Copy link
Collaborator

emillon commented Jun 1, 2023

coq 8.13.0 builds fine with dune 3.8.0, so it looks like it's a more precise combination that should be rejected.

To clarify: if the coq package as built with 3.8.0 is useless, it can conflict, but it seems like we're restricting valid combinations.

@LasseBlaauwbroek
Copy link
Contributor Author

  • I think the problem is only for Coq 8.12 and below.
  • Even for Coq 8.12, it is probably indeed still possible to build Coq itself with Dune 3.8.0. But it is impossible to build any packages that depend on Coq.

if the coq package as built with 3.8.0 is useless, it can conflict, but it seems like we're restricting valid combinations.

I guess we are indeed restricting valid combinations. But is this really a big problem? Dune 3.7 can still be used for all current packages, and Dune 3.8.1 will presumably be released soon. It would be inconvenient to have a Dune version in the Opam manager that crashes on a bunch of packages.

The only other solution is to manually mark all current and future packages that depend on Coq <= 8.12 to be conflicting with Dune 3.8.0... Not something I'd like to be in charge of.

@emillon
Copy link
Collaborator

emillon commented Jun 1, 2023

I guess it's up to the coq and opam maintainers. You can open a PR to ocaml/opam-repository to add the conflicts clause.

@erikmd
Copy link

erikmd commented Jun 1, 2023

I guess we are indeed restricting valid combinations

At first sight, I don't think so. I mean, all Coq versions upto 8.12.x certainly will trigger the crash regarding COQ_NATIVE_COMPILER_DEFAULT.

So I vote +1 for adding a conflicts: [ "coq" {< "8.13"} ] clause in dune.3.8.0

@ejgallego
Copy link
Collaborator

Indeed the conflict sounds reasonable. As we approach (lang coq 1.0) it could be worth to add Coq 8.11 to Dune's CI.

@Alizter
Copy link
Collaborator

Alizter commented Jun 1, 2023

I was imagining setting up a custom workflow for older coq versions that can be manually triggered. That would help us with a lot of these issues stemming from the coq world.

@ejgallego
Copy link
Collaborator

Actually if we do cache the Coq installl that should not be too heavy IMVHO as a regular job.

@ejgallego
Copy link
Collaborator

ejgallego commented Jun 1, 2023

I was imagining setting up a custom workflow for older coq versions that can be manually triggered. That would help us with a lot of these issues stemming from the coq world.

I think we also need to be very mindful and define (maybe we write it down in the DuneCoq paper) our methodology for ensuring compatibility with older Coq versions. Testing is very important but also we need to be very defensive when interacting with the Coq API.

@LasseBlaauwbroek
Copy link
Contributor Author

See ocaml/opam-repository#23882

LasseBlaauwbroek added a commit to LasseBlaauwbroek/opam-repository that referenced this issue Jun 5, 2023
Due to a bug, the new release of Dune 3.8.0 is incompatible with any Coq package
that depends on Coq < 8.13. See ocaml/dune#7846 for
details.
Even though Dune is technically compatible with these Coq versions itself, this
doesn't seems very useful. An older version of Dune will do perfectly well for
these use-cases and Dune 3.8.1 will be around soon. So I propose to mark a
conflict with these versions of Coq. Otherwise, we end up with a large number of
uninstallable packages with one particular version of Dune.
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants