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

feature(coq): support for interproject composition of theories #5784

Merged
merged 1 commit into from
Jun 11, 2022

Conversation

Alizter
Copy link
Collaborator

@Alizter Alizter commented May 26, 2022

  • error messages for missing transitive deps
  • error messages for private OCaml libs used as plugins for public theories
  • allow_private_deps check
  • delay library resolution errors until build time
  • restore boot support

@ejgallego
Copy link
Collaborator

See my tree in my personal repos, it was at some point close to be done.

@Alizter Alizter force-pushed the coq-compose branch 3 times, most recently from 252ab2f to fc53f31 Compare May 26, 2022 18:19
@Alizter Alizter marked this pull request as ready for review May 26, 2022 18:19
@rgrinberg rgrinberg added the coq label May 26, 2022
@Alizter

This comment was marked as off-topic.

@rgrinberg rgrinberg added this to the 3.3.0 milestone May 27, 2022
@Alizter Alizter force-pushed the coq-compose branch 3 times, most recently from fd3dfbe to 4aafc42 Compare May 31, 2022 19:32
@rgrinberg
Copy link
Member

@Alizter better not to use find in tests. If you need particular files to exist, use shell tests for that.

@rgrinberg
Copy link
Member

@ejgallego ready for review I believe.

@ejgallego ejgallego self-assigned this Jun 1, 2022
@ejgallego
Copy link
Collaborator

A couple of things:

  • this should bump coq lang to 0.4 (0.3 should disable inter scope composition, for compat)
  • missing doc
  • missing changes entry

@ejgallego
Copy link
Collaborator

After a first quick review, this good in general, will be a game changer! Gonna do some deeper testing tho.

@Alizter Alizter force-pushed the coq-compose branch 4 times, most recently from 051af67 to 0674f83 Compare June 3, 2022 22:29
@ejgallego
Copy link
Collaborator

It seems we are close to be here, I found a small bug which you can reproduce using the https://github.com/ejgallego/coq-universe/ repos (do make universe)

when the boot library is in scope -boot has to be passed to coqdep.

@ejgallego
Copy link
Collaborator

Yeah, it seems that when compiling math-comp, the boot library is not detected properly.

@ejgallego
Copy link
Collaborator

It is possible to add a test for this, I can write it, but the idea is that the test setups a theory that has prefix Coq and includes the module Init.Prelude.v , then a regular Coq composed theory.

@Alizter
Copy link
Collaborator Author

Alizter commented Jun 4, 2022

@ejgallego Looks like -boot is being passed correctly. However I noticed that the theories inside mathcomp don't declare Coq as a theories dependency. That is going to have to be done in order to compose with a local copy of coq.

@ejgallego
Copy link
Collaborator

@ejgallego Looks like -boot is being passed correctly. However I noticed that the theories inside mathcomp don't declare Coq as a theories dependency. That is going to have to be done in order to compose with a local copy of coq.

I don't see -boot being passed correctly. I'll post a build log where the problem is evident.

Regarding the second point, I see there is already code in this PR to handle that case.

@Alizter
Copy link
Collaborator Author

Alizter commented Jun 4, 2022

@ejgallego Ah, do you mean that -boot needs to be passed everywhere?

@ejgallego
Copy link
Collaborator

If the boot library is in scope, indeed you need to disable the automatic search path and let dune handle that, so yes, boot needs to be passed. Note that the code already is trying to do that when computing the flags.

@ejgallego
Copy link
Collaborator

Lib.t contains lazy values and possibly error values that are hard to compare and reason about.

Sorry I was not precise, I mean that why we can't just use Coq_lib_name.t ? Don't we have the properties that these are unique, and thus injective?

@ejgallego
Copy link
Collaborator

We should open an issue in Coq Universe w.r.t. doc building by the way, I think all we need is a @coqdoc alias :) and a few tweaks.

By the way, not a requirement, but I'd be cool to add a test coqdoc-compose even if broken.

@rgrinberg
Copy link
Member

Sorry I was not precise, I mean that why we can't just use Coq_lib_name.t ? Don't we have the properties that these are unique, and thus injective?

For the closure it wouldn't matter (if we fix that validation issue), but I still think it's better to use the one equality function we defined as well behaved.

@ejgallego
Copy link
Collaborator

