Hi,

Thanks for your work, this is a much needed update, and not an easy one!

At a cursory look the patch looks well composed, but before approval it requires some testing.

The first commits are preparation to individual ports which must be applied before applying the last commit. Those commits work on the current tree and should cause no regressions.

While I agree that keeping the commits separated in the revision is useful, especially for you, if the patches require updates, for the final commit to the ports tree I'd be more in favour of one big commit with a succinct description for the individual changes (as in a list of one line descriptions). There are reasons for this.

Every commit would require the commit message to be modified to add our standard tags, referencing this revision and all approved by/tested by lines.

This is my opinion, maybe other involved committers have different ideas.

After this REVISION is accepted, the lang/ocaml-nox11 port should be retired.

Can you explain the logic of this? While I don't really know why it exists, to remove it we need a reason.

In D43861#1000601, @madpilot wrote:

Hi,

Thanks for your work, this is a much needed update, and not an easy one!

At a cursory look the patch looks well composed, but before approval it requires some testing.

The first commits are preparation to individual ports which must be applied before applying the last commit. Those commits work on the current tree and should cause no regressions.

While I agree that keeping the commits separated in the revision is useful, especially for you, if the patches require updates, for the final commit to the ports tree I'd be more in favour of one big commit with a succinct description for the individual changes (as in a list of one line descriptions). There are reasons for this.

Every commit would require the commit message to be modified to add our standard tags, referencing this revision and all approved by/tested by lines.

This is my opinion, maybe other involved committers have different ideas.

The idea was that each commit is not introducing any more breakage (tree is buildable at any point), but it is easier to reviewed independently. It doesn't really matter in the end. I suppose that once the review is done, I can push a squashed commits with proper commit message that contains all the information plus the necessary tags.

After this REVISION is accepted, the lang/ocaml-nox11 port should be retired.

Can you explain the logic of this? While I don't really know why it exists, to remove it we need a reason.

Well the Graphics module got dropped from 4.13.0. It lives in its own upstream repositoy now. So lang/ocaml has essentially became what lang/ocaml-nox11 was. The later having no more purpose, there is no point in keeping it. The new-old Graphics module (== the new upstream repository) is now packaged in x11-toolkits/ocaml-graphics, and it is much nicer like this (better build system, simplifies lang/ocaml).

In D43861#1003621, @madpilot wrote:

Hi!

I tested your patch in poudriere. In general it is very good from my POV, congrats on your work.

Thank you very much for the review!

I noticed a few minor issues (plist issues mainly) that you should check. I created comments in the Makefiles of the ports having issues.

All those plist issues, along with other newly uncovered and longer standing issues, are now resolved.
Labltk and lablgl are now also moved into OCAML_SITELIBDIR/$package, with their stublibs.

Thank you again for the reviews. There had been a bit of fallover from the moving of labltk and lablGL to site-lib/, I should have catched them before...
All plist issues should be gone too, as I've learned to use -t with poudriere-bulk ;)

Also Mk/Uses/dune.ml was renamed to Mk/Uses/ocaml.ml, only USES= ocaml:dune is supported now and it still requires to USE_OCAML=yes for bsd.ocaml.mk get pulled in. My ultimate goal is to remove bsd.ocaml.mk, but that's better left for another revision as this ocaml 4.14 update is getting quite massive by now.

I've squashed the changes to prepare for the final commit message: only missing the Approved by: and Exp-run by : tags now.