dune-project multiple changes: required lablgtk version, generate opam files, update generated files

[liyishuai]

Follow up #658, addressing some review comments.

Now dune build generates the .opam files, avoiding potential syntax errors caused by handwriting.

Better check in the generated .opam files to the repository, since:

1. dune-project is not updated frequently
2. Hosting .opam files allows opam publish to release to OPAM more automatically.

[gdt]

Thank you for your submission, and I just now saw #674. After 2.52.0 I will have to figure out how these two relate, but if you and @olafhering could end up with a consensus set of changes that would be great.

[olafhering]

IMO opam files should not be generated by default. And generated files should not be in a SCM anyway.

The commit message does not state why changing generate_opam_files to true a desirable change.

Whatever component is the consumer of these generated local opam files, the build command is apparently switched from make to dune. Most likely this will yield wrong results on a number of target architectures.

[liyishuai]

I'm releasing control of this PR branch. Please (force-)push at will.

[gdt]

Thanks. I will look at this after release and afterwards if you want to sort through changes and rebase I will consider, or if you think whatever I do is good enough that's fine too.

[gdt]

Release made. Please either rebase on top of current master or close, if everything was in #674. (I'm trying to avoid any work I can as I feel like my time on unison is a bottleneck.)

[gdt]

I don't want to change to generated opam files without a proper discussion about why or why not, and I think that belongs on unison-hackers@. And, if we do change to generated, and the decision is made to have generated files checked in (like strings.ml is now), I would like commits separated into logical changes and regen operations. I'm not asking you to do all this work; to me the first thing is agreement on what we ought to do and why. I don't know enough about dune/opam to have a strong opinion yet.

If you want to just have the lablgtk min version change, that seems ok. With any luck that is the version that the build instructions say.

[liyishuai]

I'm still proposing to generate OPAM files from dune-project and check them in. This is actually two questions:

1. Q: Write or generate OPAM files? A: Generating them from dune-project helps keeping them synchronized.
2. Q: Check in OPAM files or ignore them? A: opam-publish requires OPAM files hosted in the repo. If we publish to OPAM manually, then there's indeed no need to check them in, and the current unison.opam should be removed to avoid confusion.

[gdt]

Feel free to bring up on unison-hackers. There is already a comment here opposing the change.

[tleedjarv]

A few random questions.

What does (version dev) mean? Should it be something else (or just removed)?

Should (wrapped_executables false) be added?

[olafhering]

(version ) is supposed to carry the version of the project. This variable can be referenced in other dune files, if needed. I think changing it should be wired up to the release process. In case a new version gets tagged, also this file should be changed. No idea if the release process is documented.

But at this point the value, or even the existence, does not matter because nothing consumes the version. Also, unison is no library, which means the version will not be part of the ocamlfind META files.

I have to check what wrapped_executables actually does.

[gdt]

The requested discussion on unison-hackers has not occurred, and there's an argument against it above, so this PR is on hold, and I'm inclined to close it after 30 days of the feedback label.

[liyishuai]

I haven’t received post access to that mailing list, and would prefer launching the discussion after depositing my dissertation in May.

[gdt]

OK, happy to wait until 7/1. It's been a long time, but I understand how finishing a thesis is, so good luck, and we'll probably still be here when you come out the other side.

[olafhering]

I'm meanwhile ok with this PR.

In the context of opam-repository, the current result of opam build unison is a binary with text UI. This will not change with this PR. In addition the opam-repository will get a unison-gui and unison-fsmonitor target.

I think it is safe to leave to built-in default for wrapped_executables as it is, and not specify this in the dune-project file.

What we/someone may do in the future is to deal with dune build on non-Linux targets. Most likely another PR is required to build unison for the various targets with dune in the github CI.

[liyishuai]

Another reason for generating unison.opam from dune-project: #566 updated the OPAM file and left dune-project untouched, making these files inconsistent on dependencies: https://github.com/bcpierce00/unison/blob/2a73c1fc781d2b7242b2820ec680abbff3545d83/dune-project#L52-L57 https://github.com/bcpierce00/unison/blob/2a73c1fc781d2b7242b2820ec680abbff3545d83/unison.opam#L13-L18

[gdt]

I see your point, but this is a cultural battle about which file is the one that is primary, and seems to be basically a tussle between two packaging systems. Unison is an outsider to this tussle, and I am inclined not to wade into it.

[olafhering]

Making dune-project the primary is the stated goal.

[gdt]

I understand that, but I have not accepted that goal, so far. I'm not opposed; it just seems like a more complicated issue.

[liyishuai]

Dune is not yet a packaging system, but a build system that allows exporting package descriptions. A dune-based project has its standardized build procedure, so writing dune-project and unison.opam in parallel poses confusion that which file to trust.

[gdt]

This PR is still pending a discussion on unison-hackers sorting out all the issues.

[liyishuai]

I've sent an email to unison-hackers on June 24. Has it been distributed or should I resend it?

[gdt]

You sent a very brief note, but it did not really explain the issues enough for someone not an opam expert to understand. I've seen comments that we shouldn't remove opam, and comments that we should because it's now generated, and comments that seem to say that dune and opam are sort of the same thing, and that they are different.

It might help to write the argument for why the change is right into the commit message, vs just saying 'polish', and to rebase as a lot as hit master. I expect the rebase is trivial, but it takes a potential hiccup off the table.

[liyishuai]

I've rebased the commit on top of the current master branch.

I have no idea of how to write the commit message, as I don't know what audience it is for. "Someone not an opam expert" should not be in charge of reviewing the changes here, and I lack the ability to teach OPAM. These changes are proposed as-is. I've granted the maintainers (force-)push access to this branch to change the commit message and/or contents.

[olafhering]

I'm not sure if it was already discussed in the context of unison: why are the build rules added? I remember most other projects removed it in the recent past from their dune-project and opam files (ocaml/opam-repository#14266).

Did you add it intentionally?

[liyishuai]

The build rules should be added if we are to provide unison* packages as executables only. If we are to have others use Unison as a library, then the build tags should be removed.

[liyishuai]

The build rules were introduced in #698. I've made 7fa617368261deb12553a00c673e55526dbf6b26 a separate commit that we can either squash or drop.

[tleedjarv]

Feel free to remove the build rules. I don't use opam myself, so trust your knowledge on the matter. That said, it seems to me that the build rules were removed in opam repository as a workaround and there never was a proper fix to retire this workaround. Also, probably could be a different approach for libraries and applications, as you mentioned.

[gdt]

Please see https://lists.seas.upenn.edu/pipermail/unison-hackers/2022-November/002061.html and followup on the list.

[liyishuai]

I've made the following changes based on the discussions on unison-hackers:

• Update the maintainers field of OPAM files to unison-hackers.
• Remove the dependency on Dune from the OPAM files.

No Not Merge Until:

• The depends and build rules are fixed in accordance with the Makefile; and
• The OPAM files are tested on CI.