Skip to content

Fix some issues in refman #21007

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

Merged
merged 3 commits into from
Aug 15, 2025
Merged

Fix some issues in refman #21007

merged 3 commits into from
Aug 15, 2025

Conversation

ia0
Copy link
Contributor

@ia0 ia0 commented Aug 13, 2025

No description provided.

@ia0 ia0 requested a review from a team as a code owner August 13, 2025 08:24
@coqbot-app coqbot-app bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Aug 13, 2025
Copy link
Member

@jfehrle jfehrle left a comment

Choose a reason for hiding this comment

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

Your changes look fine. I can merge as is if you wish. If you're willing to do a little more, how about adding a working example?

On line 70, lvar is not defined. It's probably the list passed in variables.

FWIW Looking through the code pretty thoroughly, I can't find where parameters such as radicalmax are parsed. Also I think we could do better then using 1%Z etc for strategy.

@ia0
Copy link
Contributor Author

ia0 commented Aug 14, 2025

Your changes look fine. I can merge as is if you wish. If you're willing to do a little more, how about adding a working example?

I tried to add the first example from the test-suite/success/Nsatz.v file (by the way the link was broken since stdlib moved to its own repo), but I failed. Here is what I tried:

   .. example::

      .. rocqtop:: all extra-stdlib

         From Stdlib Require Import NsatzTactic.
         Goal forall (x y z : Z),
             x + y + z = 0 -> x*y + x*z + y*z = 0 -> x*y*z = 0 ->
             x*x*x = 0.
         nsatz.

And I get this error:

Error: Cannot find a physical path bound to logical path NsatzTactic with prefix Stdlib.

I thought the extra-stdlib would take care of this, but maybe it doesn't provide access to the whole stdlib. So maybe we can't really do better than link to the stdlib test-suite as currently done.

On line 70, lvar is not defined. It's probably the list passed in variables.

I also don't know. I didn't read too much into that paragraph. I'm not planning to use nsatz, I'm just reading through to know what exists and what can be done with it.

@SkySkimmer
Copy link
Contributor

"extra" means optional, so it will ignore errors unless env variable ROCQRST_EXTRA is used. The idea is that when stdlib is installed you use ROCQRST_EXTRA=stdlib to catch errors in the extra-stdlib sections, and when it's not you can still compile the refman by tolerating errors in the extra-stdlib sections.
CI builds the refman with ROCQRST_EXTRA=all (and stdlib etc installed) in the ci-refman job.

@SkySkimmer
Copy link
Contributor

- ``extra-foo``: if environment variable 'ROCQRST_EXTRA' is set to `all`

@ia0
Copy link
Contributor Author

ia0 commented Aug 14, 2025

CI builds the refman with ROCQRST_EXTRA=all (and stdlib etc installed) in the ci-refman job.

Should I use the CI scripts to test locally? (most probably using Makefile.ci)

Because if I use ROCQRST_EXTRA=all make refman-html after make world I still get the failure because Stdlib is in a different repository without any submodule logic. I tried to find documentation explaining how to locally compile stdlib from the rocq repository but couldn't.

@SkySkimmer
Copy link
Contributor

You can try doing make ci-stdlib between world and refman.
Not sure if it works as the ci-refman job has some sed hacks.

@SkySkimmer
Copy link
Contributor

If it gets too annoying just ask us to run full CI

@ia0
Copy link
Contributor Author

ia0 commented Aug 14, 2025

You can try doing make ci-stdlib between world and refman. Not sure if it works as the ci-refman job has some sed hacks.

make ci-stdlib works, but it builds under _build_ci which might not be used by make refman-html because NsatzTactic is still not found. I tried make ci-refman but this requires more installed dependencies. If there is no simple way to build the refman locally with stdlib that's fine. I shouldn't need to do it too often.

@SkySkimmer
Copy link
Contributor

make ci-stdlib also installs (in _build/install/...). However dune may be deleting the installed files in the next build command.

@ia0
Copy link
Contributor Author

ia0 commented Aug 14, 2025

make ci-stdlib also installs (in _build/install/...). However dune may be deleting the installed files in the next build command.