ejgallego commented Jun 10, 2022

For the closure it wouldn't matter (if we fix that validation issue), but I still think it's better to use the one equality function we defined as well behaved.

Yes, we need to use a well-defined equality function. But that's the point I don't understand, why is

Coq_lib.equal t1 t2 = t1.name = t2.name && t1.path = t2.path

better behaved than

Coq_lib.equal t1 t2 = t1.name = t2.name

?

I'm Ok with identifying Coq theories as a pair of (name,path), however we need to make this clearer in the code then, as for most purposes, the code assumes that Coq theories are identified only by name. Does it make sense?

I'd be suprising for a user of Coq_lib that equals fail when the names are the same, wouldn't it?

@rgrinberg
Copy link
Member

Well, consider this setup:

$ mkdir a b
$ touch {a,b}/dune-project
$ echo "(coq.theory (name a))" > {a,b}/dune

Today this already works and has useful applications. Here we have two theories with the same name but we surely don't want Coq_lib.equal to tell us they're equivalent.

@ejgallego
Copy link
Collaborator

Today this already works and has useful applications. Here we have two theories with the same name but we surely don't want Coq_lib.equal to tell us they're equivalent.

Indeed, the example makes sense with private libs. The thing is that if I am correct, we have no user for Coq_lib.equal so we can't really know if that's OK or not, except for the closure computation, but there, the name only is OK.

So that's what got me confused, I was thinking that the more fancy compare was due to an actual use case.

@rgrinberg
Copy link
Member

Actually, we also use it to make sure we use the same boot lib everywhere.

OK or not, except for the closure computation, but there, the name only is OK.

It's only OK if we introduce the validation I mentioned earlier. Without it, the closure will report cycles instead of giving us a proper error that we're linking two different libraries with the same name.

@Alizter Alizter changed the title feature(coq): support for inter-project composition of theories feature(coq): support for interproject composition of theories Jun 10, 2022
@Alizter Alizter force-pushed the coq-compose branch 3 times, most recently from 5ed3228 to 4d4a3fc Compare June 11, 2022 00:08
@Alizter
Copy link
Collaborator Author

Alizter commented Jun 11, 2022

Unfortunately, coqdoc doesn't take the -boot option!

The sooner we rid ourselves of (no) -boot the better.

So to fix this, we can pass -coqlib with the location of Coq_lib.boot , if that one is present.

We can do this.

We should open an issue in Coq Universe w.r.t. doc building by the way, I think all we need is a @coqdoc alias :) and a few tweaks.

The @doc and @doc-latex aliases should already work.

@Alizter Alizter force-pushed the coq-compose branch 2 times, most recently from d35c621 to 472e707 Compare June 11, 2022 01:18
@Alizter
Copy link
Collaborator Author

Alizter commented Jun 11, 2022

@rgrinberg @ejgallego I did some further refactoring in a separate commit, please review and I will squash with the rest.

We should merge this soon, or else I will keep adding patches. :-)

@rgrinberg
Copy link
Member

@Alizter okay, but we need to add a test case for #5784 (comment) first.

It demonstrates a real issue Emilio pointed out. It doesn't necessarily needs to be addressed right now, since we are not yet sure whether it should be dune or coqc giving the error.

@Alizter
Copy link
Collaborator Author

Alizter commented Jun 11, 2022

@rgrinberg Added a test.

@Alizter Alizter requested a review from christinerose June 11, 2022 02:00
Co-authored-by: Rudi Grinberg <me@rgrinberg.com>

Signed-off-by: Ali Caglayan <alizter@gmail.com>
@rgrinberg rgrinberg merged commit f2f42dd into ocaml:main Jun 11, 2022
@ejgallego
Copy link
Collaborator

The @doc and @doc-latex aliases should already work.

Oh, that's great! Let's add this Coq's Universe CI and deploy the artifact 😈

@ejgallego
Copy link
Collaborator

Congrats on this great milestone!

@Alizter Alizter deleted the coq-compose branch June 11, 2022 13:35
kit-ty-kate pushed a commit to ocaml/opam-repository that referenced this pull request Jun 17, 2022
…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.3.0)

CHANGES:

- Sandbox preprocessing, lint, and dialect rules by default. All these rules
  now require precise dependency specifications (ocaml/dune#5807, @rgrinberg)

- Allow list expansion in the `pps` specification for preprocessing (ocaml/dune#5820,
  @Firobe)

- Add warnings 67-69 to dune's default set of warnings. These are warnings of
  the form "unused X.." (ocaml/dune#5844, @rgrinbreg)

- Introduce project "composition" for coq theories. Coq theories in separate
  projects can now refer to each other when in the same workspace (ocaml/dune#5784,
  @Alitzer, @rgrinberg)

- Fix hint message for ``data_only_dirs`` that wrongly mentions the unknown
  constructor ``data_only`` (ocaml/dune#5803, @lambdaxdotx)

- Fix creating sandbox directory trees by getting rid of buggy memoization
  (@5794, @rgrinberg, @snowleopard)

- Handle directory dependencies in sandboxed rules. Previously, the parents of
  these directory dependencies weren't created. (ocaml/dune#5754, @rgrinberg)

- Set the exit code to 130 when dune is terminated with a signal (ocaml/dune#5769, fixes
  ocaml/dune#5757)

- Support new locations of unix, str, dynlink in OCaml >= 5.0 (ocaml/dune#5582, @dra27)

- The ``coq.theory`` stanza now produces rules for running ``coqdoc``. Given a
  theory named ``mytheory``, the directory targets ``mytheory.html/`` and
  ``mytheory.tex/`` or additionally the aliases `@doc` and `@doc-latex` will
  build the HTML and LaTeX documentation repsectively. (ocaml/dune#5695, fixes ocaml/dune#3760,
  @Alizter)

- Coq theories marked as `(boot)` cannot depend on other theories
  (ocaml/dune#5867, @ejgallego)

- Ignore `bigarray` in `(libraries)` with OCaml >= 5.0. (ocaml/dune#5526, fixes ocaml/dune#5494,
  @moyodiallo)

- Start with :standard when building the ctypes generated foreign stubs so that
  we include important compiler flags, such as -fPIC (ocaml/dune#5816, fixes ocaml/dune#5809).
emillon added a commit to emillon/opam-repository that referenced this pull request Jul 20, 2022
…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.4.0)

CHANGES:

- Make `dune describe` correctly handle overlapping implementations
  for virtual libraries (ocaml/dune#5971, fixes ocaml/dune#5747, @esope)

- Building the `@check` alias should make sure the libraries and executables
  don't have dependency cycles (ocaml/dune#5892, @rgrinberg)

- [ctypes] Add support for the `errno` parameter using the `errno_policy` field
  in the ctypes settings. (ocaml/dune#5827, @droyo)

- Fix `dune coq top` when it is invoked on files from a subdirectory of the
  directory containing the associated stanza (ocaml/dune#5784, fixes ocaml/dune#5552, @ejgallego,
  @rlepigre, @Alizter)

- Fix hint when an invalid module name is found. (ocaml/dune#5922, fixes ocaml/dune#5273, @emillon)

- The `(cat)` action now supports several files. (ocaml/dune#5928, fixes ocaml/dune#5795, @emillon)

- Dune no longer uses shimmed `META` files for OCaml 5.x, solely using the ones
  installed by the compiler. (ocaml/dune#5916, @dra27)

- Fix handling of the `(deps)` field in `(test)` stanzas when there is an
  `.expected` file. (ocaml/dune#5952, ocaml/dune#5951, fixes ocaml/dune#5950, @emillon)

- Ignore insignificant filesystem events. This stops RPC in watch mode from
  flashing errors on insignificant file system events such as changes in the
  `.git/` directory. (ocaml/dune#5953, @rgrinberg)

- Fix parsing more error messages emitted by the OCaml compiler. In
  particular, messages where the excerpt line number started with a blank
  character were skipped. (ocaml/dune#5981, @rgrinberg)

- env stanza: warn if some rules are ignored because they appear after a
  wildcard rule. (ocaml/dune#5898, fixes ocaml/dune#5886, @emillon)

- On Windows, XDG_CACHE_HOME is taken to be the `FOLDERID_InternetCache` if
  unset, and XDG_CONFIG_HOME and XDG_DATA_HOME are both taken to be
  `FOLDERID_LocalAppData` if unset.  (ocaml/dune#5943, fixes ocaml/dune#5808, @nojb)
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.

4 participants