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] Overlay for coq/coq#18385 #1951

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

ejgallego
Copy link
Contributor

No description provided.

@Alizter
Copy link
Collaborator

Alizter commented May 4, 2024

Are we missing the plugins field for the coq.theory stanzas?

@Alizter
Copy link
Collaborator

Alizter commented May 4, 2024

When I add the plugins field:

diff --git a/theories/dune b/theories/dune
index 7e3073666..4139942ab 100644
--- a/theories/dune
+++ b/theories/dune
@@ -6,4 +6,5 @@
 
 (coq.theory
  (name HoTT)
+ (plugins coq-core.plugins.ltac coq-core.plugins.number_string_notation)
  (package coq-hott))

I get:

Error: path outside the workspace:       
../../../../../../nix/store/yb1h6vdd03mr0mlzznsx9yfd183f0n2x-coq-8.19.1/lib/ocaml/4.14.2/site-lib/coq-core/META
from default/theories
-> required by _build/default/theories/Basics.glob
-> required by alias theories/all
-> required by alias default in dune:44

So it seems there is some bad interaction with the plugin loading and noinit.

@ejgallego
Copy link
Contributor Author

That's an interesting bug due in Coq due to the workaround for ocaml/dune#5833 that was implemented in coqdep (cc: coq/coq#18165 )

So indeed, coqdep path normalization works fine for example in Coq's CI, however in build setups where Coq lives outside the build path, the normalization returns a path that is a bit non-sensical as it is relativized up to root.

We need to think a bit more how to handle this, best would be to solve Dune's bug.

@@ -7,9 +7,9 @@
(** ** Plugins *)

(** Load the Ltac plugin. This is the tactic language we use for proofs. *)
Declare ML Module "ltac_plugin".
Declare ML Module "ltac_plugin:coq-core.plugins.ltac".
Copy link
Collaborator

@SkySkimmer SkySkimmer Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to work better (more backwards compat) with the semi-legacy loading syntax

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(after coq/coq#18385 the legacy part is ignored and prints a deprecation warning)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix! I'm happy with this now.

@Alizter Alizter merged commit b840fec into HoTT:master Nov 7, 2024
20 checks passed
@ejgallego ejgallego deleted the remove_legacy_build_mode branch November 8, 2024 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants