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] [doc] Some fixes to documentation. #6208

Merged
merged 1 commit into from
Oct 11, 2022

Conversation

ejgallego
Copy link
Collaborator

In particular the advice about directory split was bogus.

@ejgallego ejgallego added this to the 3.5.0 milestone Oct 9, 2022
@ejgallego ejgallego added the coq label Oct 9, 2022
@Alizter
Copy link
Collaborator

Alizter commented Oct 9, 2022

The directory split advice was the workaround to the META issue that will now be fixed. So it wasn't bogus at some point.

@ejgallego
Copy link
Collaborator Author

The directory split advice was the workaround to the META issue that has now been fixed. So it wasn't bogus at some point.

I think it was bogus back then too.

The META issue for me == a missing dependency ; what do you mean by "META issue"?

How does splitting into two directories help for a missing dependency to appear?

@ejgallego
Copy link
Collaborator Author

Note that #6167 still solves nothing, coqdep (and likely coqc) are still very much broken for any build setup which is not the one used in Coq's test suite.

doc/coq.rst Outdated
- If ``plugins`` is present, Dune will pass the corresponding flags to
Coq so that ``coqdep`` and ``coqc`` can find the corresponding OCaml
libraries declared in ``<ocaml_plugins>``. This allows a Coq theory
to depend on OCaml plugins. Starting with ``(lang coq 0.6)``,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we checked for private plugins before 0.6? I may be confused however. I seem to remember introducing the limitation in the workspace composition PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that was only checked if the plugins live in a different scope.

With the legacy loading method it is fine to have private libs as plugins, only now that we depend (unconditionally) on the META file this is a hard limitation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Private plugins don't make any sense currently in Coq (only for private theories). If you have a public theory then you can install .vo files, however they will rely on the plugin to be installed somewhere to work hence it doesn't make sense to support this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure I understand what you mean. Indeed it is logical that private plugins do only make sense for private theories, but that's a valid use case we could support.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right I was talking about before. Having private libs in private theories was the only use case that made sense. Private libs in public theories doesn't make any sense. Now with the new loading method however, it means that even private theories can only import public libs which is not currently restricted by Dune IIRC.

To allow for the user case you mention, we would need to yet again change how Coq loads plugins.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't need to change how Coq loads plugins, it is enough for Dune to setup a virtual findlib env where the private libraries are exposed. But that's quite some work tho.

@Alizter
Copy link
Collaborator

Alizter commented Oct 9, 2022

The directory split advice was the workaround to the META issue that has now been fixed. So it wasn't bogus at some point.

I think it was bogus back then too.

The META issue for me == a missing dependency ; what do you mean by "META issue"?

How does splitting into two directories help for a missing dependency to appear?

It caused the META file to be build in time by chance. It was only a workaround.

@ejgallego
Copy link
Collaborator Author

It caused the META file to be build in time by chance. It was only a workaround.

How did having two directories cause the META file to build?

@Alizter
Copy link
Collaborator

Alizter commented Oct 9, 2022

@ejgallego No idea, that was how we worked around the issue.

@ejgallego
Copy link
Collaborator Author

ejgallego commented Oct 9, 2022

@ejgallego No idea, that was how we worked around the issue.

You think you workarounded the issue but I bet you did not, because for Dune that split makes 0 difference, unless I'm missing something. You were likely confused by the terrible (non deterministic) coqdep code.

Moreover, now that we correctly setup the findlib env (and META), coqdep outputs garbage, so yeah, the situation was not nice.

Maybe a problem was with -I pointing out to a place with a stale META?

But if so, that problem still persists no matter what you do with the dirs. Needs to be fixed at the coq lang version level.

@ejgallego ejgallego force-pushed the coq+doc_fixes branch 4 times, most recently from d62199c to c2a9c32 Compare October 10, 2022 14:27
Copy link
Contributor

@christinerose christinerose left a comment

Choose a reason for hiding this comment

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

Looks good! I just found two commas I'd suggest removing.

In particular the advice about directory split was bogus.

Signed-off-by: Emilio Jesus Gallego Arias <[email protected]>
@ejgallego
Copy link
Collaborator Author

Thanks @christinerose !

@ejgallego ejgallego merged commit f0480e0 into ocaml:main Oct 11, 2022
@ejgallego ejgallego deleted the coq+doc_fixes branch October 11, 2022 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants