-
Notifications
You must be signed in to change notification settings - Fork 415
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
Create (stdlib) syntax for Coq stanza #6164
Conversation
b56b0e7
to
4fb9afa
Compare
This is def going in the good direction ! A test case is needed too. It could be the case that the logic for this is better handled using the |
I tried testing this with Tactician while composed with Coq. But I didn't get very far, even without these new changes and without any kind of
It seems that Dune is trying to build Tactician's library before the prelude is built. I do not remember this issue existing before... |
@LasseBlaauwbroek oh, likely that's because Coq upstream doesn't ship Yes, the whole setup evolution of Coq + Dune is "interesting" to say the very least. |
You mean rename |
Hopefully. The story is that Coq now generates The current state is interim only in order to be able to cleanup all the non-dune infrastructure in Coq. Also, we are blocked in Dune not allowing to include auto-generated dune files, otherwise we could have a much simpler setup with |
Another issue
|
This is a known issue, you need to use the hybrid |
Here, you informed that I shouldn't use the hybrid syntax... coq/coq#16510 (comment) |
I'm also trying to write a test, but I cannot even get the existing tests to complete. I'm running |
Unfortunately that only works if you are not composing with the plugin in scope... If the plugin is not installed, you have this problem. Yeah the current state is not the best :/ Maybe you can keep that syntax for now, but issue a There are two serious bugs for compositing with Coq >= 8.16 in current dune master, hopefully we can fix them asap. |
Yes that works. But given that there are currently quite a few issues with composition, would it be reasonable to forego integrating this PR with the If you can explain to me how to properly run the |
That should work out of the box [use the suggested opam switch in Dune's makefile]
I think it is a good idea yes that |
Thanks. I missed the
I don't really know what that means. I don't really understand the full semantics of |
I mean that the place that you will have to tweak the flags generation is the function named |
Another issue: It appears that dune is not passing the following flags to |
Unfortunately, I don't really know what this 'tweak' should be. W.r.t. the composed build, I'm now stuck on this error:
It seems that plugins are not being properly promoted to the |
Umm, that last one seems bizarre, but maybe you can try what I posted on the issue: @LasseBlaauwbroek , Ali and I tried to fix the composition bug in #6165 , it was a bit tricky, but indeed, we added the flag to the buildable as we were going along. I think you can rebase your PR on top of that one, and add the tests, the syntax extension, and the documentation, and let us know how it works. |
I'd suggest first adding a use case, use the one we added in the PR as a base if you want. Once the use case is working fine, you can see how it works on your own setup. I'd be happy to try the tactician build if you point me out to the right tree. |
Thanks @ejgallego, I'll try out the PR and see if it works. With regard to writing tests and use cases, I think that we are talking past each other a little bit: I have absolutely no idea what I'm doing...
|
Here is the tree with some changes to Tactician that I'm currently using to try to make the composable build work: https://github.com/coq-tactician/coq-tactician/tree/no-stdlib-attempt |
Yes. To merge this we need that test, a changes entry, and a documentation update.
That's optional IMHO, but easy to do if you want, start from some of the By the way, the semantics of
Actually both of these features could be modeled with smaller independent features:
|
Your tree builds fine, but you need to still use the compat syntax, diff: diff --git a/theories/Ltac1/Record.v b/theories/Ltac1/Record.v
index 0b1158c..1bc407c 100644
--- a/theories/Ltac1/Record.v
+++ b/theories/Ltac1/Record.v
@@ -2,11 +2,11 @@
Definition private_constant_placeholder (x : Type) := x.
Register private_constant_placeholder as tactician.private_constant_placeholder.
-Declare ML Module "coq-core.plugins.ltac".
-Declare ML Module "coq-core.plugins.firstorder".
-Declare ML Module "coq-core.plugins.extraction".
-Declare ML Module "coq-core.plugins.funind".
-Declare ML Module "coq-tactician.record-plugin".
+Declare ML Module "ltac_plugin:coq-core.plugins.ltac".
+Declare ML Module "firstorder_plugin:coq-core.plugins.firstorder".
+Declare ML Module "extraction_plugin:coq-core.plugins.extraction".
+Declare ML Module "funind_plugin:coq-core.plugins.funind".
+Declare ML Module "tactician_ltac1_record_plugin:coq-tactician.record-plugin".
Export Set Default Proof Mode "Tactician Ltac1".
Tactician Record Then Decompose.
diff --git a/theories/Ltac1/Tactics.v b/theories/Ltac1/Tactics.v
index 9461a3d..06e809c 100644
--- a/theories/Ltac1/Tactics.v
+++ b/theories/Ltac1/Tactics.v
@@ -1,6 +1,6 @@
From Tactician Require Export Ltac1.Record.
-Declare ML Module "coq-tactician.tactics-plugin".
+Declare ML Module "tactician_ltac1_tactics_plugin:coq-tactician.tactics-plugin".
Tactic Notation (at level 5) "search" "with" "cache" tactic3(t) := fail
"'search with cache' has been renamed to 'synth with cache'". |
Yes, that does indeed work (I feel stupid now for not thinking about that but the error messages were truly coming from everwhere). Thanks. |
I tried to rebase this on top of #6165, but it looks like you've already integrated the new syntax introduced here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo maybe a couple of typos below? I think we should squash the commits too.
Also let's add a test where boot is in scope, and we use stdlib={yes,no}
5868645
to
384a0b8
Compare
Squashed and fixed the comments you made. |
384a0b8
to
9f30645
Compare
@LasseBlaauwbroek Can you |
9f30645
to
726cc6f
Compare
Done. |
726cc6f
to
1c02abf
Compare
I believe that I already addressed that one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestions for formatting, typo fixed
test/blackbox-tests/test-cases/coq/compose-boot-nostdlib.t/run.t
Outdated
Show resolved
Hide resolved
test/blackbox-tests/test-cases/coq/compose-boot-nostdlib.t/run.t
Outdated
Show resolved
Hide resolved
test/blackbox-tests/test-cases/coq/compose-boot-nostdlib.t/run.t
Outdated
Show resolved
Hide resolved
@@ -14,5 +14,5 @@ This test is currently broken due to the workspace resolution being faulty #5899 | |||
1 | (coq.theory | |||
2 | (name foo)) | |||
Error: 'coq.theory' is available only when coq is enabled in the dune-project |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error: 'coq.theory' is available only when coq is enabled in the dune-project | |
Error: 'coq.theory' is available only when Coq is enabled in the ``dune-project`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is automatically generated output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But also technically correct since it is talking about the coq
mode which is enabled in the dune-project
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Thanks for clarifying. Perhaps both coq
and dune-project
in monospace then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it appears in a command line error message, demarking it monospace would waste space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@christinerose These are Cram (I/O) tests. The test system runs specified commands, and the output of the commands is compared against what is committed here. Hence, you cannot just change this output because then the tests would fail. If you would want to change the wording of the messages, you'd have to do it where the I/O is actually performed.
(As an additional note, I'd say that the required standard of copy-editing in tests is a bit lower than one would expect for, say, documentation.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. Please feel free to discard anything that's not useful. I was tagged to review, so I did my thing. 😄 .... Do you need me to go back and "approve" it before you can move forward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the PR has already been merged and completed. Thanks for your comments @christinerose.
@@ -14,5 +14,5 @@ This test is currently broken due to the workspace resolution being faulty #5899 | |||
1 | (coq.theory | |||
2 | (name foo)) | |||
Error: 'coq.theory' is available only when coq is enabled in the dune-project | |||
file. You must enable it using (using coq 0.5) in your dune-project file. | |||
file. You must enable it using (using coq 0.6) in your dune-project file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file. You must enable it using (using coq 0.6) in your dune-project file. | |
file. You must enable it using (using ``coq 0.6``) in your ``dune-project`` file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Autogenerated output.
@@ -16,5 +16,5 @@ that the error message is good when the coq extension is not enabled. | |||
1 | (coq.theory | |||
2 | (name foo)) | |||
Error: 'coq.theory' is available only when coq is enabled in the dune-project |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error: 'coq.theory' is available only when coq is enabled in the dune-project | |
Error: 'coq.theory' is available only when Coq is enabled in the ``dune-project`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Autogenerated output.
@@ -16,5 +16,5 @@ that the error message is good when the coq extension is not enabled. | |||
1 | (coq.theory | |||
2 | (name foo)) | |||
Error: 'coq.theory' is available only when coq is enabled in the dune-project | |||
file. You must enable it using (using coq 0.5) in your dune-project file. | |||
file. You must enable it using (using coq 0.6) in your dune-project file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file. You must enable it using (using coq 0.6) in your dune-project file. | |
file. You must enable it using (using ``coq 0.6``) in your ``dune-project`` file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Autogenerated output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Autogenerated from where? Like by odoc
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the command line error message of Dune.
3ff580c
to
2d574e4
Compare
Fixes ocaml#6163 Co-authored-by: Christine Rose <[email protected]> Signed-off-by: Lasse Blaauwbroek <[email protected]>
2d574e4
to
d45acf6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
Thanks everyone! |
…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.5.0~alpha1) CHANGES: - Sandbox running cinaps actions starting from cinaps 1.1 (ocaml/dune#6176, @rgrinberg) - Add a `runtime_deps` field in the `cinaps` stanza to specify runtime dependencies for running the cinaps preprocessing action (ocaml/dune#6175, @rgrinberg) - Shadow alias module `Foo__` when building a library `Foo` (ocaml/dune#6126, @rgrinberg) - Extend dune describe to include the root path of the workspace and the relative path to the build directory. (ocaml/dune#6136, @reubenrowe) - Allow dune describe workspace to accept directories as arguments. The provided directories restrict the worskpace description to those directories. (ocaml/dune#6107, fixes ocaml/dune#3893, @esope) - Add a terminal persistence mode that attempts to clear the terminal history. It is enabled by setting terminal persistence to `clear-on-rebuild-and-flush-history` (ocaml/dune#6065, @rgrinberg) - Disallow generating targets in sub direcories in inferred rules. The check to forbid this was accidentally done only for manually specified targets (ocaml/dune#6031, @rgrinberg) - Do not ignore rules marked `(promote (until-clean))` when `--ignore-promoted-rules` (or `-p`) is passed. (ocaml/dune#6010, fixes ocaml/dune#4401, @emillon) - Dune no longer considers .aux files as targets during Coq compilation. This means that .aux files are no longer cached. (ocaml/dune#6024, fixes ocaml/dune#6004, @Alizter) - Cinaps actions are now sandboxed by default (ocaml/dune#6062, @rgrinberg) - Allow rules producing directory targets to be not sandboxed (ocaml/dune#6056, @rgrinberg) - Introduce a `dirs` field in the `install` stanza to install entire directories (ocaml/dune#5097, fixes ocaml/dune#5059, @rgrinberg) - Menhir rules are now sandboxed by default (ocaml/dune#6076, @rgrinberg) - Allow rules producing directory targets to create symlinks (ocaml/dune#6077, fixes ocaml/dune#5945, @rgrinberg) - Inline tests are now sandboxed by default (ocaml/dune#6079, @rgrinberg) - Fix build-info version when used with flambda (ocaml/dune#6089, fixes ocaml/dune#6075, @jberdine) - Add an `(include <file>)` term to the `include_dirs` field for adding directories to the include paths sourced from a file. (ocaml/dune#6058, fixes ocaml/dune#3993, @gridbugs) - Support `(extra_objects ...)` field in `(executable ...)` and `(library ...)` stanzas (ocaml/dune#6084, fixes ocaml/dune#4129, @gridbugs) - Fix compilation of Dune under esy on Windows (ocaml/dune#6109, fixes ocaml/dune#6098, @nojb) - Improve error message when parsing several licenses in `(license)` (ocaml/dune#6114, fixes ocaml/dune#6103, @emillon) - odoc rules now about `ODOC_SYNTAX` and will rerun accordingly (ocaml/dune#6010, fixes ocaml/dune#1117, @emillon) - dune install: copy files in an atomic way (ocaml/dune#6150, @emillon) - Add `%{coq:...}` macro for accessing data about the configuration about Coq. For instance `%{coq:version}` (ocaml/dune#6049, @Alizter) - update vendored copy of cmdliner to 1.1.1. This improves the built-in documentation for command groups such as `dune ocaml`. (ocaml/dune#6038, @emillon, ocaml/dune#6169, @shonfeder) - The test suite for Coq now requires Coq >= 8.16 due to changes in the plugin loading mechanism upstream (which now uses findlib). - Starting with Coq build language 0.6, theories can be built without importing Coq's standard library by including `(stdlib no)`. (ocaml/dune#6165 ocaml/dune#6164, fixes ocaml/dune#6163, @ejgallego @Alizter @LasseBlaauwbroek) - on macOS, sign executables produced by artifact substitution (ocaml/dune#6137, fixes ocaml/dune#5650, @emillon) - Added an (aliases ...) field to the (rules ...) stanza which allows the specification of multiple aliases per rule (ocaml/dune#6194, @Alizter)
…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.5.0) CHANGES: - macOS: Handle unknown fsevents without crashing (ocaml/dune#6217, @rgrinberg) - Enable file watching on MacOS SDK < 10.13. (ocaml/dune#6218, @rgrinberg) - Sandbox running cinaps actions starting from cinaps 1.1 (ocaml/dune#6176, @rgrinberg) - Add a `runtime_deps` field in the `cinaps` stanza to specify runtime dependencies for running the cinaps preprocessing action (ocaml/dune#6175, @rgrinberg) - Shadow alias module `Foo__` when building a library `Foo` (ocaml/dune#6126, @rgrinberg) - Extend dune describe to include the root path of the workspace and the relative path to the build directory. (ocaml/dune#6136, @reubenrowe) - Allow dune describe workspace to accept directories as arguments. The provided directories restrict the worskpace description to those directories. (ocaml/dune#6107, fixes ocaml/dune#3893, @esope) - Add a terminal persistence mode that attempts to clear the terminal history. It is enabled by setting terminal persistence to `clear-on-rebuild-and-flush-history` (ocaml/dune#6065, @rgrinberg) - Disallow generating targets in sub direcories in inferred rules. The check to forbid this was accidentally done only for manually specified targets (ocaml/dune#6031, @rgrinberg) - Do not ignore rules marked `(promote (until-clean))` when `--ignore-promoted-rules` (or `-p`) is passed. (ocaml/dune#6010, fixes ocaml/dune#4401, @emillon) - Dune no longer considers .aux files as targets during Coq compilation. This means that .aux files are no longer cached. (ocaml/dune#6024, fixes ocaml/dune#6004, @Alizter) - Cinaps actions are now sandboxed by default (ocaml/dune#6062, @rgrinberg) - Allow rules producing directory targets to be not sandboxed (ocaml/dune#6056, @rgrinberg) - Introduce a `dirs` field in the `install` stanza to install entire directories (ocaml/dune#5097, fixes ocaml/dune#5059, @rgrinberg) - Menhir rules are now sandboxed by default (ocaml/dune#6076, @rgrinberg) - Allow rules producing directory targets to create symlinks (ocaml/dune#6077, fixes ocaml/dune#5945, @rgrinberg) - Inline tests are now sandboxed by default (ocaml/dune#6079, @rgrinberg) - Fix build-info version when used with flambda (ocaml/dune#6089, fixes ocaml/dune#6075, @jberdine) - Add an `(include <file>)` term to the `include_dirs` field for adding directories to the include paths sourced from a file. (ocaml/dune#6058, fixes ocaml/dune#3993, @gridbugs) - Support `(extra_objects ...)` field in `(executable ...)` and `(library ...)` stanzas (ocaml/dune#6084, fixes ocaml/dune#4129, @gridbugs) - Fix compilation of Dune under esy on Windows (ocaml/dune#6109, fixes ocaml/dune#6098, @nojb) - Improve error message when parsing several licenses in `(license)` (ocaml/dune#6114, fixes ocaml/dune#6103, @emillon) - odoc rules now about `ODOC_SYNTAX` and will rerun accordingly (ocaml/dune#6010, fixes ocaml/dune#1117, @emillon) - dune install: copy files in an atomic way (ocaml/dune#6150, @emillon) - Add `%{coq:...}` macro for accessing data about the configuration about Coq. For instance `%{coq:version}` (ocaml/dune#6049, @Alizter) - update vendored copy of cmdliner to 1.1.1. This improves the built-in documentation for command groups such as `dune ocaml`. (ocaml/dune#6038, @emillon, ocaml/dune#6169, @shonfeder) - The test suite for Coq now requires Coq >= 8.16 due to changes in the plugin loading mechanism upstream (which now uses `Findlib`). - Starting with Coq build language 0.6, theories can be built without importing Coq's standard library by including `(stdlib no)`. (ocaml/dune#6165 ocaml/dune#6164, fixes ocaml/dune#6163, @ejgallego @Alizter @LasseBlaauwbroek) - on macOS, sign executables produced by artifact substitution (ocaml/dune#6137, ocaml/dune#6231, fixes ocaml/dune#5650, fixes ocaml/dune#6226, @emillon) - Added an (aliases ...) field to the (rules ...) stanza which allows the specification of multiple aliases per rule (ocaml/dune#6194, @Alizter) - The `(coq.theory ...)` stanza will now ensure that for each declared `(plugin ...)`, the `META` file for it is built before calling `coqdep`. This enables the use of the new `Findlib`-based loading method in Coq 8.16; however as of Coq 8.16.0, Coq itself has some bugs preventing this to work yet. (ocaml/dune#6167 , workarounds ocaml/dune#5767, @ejgallego) - Allow include statement in install stanza (ocaml/dune#6139, fixes ocaml/dune#256, @gridbugs) - Handle CSI n K code in ANSI escape codes from commands. (ocaml/dune#6214, fixes ocaml/dune#5528, @emillon) - Add a new experimental feature `mode_specific_stubs` that allows the specification of different flags and sources for foreign stubs depending on the build mode (ocaml/dune#5649, @voodoos)
Fixes #6163
@ejgallego could you make some comments?