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@cloud.com>
---
 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
-- 
2.39.2

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@cloud.com>
> ---
>  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


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?

~Andrew

On Thu, Aug 22, 2024 at 1:25 PM Andrew Cooper <andrew.cooper3@citrix.com> 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@cloud.com>
> > ---
> >  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

> On 22 Aug 2024, at 13:25, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> Are there any ABI/API implication from changing the META file?

The META file is for package/library management in an OCaml environment. I don’t see much relevance for it in the code that is part of the Xen tree - so see no problem changing the version.

— C