Build breaks if either of CAIRO_HAS_FT_FONT or CAIRO_HAS_FC_FONT is not defined

[tleedjarv]

I was testing the new opam system-msvc feature and found a few small issues that break building cairo2. Apologies for cramming all into the same ticket, yet they're all kind of related.

I propose fixes along the way but I'm unable to open a proper PR because I haven't tested those on other systems.

First, there is a compilation issue where a non-C99 variadic macro is used. Proposed fix:

diff --git a/src/cairo_stubs.c b/src/cairo_stubs.c
index b32de99..bb048d7 100644
--- a/src/cairo_stubs.c
+++ b/src/cairo_stubs.c
@@ -1613,13 +1613,13 @@ SURFACE_CREATE_DATA(data8)
 SURFACE_CREATE_DATA(data32)
 #undef b

-#define SURFACE_GET_DATA(type, num_dims, dims ...)                      \
+#define SURFACE_GET_DATA(type, num_dims, ...)                           \
   CAMLexport value caml_cairo_image_surface_get_##type(value vsurf)     \
   {                                                                     \
     CAMLparam1(vsurf);                                                  \
     CAMLlocal1(vb);                                                     \
     unsigned char* data = cairo_image_surface_get_data(SURFACE_VAL(vsurf)); \
-    intnat dim[num_dims] = {dims};                                      \
+    intnat dim[num_dims] = {__VA_ARGS__};                               \
     struct caml_ba_proxy * proxy = (struct caml_ba_proxy *)             \
       cairo_surface_get_user_data(SURFACE_VAL(vsurf), &image_bigarray_key); \
                                                                         \

bca3b2c fixes a similar issue but by duplicating the macro definition. This should not be necessary as all compilers should support the C99 syntax for a long time now.

Next, if either of CAIRO_HAS_FT_FONT or CAIRO_HAS_FC_FONT is not defined, the build breaks because the dune discover script will not produce cairo_ocaml.h at all. Additionally, the fallback stub names are incorrect and will cause link failures. Proposed fix:

diff --git a/config/discover.ml b/config/discover.ml
index e95cb5d..279bb53 100644
--- a/config/discover.ml
+++ b/config/discover.ml
@@ -15,7 +15,7 @@ let default_cairo c =
       libs = ["/LC:\\gtk\\lib"; "cairo.lib"] }
   else { P.cflags = ["-I/usr/include/cairo"];  libs = ["-lcairo"] }

-let c_header_has_ft () =
+let c_header ~has_ft () =
   let fh = open_in "cairo_ocaml.h.p" in
   let buf = Buffer.create 4096 in
   let b = Bytes.create 4096 in
@@ -25,8 +25,12 @@ let c_header_has_ft () =
   done;
   close_in fh;
   let s = Buffer.contents buf in
-  let re = Str.regexp "/\\* *#define *OCAML_CAIRO_HAS_FT .*\\*/" in
-  let s = Str.global_replace re "#define OCAML_CAIRO_HAS_FT 1" s in
+  let s =
+    if has_ft then
+      let re = Str.regexp "/\\* *#define *OCAML_CAIRO_HAS_FT .*\\*/" in
+      Str.global_replace re "#define OCAML_CAIRO_HAS_FT 1" s
+    else s
+  in
   let fh = open_out "cairo_ocaml.h" in
   output_string fh s;
   close_out fh
@@ -70,6 +74,7 @@ let discover_cairo c =
     | C.C_define.Value.Switch b -> b
     | _ -> false in
   let cflags, libs =
+    c_header ~has_ft:has_ft_font ();
     if has_ft_font && has_fc_font then (
       match P.get c with
       | Some p ->
@@ -81,7 +86,6 @@ let discover_cairo c =
                if String.length f > 2 && f.[0] = '-' && f.[1] = 'I' then
                  f :: (f ^ "/freetype") :: l
                else f :: l in
-             c_header_has_ft ();
              (List.fold_left freetype [] f.cflags @ cflags,
               f.libs @ libs)
           | None -> cflags, libs)
diff --git a/src/cairo_stubs.c b/src/cairo_stubs.c
index bb048d7..2b35a36 100644
--- a/src/cairo_stubs.c
+++ b/src/cairo_stubs.c
@@ -1228,15 +1228,15 @@ CAMLexport value caml_cairo_ft_synthesize_unset(

 #else

-UNAVAILABLE1(Ft_init_FreeType)
-UNAVAILABLE2(caml_Ft_new_face)
-UNAVAILABLE3(caml_cairo_ft_create_for_ft_face)
-UNAVAILABLE2(caml_cairo_ft_create_for_pattern)
-UNAVAILABLE1(caml_cairo_ft_scaled_font_lock_face)
-UNAVAILABLE1(caml_cairo_ft_scaled_font_unlock_face)
-UNAVAILABLE1(caml_cairo_ft_synthesize_get)
-UNAVAILABLE3(caml_cairo_ft_synthesize_set)
-UNAVAILABLE3(caml_cairo_ft_synthesize_unset)
+UNAVAILABLE1(cairo_Ft_init_FreeType)
+UNAVAILABLE2(cairo_Ft_new_face)
+UNAVAILABLE3(cairo_ft_create_for_ft_face)
+UNAVAILABLE2(cairo_ft_create_for_pattern)
+UNAVAILABLE1(cairo_ft_scaled_font_lock_face)
+UNAVAILABLE1(cairo_ft_scaled_font_unlock_face)
+UNAVAILABLE1(cairo_ft_synthesize_get)
+UNAVAILABLE3(cairo_ft_synthesize_set)
+UNAVAILABLE3(cairo_ft_synthesize_unset)

 #endif

Current code requires both CAIRO_HAS_FT_FONT and CAIRO_HAS_FC_FONT to be defined. Requiring both should not be necessary and currently breaks if only CAIRO_HAS_FT_FONT is defined (even with the proposed fix above). Proposed fix:

diff --git a/config/discover.ml b/config/discover.ml
index 279bb53..8773074 100644
--- a/config/discover.ml
+++ b/config/discover.ml
@@ -75,7 +75,7 @@ let discover_cairo c =
     | _ -> false in
   let cflags, libs =
     c_header ~has_ft:has_ft_font ();
-    if has_ft_font && has_fc_font then (
+    if has_ft_font || has_fc_font then (
       match P.get c with
       | Some p ->
          (match P.query p ~package:"fontconfig freetype2" with
diff --git a/src/cairo_stubs.c b/src/cairo_stubs.c
index 2b35a36..a4fd3cd 100644
--- a/src/cairo_stubs.c
+++ b/src/cairo_stubs.c
@@ -1082,7 +1082,7 @@ CAMLexport value caml_cairo_scaled_font_get_type(value vff)
 /* Ft : TrueType fonts
 ***********************************************************************/

-#if CAIRO_HAS_FT_FONT && CAIRO_HAS_FC_FONT
+#if CAIRO_HAS_FT_FONT
 #include <cairo-ft.h>

 CAMLexport value caml_cairo_Ft_init_FreeType(value unit)
@@ -1132,6 +1132,8 @@ CAMLexport value caml_cairo_ft_create_for_ft_face(
   CAMLreturn(vff);
 }

+#if CAIRO_HAS_FC_FONT
+
 CAMLexport value caml_cairo_ft_create_for_pattern(
   value voptions, value vpattern)
 {
@@ -1170,6 +1172,12 @@ CAMLexport value caml_cairo_ft_create_for_pattern(
   CAMLreturn(vff);
 }

+#else
+
+UNAVAILABLE2(cairo_ft_create_for_pattern)
+
+#endif
+
 CAMLexport value caml_cairo_ft_scaled_font_lock_face(value vsf)
 {
   CAMLparam1(vsf);

Finally, while pkg-config is expected to be the primary source of configuration, tweaking the defaults slightly can produce a fully working build even without pkg-config available, which might be helpful for someone:

diff --git a/config/discover.ml b/config/discover.ml
index 8773074..37c183b 100644
--- a/config/discover.ml
+++ b/config/discover.ml
@@ -10,9 +10,13 @@ let default_cairo c =
   (* In case pkg-config fails *)
   let sys = C.ocaml_config_var_exn c "system" in
   if sys = "msvc" || sys = "win64" then
+    let () = prerr_endline "Warning: You may need to add the location of \
+      GTK installation (for example, C:\\gtk\\bin) to environment variable \
+      PATH." in
     { P.cflags = ["-I"; "C:\\gtk\\include\\cairo";
-                  "-I"; "C:\\gtk\\include\\freetype2"];
-      libs = ["/LC:\\gtk\\lib"; "cairo.lib"] }
+                  "-I"; "C:\\gtk\\include\\freetype2";
+                  "-I"; "C:\\gtk\\include\\freetype2\\freetype"];
+      libs = ["-LC:\\gtk\\lib"; "cairo.lib"] }
   else { P.cflags = ["-I/usr/include/cairo"];  libs = ["-lcairo"] }

 let c_header ~has_ft () =
@@ -76,6 +80,10 @@ let discover_cairo c =
   let cflags, libs =
     c_header ~has_ft:has_ft_font ();
     if has_ft_font || has_fc_font then (
+      let default_ft_fc_libs =
+        [if has_ft_font then "-lfreetype" else "";
+         if has_fc_font then "-lfontconfig" else ""]
+      in
       match P.get c with
       | Some p ->
          (match P.query p ~package:"fontconfig freetype2" with
@@ -88,8 +96,8 @@ let discover_cairo c =
                else f :: l in
              (List.fold_left freetype [] f.cflags @ cflags,
               f.libs @ libs)
-          | None -> cflags, libs)
-      | None -> cflags, libs
+          | None -> cflags, default_ft_fc_libs @ libs)
+      | None -> cflags, default_ft_fc_libs @ libs
     )
     else cflags, libs in
   write ~cflags ~libs