Re: [PATCH v1 1/4] tools/ocaml/common.make: Remove '-cc $(CC)' flag from OCAMLOPTFLAGS

[Edwin Torok <edwin.torok@xxxxxxxxx>]

On Thu, Aug 22, 2024 at 1:25 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 22/08/2024 10:06 am, Andrii Sultanov wrote:
> > This flag does not work as assumed and will not pass
> > options (such as -shared) to the C compiler:
> > https://github.com/ocaml/ocaml/issues/12284
> >
> > Signed-off-by: Andrii Sultanov <andrii.sultanov@xxxxxxxxx>
> > ---
> >  tools/ocaml/common.make | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/ocaml/common.make b/tools/ocaml/common.make
> > index 0c8a597d5b..cc126b749f 100644
> > --- a/tools/ocaml/common.make
> > +++ b/tools/ocaml/common.make
> > @@ -12,7 +12,7 @@ OCAMLFIND ?= ocamlfind
> >  CFLAGS += -fPIC -I$(shell ocamlc -where)
> >
> >  OCAMLOPTFLAG_G := $(shell $(OCAMLOPT) -h 2>&1 | sed -n 's/^  *\(-g\) 
> > .*/\1/p')
> > -OCAMLOPTFLAGS = $(OCAMLOPTFLAG_G) -ccopt "$(LDFLAGS)" -dtypes 
> > $(OCAMLINCLUDE) -cc $(CC) -w F -warn-error F
> > +OCAMLOPTFLAGS = $(OCAMLOPTFLAG_G) -ccopt "$(LDFLAGS)" -dtypes 
> > $(OCAMLINCLUDE) -w F -warn-error F
> >  OCAMLCFLAGS += -g $(OCAMLINCLUDE) -w F -warn-error F
> >
> >  VERSION := 4.1
>
> This patch itself is fine, and I'll commit it in due course, but then I
> got looking at the surrounding context...
>
> `$(OCAMLOPT) -h` tells you on stderr to try `--help instead`, so
> OCAMLOPTFLAG_G is never going to contain -g.
>
> Also, why is -g conditional for OCAMLOPTFLAGS but unconditional for
> OCAMLCFLAGS?  I think we can safely drop OCAMLOPTFLAG_G

I'm not aware of a version of OCaml that didn't support -g, but maybe
a very old version (that wouldn't pass the minimum version check)
didn't have it.
I agree that we can safely drop the conditional and always pass `-g`.
>
>
> Next, there's VERSION and git grep says its only used in META files.
>
> xen.git/tools/ocaml$ git grep -w VERSION
> Makefile.rules:43:      sed 's/@VERSION@/$(VERSION)/g' < $< $o
> common.make:18:VERSION := 4.1
> libs/eventchn/META.in:1:version = "@VERSION@"
> libs/mmap/META.in:1:version = "@VERSION@"
> libs/xb/META.in:1:version = "@VERSION@"
> libs/xc/META.in:1:version = "@VERSION@"
> libs/xenstoredglue/META.in:1:version = "@VERSION@"
> libs/xenstoredglue/domain_getinfo_plugin_v1/META.in:1:version = "@VERSION@"
> libs/xs/META.in:1:version = "@VERSION@"
>
> 4.1 is very very stale and should say 4.19 these days (definitely for
> xc, and whatever else is using an unstable API), yet should definitely
> not be 4.19 for xenstoredglue.
>
> Are there any ABI/API implication from changing the META file?

It is purely informational (e.g. show up in the output of `ocamlfind
list`), dependency resolution is done using `opam` files (which Xen
doesn't have), not `META` files.
You can link some code into an executable that lists the versions of
all the libraries that it got linked with (using an OCamlfind module),
and in that case it might be nice to have correct information there,
but I don't think any of our code does that.

>
> ~Andrew