Created attachment 352926 [details] [review]
meson: Check for FontConfig before enabling FreeType support

Hi,

This is the update to the meson build files to check for FontConfig before enabling FreeType support, as FontConfig is really required for this.

With blessings, thank you!

Created attachment 352929 [details] [review]
tests: Don't include unistd.h unconditionally

Hi,

This updates the test programs that include unistd.h to only include it on non-Visual Studio, and instead include io.h if necessary, as Visual Studio does not come with unistd.h

With blessings, thank you!

Ping?

Created attachment 356925 [details] [review]
meson: Check for Harfbuzz and FontConfig before enabling FreeType support

Hi,

It turns out that we need to also check for HarfBuzz before we can enable PangoFT2, as it depends on HarfBuzz, in addition to FontConfig and FreeType.  I also cleaned up the checks for them a bit in this new patch.

With blessings, thank you!

Created attachment 356926 [details] [review]
tests: Don't include unistd.h unconditionally (take ii)

Hi,

For the tests, simply don't include unistd.h and include io.h when necessary, when we are on Windows, since unistd.h shipped with MinGW is essentially a wrapper that includes io.h among other standard headers shipped with Windows compilers.

With blessings, thank you!

Created attachment 356927 [details] [review]
meson: Build resource (.rc) files on Windows

Hi,

The MSVC projects and the autotools builds on Windows both build the resource (.rc) files into PangoCairo, PangoFT2, PangoWin32 and Pango DLLs, so we ought to do likewise with the Meson builds, so that people can determine the version info of the DLLs more easily.

With blessings, thank you!

Created attachment 356928 [details] [review]
meson: Clean up things a bit

Hi,

We can just force-include msvc_recommended_pragmas.h on Visual Studio, which will do the job of silencing the unwanted noise as well as pointing out potential issues in the code, so we can clean things up a little bit for Visual Studio builds.

With blessings, thank you!

Created attachment 357081 [details] [review]
meson: Check for HarfBuzz and FontConfig before enabling FreeType support (take ii)

Hi,

It turns out that we need to check that we are really ready to build PangoFT2 before enabling them, such as docs and .pc files, as they really require the presence of PangoFT2.

With blessings, thank you!

Review of attachment 356926 [details] [review]:

Okay.

Is it worth adding a test for unistd.h in the build, and then use something like:

```
#ifdef HAVE_UNISTD_H
#include <unistd.h>
#endif
```

Review of attachment 356927 [details] [review]:

Looks good, plus minor nitpicks.

::: pango/meson.build
@@ +66,3 @@
 pango_features_conf.set('PANGO_VERSION_MICRO', pango_micro_version)
+pango_features_conf.set('PANGO_API_VERSION', pango_api_version)
+pango_features_conf.set('PANGO_CURRENT_MINUS_AGE', '0')

I'm not sure what 'current-minus-age' is supposed to be, to be fair. It's basically always 0.

Maybe it's time to retire it?

::: pango/pango.rc.in
@@ +18,3 @@
 	VALUE "FileVersion", "@PANGO_VERSION_MAJOR@.@PANGO_VERSION_MINOR@.@PANGO_VERSION_MICRO@.0"
 	VALUE "InternalName", "pango-@PANGO_API_VERSION@-@PANGO_CURRENT_MINUS_AGE@"
+	VALUE "LegalCopyright", "Copyright (C) 1999 Red Hat Software."

Pet peeve: this line reads as "Copyright copyright", because "(C)" (as well as ©) means "copyright".

You can choose between "Copyright", "(C)", or "©", but not two or more of them.

::: pango/pangocairo.rc.in
@@ +18,3 @@
 	VALUE "FileVersion", "@PANGO_VERSION_MAJOR@.@PANGO_VERSION_MINOR@.@PANGO_VERSION_MICRO@.0"
 	VALUE "InternalName", "pangocairo-@PANGO_API_VERSION@-@PANGO_CURRENT_MINUS_AGE@"
+	VALUE "LegalCopyright", "Copyright (C) 2010 Red Hat Software."

Same as above.

::: pango/pangoft2.rc.in
@@ +18,3 @@
 	VALUE "FileVersion", "@PANGO_VERSION_MAJOR@.@PANGO_VERSION_MINOR@.@PANGO_VERSION_MICRO@.0"
 	VALUE "InternalName", "pangoft2-@PANGO_API_VERSION@-@PANGO_CURRENT_MINUS_AGE@"
+	VALUE "LegalCopyright", "Copyright (C) 1999 Red Hat Software. Copyright (C) 2000 Tor Lillqvist"

Same as above.

::: pango/pangowin32.rc.in
@@ +18,3 @@
 	VALUE "FileVersion", "@PANGO_VERSION_MAJOR@.@PANGO_VERSION_MINOR@.@PANGO_VERSION_MICRO@.0"
 	VALUE "InternalName", "pangowin32-@PANGO_API_VERSION@-@PANGO_CURRENT_MINUS_AGE@"
+	VALUE "LegalCopyright", "Copyright (C) 1999 Red Hat Software. Copyright (C) 2000 Tor Lillqvist"

Same as above.

Review of attachment 356928 [details] [review]:

Okay.

There's a minimal difference between the current approach of checking all compiler flags and including the header, which is we can avoid using strict compiler warnings if we don't want them. I don't think it's worth troubling our sleep over it, to be fair.

Review of attachment 357081 [details] [review]:

Looks generally good to me.

::: pango-view/meson.build
@@ +31,3 @@
 endif
 
+cc = meson.get_compiler('c')

cc should already be defined, since this meson.build file is included after the top-level meson.build declared the `cc` variable.

Review of attachment 356926 [details] [review]:

Hi Emmanuele,

Thanks...

Hmm, we could do that, the reason I did this with this approach is because we don't include config.h in any of these files.  But we would still need to include io.h on Windows in test-common.c either via MinGW/mingw-w64's unistd.h, or directly.

I pushed the original patch anyways.

With blessings, thank you!

Review of attachment 356928 [details] [review]:

Hi Emmanuele,

Thanks, I pushed the patch as bcbe5c9.

I forgot to mention the other patch on unistd.h was committed as 9b11f35.

With blessings, thank you!

Created attachment 357172 [details] [review]
meson: Check for HarfBuzz and FontConfig before enabling FreeType support (take iii)

Hi,

Here's the new patch with the 'cc = ...' purged, as it was indeed extraneous.

With blesssings, thank you!

Created attachment 357173 [details] [review]
meson: Check for HarfBuzz and FontConfig before enabling FreeType support (take iv)

Hi,

Re-uploading this patch as I realized there was a dumb mistake on my side on whether to build pangocairo with FontConfig/FreeType support... :|

Created attachment 357174 [details] [review]
meson: Buld resource (.rc) files on Windows (take ii)

Hi Emmanuele,

This is the new patch that gets rid of the "(C)" in the .rc.in files.  I think I am for getting rid of the libtool age/version stuff that you mentioned, but I am leaving it in there for the moment, until we are sure that we want to get rid of it.

With blessings, thank you!

Created attachment 357875 [details] [review]
meson: Check for HarfBuzz and FontConfig before enabling FreeType support (take v)

Hi,

It seems that I need to update this patch to go back to HAVE_PANGO_FREETYPE as we want to use that check macro to build PangoCairo with FreeType (FontConfig) support, so that should not have been changed.

With blessings, thank you!

Created attachment 357877 [details] [review]
meson: Add fallbacks for finding FreeType, Harfbuzz, Cairo and FontConfig on Visual Studio

Hi,

Many of the non-GNOME dependencies of Pango do not generate pkg-config files for themselves in their build systems for Visual Studio, which means that we want to fallbacks to look for them when we are building using Visual Studio, by using cc.hsa_header() and cc.find_library().  This also avoids the need to use crafted pkg-config files for them which can be error-prone and tedious.

For Cairo, after we found Cairo using the has_header() and find_library() approach, we need to be test for the individual items whether they are supported, as Cairo by itself has many build options, so:

-For the Win32 and Quartz font backend support, along with the PDF, PS and XLib
 surface backend support, we use has_header() for cairo-xxx.h, since
 cairo-features.h will determine whether the resspective header could be
 included successfully.
