[hannesm]

/cc @dinosaure @shym @palainp

[palainp]

Thank you! From my POV, the diff is small enough to use sed again :)

[hannesm]

my thinking is that with the two patches we apply nowadays, a sed is more robust, and it will allow us to more quickly add new OCaml releases (famous last words). this as well drops the opatch dependency.

[shym]

At that number of fixes, I agree that going with sed is reasonable. But I’d rather not lose the documentation that makes maintaining that a lot easier (converting from partially broken sed patching to real patches was unnecessarily painful for that reason).
So I propose this extra commit to keep the documentation (I hope it’s portable, but I don’t have a FreeBSD machine to really check). I also propose to keep the patching inside the rule that generates the ocaml directory, like before, instead of all configuration steps.
Note also that we lose the detection of when the patching doesn’t do what we expect it to, as sed will ignore if no change was performed. (We could add a final check that the files were at least modified, but maybe let’s wait to be bitten by it first).

[hannesm]

Thanks, I included and fixed your commit.

For patch 2, we have the test of hello;hvt (which uses fmt, and compilation won't suceed if the stdlib has a different checksum).

For patch 1, I guess it would be great to add a test / example. But I'm not entirely sure how to construct it -- as far as I can tell, we would need to inject the OCAMLRUNPARAM via environment, and could there test via d the Max_domains_max... While the Max_domains_def is only filling the initial params.max_domains -- and I can't see to figure out how to retrieve the caml_params->max_domains -- but maybe we can write some C stub for that?

[shym]

Sorry about the extra $ 🙈
For the check for patch 1, that’s a good idea! (it can be left for a future PR).

[hannesm]

@shym I pushed commits which check caml_params->max_domains, and also Max_domains_def / Max_domains_max.

[hannesm]

also look at the CI failure at 6e7a666 -- (where caml_params is accessed before caml_startup had a chance to fill it) -- which nicely fails.

so, below the line, /cc @dinosaure @reynir maybe they have an opinion on this PR.

the tests would as well have helped the 1.3.0/1.3.1 issues (where the patch didn't apply and we ended up with lots of memory being used) /cc @palainp

[shym]

This is nice, congratulations @hannesm!
Maybe a comment to make the link in the other direction, from example/startup.c to patch_ocaml.sh would be nice. In particular, as it’s in example, it could explain that it contains extra assertions for our test suite and that only _nolibc_init; caml_startup is really required there. (Though I expect very very few people use a different implementation from the one provided by mirage-solo5)

[hannesm]

good idea @shym, done in dc94134

[shym]

Another suggestion to make patch_ocaml.sh shellcheck-clean as its default warnings correspond to on-purpose choices.