Indeed:

% make ci-stdlib
[...]
% find _build -name NsatzTactic.vo
_build/install/default/lib/coq/user-contrib/Stdlib/nsatz/NsatzTactic.vo
% ROCQRST_EXTRA=all make refman-html
[...]
% find _build -name NsatzTactic.vo
%

@SkySkimmer
Copy link
Contributor

@coqbot run full ci

@coqbot-app coqbot-app bot removed the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Aug 14, 2025
@ia0
Copy link
Contributor Author

ia0 commented Aug 14, 2025

Weird, the tactic fails:

Toplevel input, characters 0-5:
> nsatz.
> ^^^^^
Error: Tactic failure: Proof search failed.

But rocq c foo.v works with Rocq version 9.0.0 and foo.v being:

From Stdlib Require Import NsatzTactic.
Goal forall (x y z : Z),
    x + y + z = 0 -> x*y + x*z + y*z = 0 -> x*y*z = 0 ->
    x*x*x = 0.
nsatz.
Abort.

@SkySkimmer
Copy link
Contributor

try From Stdlib Require Import Nsatz instead

@SkySkimmer
Copy link
Contributor

(probably changed in rocq-prover/stdlib#155)

@Zimmi48
Copy link
Member

Zimmi48 commented Aug 14, 2025

Why is the CI job green if the tactic failed? Is this a bug of the extra parameter?

@coqbot-app coqbot-app bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Aug 14, 2025
@ia0
Copy link
Contributor Author

ia0 commented Aug 14, 2025

Why is the CI job green if the tactic failed? Is this a bug of the extra parameter?

I've noticed this locally. Without extra-stdlib, make refman-html completes without error. There's only the error message in the rendered HTML. So I guess it's the same reason why even with extra-stdlib errors are not reported to the build command line.

@jfehrle
Copy link
Member

jfehrle commented Aug 14, 2025

Why is the CI job green if the tactic failed? Is this a bug of the extra parameter?

It sounds like errors in .. rocqtop blocks don't return failure status to the refman process (and cause a build failure). We could add that, but if there are blocks that are supposed to end with a failure status, we would may need an "ignore_error" flag on the block so the refman build won't fail in those exceptional cases.

When I try to build locally, the refman job is not finding stdlib :-(

image

But if I run rocqide locally (dune exec -- rocqide) , it does find stdlib:

image

(Tested on master from maybe a month ago.)

@SkySkimmer
Copy link
Contributor

if there are blocks that are supposed to end with a failure status, we would may need an "ignore_error" flag on the block

we already have that, it's called "fail"

@jfehrle
Copy link
Member

jfehrle commented Aug 14, 2025

@ia0 I'm just reading through to know what exists and what can be done with it.

You probably already know that there are many external libraries that are not documented in the refman. (I've never looked at them; I'm not a typical user of Rocq.)

@ia0
Copy link
Contributor Author

ia0 commented Aug 15, 2025

I suggest to delete the last 2 commits (essentially not adding any example and only fixing immediate issues). The link to the stdlib test-suite seems enough and the rocqtop-to-refman failure propagation issue is orthogonal (but should probably be addressed).

What do you think?

@Zimmi48
Copy link
Member

Zimmi48 commented Aug 15, 2025

the rocqtop-to-refman failure propagation issue is orthogonal (but should probably be addressed).

I opened an issue about this: #21010.

@jfehrle jfehrle self-assigned this Aug 15, 2025
@jfehrle jfehrle added kind: documentation Additions or improvement to documentation. and removed needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. labels Aug 15, 2025
@jfehrle jfehrle added this to the 9.2+rc1 milestone Aug 15, 2025
@jfehrle
Copy link
Member

jfehrle commented Aug 15, 2025

Let's declare victory.

@coqbot: merge now

Thanks for the PR!

@coqbot-app coqbot-app bot merged commit ce45f4e into rocq-prover:master Aug 15, 2025
6 checks passed
@ia0 ia0 deleted the fix branch August 16, 2025 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: documentation Additions or improvement to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants