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] expand coq.version & co without using coq-lib #10629

Closed
gares opened this issue Jun 8, 2024 · 11 comments · Fixed by #10631
Closed

[coq] expand coq.version & co without using coq-lib #10629

gares opened this issue Jun 8, 2024 · 11 comments · Fixed by #10631
Labels

Comments

@gares
Copy link
Contributor

gares commented Jun 8, 2024

The file coq_config.ml is able to expand variables by two means:

  • coqc --print-version
  • coqc -config

The former works fine with just coq-core being built (if one also passes -boot -noinit). The use case is to compose with other projects using the coq.version variable (for example to feed ppx_optcomp) and run dune build @check.

Unfortunately the file coq_config.ml first computes all data and then checks which variable to expand. Hence, both commands are run, and the latter requires a more "complete" build of coq to work.

It would be nice if the by_name dispatching could be used upfront, and only call the command needed in order to fulfill the request.

@Alizter
Copy link
Collaborator

Alizter commented Jun 10, 2024

IIRC coqc -noinit --version had trouble when the stdlib wasn't available. Can you check that this is the case when only coq-core is installed via opam? We can't really check this in the Dune CI since we run our test-suite with the full coq package.

@ejgallego
Copy link
Collaborator

@Alizter did you intend this comment to go in Enrico's PR itself?

@gares
Copy link
Contributor Author

gares commented Jun 11, 2024

I think the test suite you have tests it. Indeed I had to fix the expected output by removing a failure and add a version number (for coq 8.16 IIRC).

@Alizter
Copy link
Collaborator

Alizter commented Jun 11, 2024

@gares IIRC that test-suite still has coq-stdlib around. From what I remember, not having coq-stdlib installed anywhere caused issues with coqc --print-version.

@gares
Copy link
Contributor Author

gares commented Jun 11, 2024

I do pass -boot -noinit now, before it did not pass these.

@Alizter
Copy link
Collaborator

Alizter commented Jun 11, 2024

Right, but back when I tested it doing coqc --print-version -boot -noinit with no coq-stdlib installed caused an error. I am wondering if this is still the case.

@ejgallego
Copy link
Collaborator

@Alizter , it is easy to check this in master:

$ git clean -fxd
$ dune build -p coq-core
$ ls _build/install/default/lib/
$ dune exec -- coqc -print-version -boot -noinit

8.20+alpha 4.14.2

For our base CI base (8.16): 8.16.1 4.14.2

So indeed, at least here, it works!

@Alizter
Copy link
Collaborator

Alizter commented Jun 11, 2024

@ejgallego Do you have an opam switch with Coq around?

@ejgallego
Copy link
Collaborator

I do have quite a few.

@Alizter
Copy link
Collaborator

Alizter commented Jun 11, 2024

@ejgallego Could you try testing this in an isolated environment. The coqlib detection code in coqc does some weird directory checks and if those fails leads to an error in this case I believe. If this is still OK, then I support this PR. But I am under the impression that Coq is lacking the functionality that it supposedly can support.

@ejgallego
Copy link
Collaborator

Actually the code is not so weird, it is pretty simple, and we refactored it a lot in the last releases (since 8.8 I guess) so it is not in such a bad shape, tho it could use still quite a few improvements.

The code just tries to first locate coqlib, then it checks prelude is there, as otherwise the auto require import prelude will fail.

Check under dune shouldn't be too different from opam, anyways, checked opam for 8.19 and it works fine.

Note that if Nix uses env vars, this is less tested actually.

$ opam list | grep coq
$ dune exec -- coqc -print-version -boot -noinit

Output 8.19.1 4.14.1

As Enrico pointed out, -boot should avoid triggering this problem.

Actually I just realized before pressing send, that you may be thinking of problems with coqdep or coqchk, that could be the case, but I think we are good in the latest releases too.

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

Successfully merging a pull request may close this issue.

4 participants