-For the FreeType font backend support, we need to check Cairo is built with
 *both* FreeType and FontConfig support, by testing whether the FontConfig-using
 Cairo API compiles and links.
-For the PNG output surface support, we need to check whether Cairo is built
 with such support as well.

With blessings, thank you!

Created attachment 358108 [details] [review]
meson: Add fallbacks for finding FreeType, Harfbuzz, Cairo and FontConfig on Visual Studio (take ii)

Hi,

It seems that we need to update how pangocairo.pc is generated as well, as on Visual Studio cairo.pc may very well not exist.  This is so that items that need to use pangocairo.pc can use it properly.

I also made the fallback checks, including the headers, more for Visual Studio only.

With blessings, thank you!

Review of attachment 357174 [details] [review]:

Looks good to me. Fix the issue pointed out and feel free to push it.

::: pango/meson.build
@@ +348,3 @@
       'pangocairo-win32fontmap.c',
     ]
+

let's not add so many empty lines

Review of attachment 357174 [details] [review]:

Hi Nacho,

Thanks, and I sure did overlook the extra white spaces, that was pointed out.  The patch was fixed with those extra white spaces removed, as 2b4f160.

With blessings, thank you!

Review of attachment 357875 [details] [review]:

See the comment inline

::: pango-view/meson.build
@@ +31,3 @@
 endif
 
+strcasecmp_define = []

I'd like to see this as a differeent patch. Moreover, can we use glib instead of calling strcasecmp so we can avoid this completely?

Review of attachment 358108 [details] [review]:

Patch looks good to me. See the minor comment inline. Also get an ack from Matthias before pushing it.

::: meson.build
@@ -267,2 +306,3 @@
   # - backend name
   # Note that Cairo can be built with FreeType but without FontConfig
+

do not add empty line

Review of attachment 358108 [details] [review]:

Let's get this in, none sense to wait longer.

Created attachment 362529 [details] [review]
meson: Check for HarfBuzz and FontConfig before enabling FreeType support (take vi)

Hi Nacho,

Sorry, it took me some time to get back to this... This is the check for HarfBuzz and FontConfig sans the part where we compensate for strcasecmp(), which will be in a subsequent patch.

I will push the patch for the non-GNOME deps after this gets in, as it depends on this patch in a way.  Thanks for the ack though.

With blessings, thank you!

Created attachment 362530 [details] [review]
pango-view/viewer-cairo.c: Use g_ascii_strcasecmp() instead of strcasecmp()

Hi,

This makes use of g_ascii_strcasecmp() instead of strcasecmp() as not all supported compilers support strcasecmp(), such as Visual Studio (2017 included, even).

With blessings, thank you!

Created attachment 365472 [details] [review]
Meson: Provide option not to build introspection files

Hi,

This adds support to the Meson build process to not build the introspection files even if GOBject-Introspection is found, as it is done in ATK.

With blessings, thank you!

Created attachment 369986 [details] [review]
meson: Check for HarfBuzz and FontConfig before enabling FreeType support (take v)

Hi,

This is the new version of the patch that was slightly adjusted and rebased against upstream.

With blessings, thank you!

Created attachment 369987 [details] [review]
meson: Add fallbacks for finding FreeType, Harfbuzz, Cairo and FontConfig on Visual Studio (take iii)

Hi,

This expands the previous version of the patch as we need to pay attention to how pangoft2.pc is generated.  This also fixes the case where we generate pangocairo.pc when both pangowin32 and pangoft2 are built.

With blessings, thank you!

Created attachment 369988 [details] [review]
tests: Don't include unistd.h in tests/markup-parse.c unconditionally

Hi,

An unconditional unistd.h inclusion crept into markup-parse.c, so update the code not to include it unconditionally, and include the corresponding header on Windows when unistd.h is not found.

With blessings, thank you!

Review of attachment 369986 [details] [review]:

Looks ok to me

Review of attachment 369987 [details] [review]:

Fine for me

Review of attachment 369988 [details] [review]:

Sure

Hi Nacho,

Thanks for the reviews, I pushed the patches as follows:
369986: 55afeec
369987: 18fb749
369988: 08649ce

I will close the bug shortly-Pango is now buildable with Visual Studio and Meson out-of-the-box.

With blessings, thank you!