-
Notifications
You must be signed in to change notification settings - Fork 27
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
Port to use Dune as build system #89
Conversation
21b8b28
to
0b3ed76
Compare
0b3ed76
to
4781f7a
Compare
Since the test-suite has been ported, what is left keeping this back? |
@Alizter the final refactorings that I have pending locally before I mark this to be ready are:
|
@palmskog I can split those .v files. |
I have already done the splitting locally, but I ran into the Dune issues discussed on Zulip. I'll take care of the final changes, and you can review when I mark as ready for review. |
5aab846
to
27b645d
Compare
@Alizter this PR is now ready for re-review. The only other changes I might do at this point (other those than suggested during review) are to documentation, which still talks about autoconf and similar. |
We still have the
issue. I am unsure if there is a work around other than doing |
@Alizter I can't replicate that locally (I don't get the error anymore). Shouldn't that error show up in CI then? I don't see it here: https://github.com/coq-community/coq-dpdgraph/runs/6453619501?check_suite_focus=true#step:4:390 |
@palmskog Seems my cache got corrupted from before, Actually no. Let me see. Seems to be an issue with coqdep. I suspect its because we are relying on the META file of the coq-dpdgraph package whilst generating rules that will be installed in the coq-dpdgraph package. |
OK I had a discussion with Rudi about this. I think barring Coq's poor design choice to have circular deps on META, the best solution here to avoid this problem is to have two separate packages. One for the plugin and one for the theory. We can later package them up further into coq-dpdgraph at the end for opam. |
@Alizter I don't think this is a workable solution. The "theory" here consists of exactly one line of code: Declare ML Module "coq-dpdgraph.plugin". coq-dpdgraph is currently a single package using autoconf + a regular Makefile that is part of the Coq Platform. To avoid the large overhead of maintaining two packages, I think we should then instead keep this PR open until the Dune issues with coqdep are sorted. |
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.
Never mind, I hadn't checked out the PR properly and was stuck on an older version where the ml and v fiels where mixed. That was causing the coq.theory stanza to break. Now that everything is in the proper folder it looks like it is working as intended.
@ybertot Ali and I believe this is ready to be merged now, but we would appreciate if you could take a look at the changes, in particular in the test suite. The key command you may want to run locally is: dune runtest One important thing to note is that if we merge this, we must update the build scripts in Coq's CI - the old build process scripted there will not work anymore. |
I have to run to a few things these days, I may not have time to look at it before the end of the week. I will try to keep it on my agenda. |
@ybertot do you think we can rebase+merge this PR and switch to Dune for a release of dpdgraph for Coq 8.17 now that 8.17+rc1 is out? If you don't mind, I could do it. |
@palmskog Are we really ready to switch to Dune for this? I thought we were going to do less changes to the tests then we currently do. Also what is the rush for 8.17? |
There is no big rush, but if we can throw out the dependency on autoconf/autotools for good for the next Coq Platform release (in March 2023 I guess), that would be good for everyone involved. Since I believe we faithfully capture all existing tests, there is nothing in my view that stand in the way. |
Moving from autoconf to dune means moving from a system I don't understand fully to a system I barely understand. Right now, I have administrative duties that make that I cannot guarantee reading the documentation of Dune before end of January, but it is good of you to have sent me a reminder. If there is no hurry, I suggest we pospone my answer another 2 or 3 weeks, to see if I can gather enough information by then. |
Fine by me to postpone further PR discussion until February. |
This reverts commit 7918c3f.
We also add a Makefile of common Dune commands. Signed-off-by: Ali Caglayan <alizter@gmail.com>
cc8eaf7
to
b5a6946
Compare
@ybertot I have added more details to the README.md and a basic Makefile with things like:
The CI also now runs the test-suite. |
@ybertot @Alizter do we have any consensus here on a plan for merging the PR? The advantage with a Dune-based build is that many people in Coq-community could potentially do releases of coq-dpdgraph when required - there is no local preparation of a release archive needed, just a Git tag and package definition. |
Thanks for the reminder. |
coq_makefile is not deprecated. |
…y/coq-master+dune"" This reverts commit f00d514. <!-- ps-id: 466b1c2e-3e9a-4670-b817-256bde3cbb79 -->
…y/coq-master+dune"" This reverts commit f00d514. <!-- ps-id: 466b1c2e-3e9a-4670-b817-256bde3cbb79 -->
Using the regular coq_makefile for coq-dpdgraph is a lot of work for an essentially deprecated approach to building Coq plugins. Here is a quick port to Dune instead. The plugin and tools build fine, but two issues are not resolved yet:
The first issue is straightforward, but the second one is time-consuming without in-depth Dune knowledge that I currently lack.
@ybertot perhaps we could merge this after the documentation is done and postpone the test suite port to some later point? We may be able to get some help from the Dune experts.