Re: [PATCH v2 4/5] tools/ocaml/libs/xc: OCaml 5.0 compatibility

[Edwin Torok <edvin.torok@xxxxxxxxxx>]

> On 30 Sep 2022, at 15:59, Christian Lindig <christian.lindig@xxxxxxxxxx> 
> wrote:
> 
> 
> 
>> On 27 Sep 2022, at 17:13, Edwin Torok <edvin.torok@xxxxxxxxxx> wrote:
>> 
>> 
>> See below for a patch for that. I've included this patch in the correct 
>> place (before the patch that breaks it) in the git repository at: 
>> https://github.com/edwintorok/xen/compare/private/edvint/public0
>> 
> 
> 
> Acked-by: Christian Lindig <christian.lindig@xxxxxxxxxx>
> 
> I believe these changes are fine. We are now allocating the event channel 
> dynamically and this requires using a finaliser to free that memory. 

Thanks,

> 
> 
>> -ifneq ($(MAKECMDGOALS),clean)
>> +ifeq (,$(findstring clean,$(MAKECMDGOALS)))
>> .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile 
>> $(OCAML_TOPLEVEL)/Makefile.rules
>>      $(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli 
>> $o,MLDEP,)
>> endif
> 
> Is this the right logic? Moving from ifneq to ifeq here.
> 
> I am not so familiar with the Makfile rules. The gist seems to be: we depend 
> on auto-generated Make dependencies that the Makefile in general depends on. 
> But in a “make clean” (or other “*clean” it is wasteful to generate these 
> only to later remove them. However, these kind of subtleties are obvious 
> enough while we are working on this but over time accumulate to Makefiles 
> that are hard to change. So I wonder if this kind of optimisation, while 
> correct, is worth it, but fine going along with it.
> 

Makefile functions can be a bit confusing to read.

"ifneq ($(MAKECMDGOALS), clean)" means $(MAKECMDGOALS) != "clean"
"ifeq (,$(findstring clean,$(MAKECMDGOALS)))" means that "clean" in 
$(MAKECMDGOALS) == "" (the empty string), or i.o.w. "clean" not in 
$(MAKECMDGOALS), which is a bit more generic than the previous one,
since we have all sorts of rules in the Makefile (especially around subdirs) 
where 'clean' is a substring.
This is quite subtle and I had to reread this line many times too to check it 
is correct.

The real solution here would be to have a single non-recursive Makefile (and 
there is some discussion/patches heading in that direction in xen-devel 
particularly from Anthony), and then evaluating the "clean" rules would be a 
lot less expensive, it'd only have to be done once. But there might be a while 
until we get there, and meanwhile these clean rules slow down the OCaml build 
too much (just running the "clean" takes a lot longer than building the entire 
OCaml libraries and oxenstored sequentially).

Although I only need to use 'clean' when using the upstream Makefiles (where 
almost every incremental change requires a 'clean' inbetween because the 
Makefiles express the dependencies incorrectly),
or when switching from upstream Makefile to 'dune' (a one-off event usually).
Since I use 'Dune' for my daily work anyway (and the makefile is used in our 
internal build system) perhaps this Makefile patch is not needed at all, I can 
change 'Makefile.dune' to not call 'make clean' at all, and I'll know to 
remember to run it if things fail anyway (it'll be pretty obvious when Dune 
says you've got a build artifact in the wrong place).

Best regards,
--Edwin