After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 785767 - Meson: Improve things a bit on Windows (MSVC side at least)
Meson: Improve things a bit on Windows (MSVC side at least)
Status: RESOLVED FIXED
Product: gdk-pixbuf
Classification: Platform
Component: general
git master
Other Windows
: Normal normal
: ---
Assigned To: gdk-pixbuf-maint
gdk-pixbuf-maint
Depends on:
Blocks:
 
 
Reported: 2017-08-03 10:09 UTC by Fan, Chun-wei
Modified: 2017-09-08 08:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
meson: Check for sys/time.h and sys/resources.h (1.46 KB, patch)
2017-08-03 10:15 UTC, Fan, Chun-wei
none Details | Review
gen-thumbnailer.py: Fix running on Windows. (1.20 KB, patch)
2017-08-03 10:19 UTC, Fan, Chun-wei
none Details | Review
meson: Fix build of built-in loaders (1.50 KB, patch)
2017-08-03 10:22 UTC, Fan, Chun-wei
none Details | Review
meson: Add option for relocatablilty (1.62 KB, patch)
2017-08-03 10:28 UTC, Fan, Chun-wei
none Details | Review
gen-thumbnailer.py: Fix running on Windows (take ii) (1.18 KB, patch)
2017-08-04 06:49 UTC, Fan, Chun-wei
none Details | Review
meson: Add a buildin_all_loaders configuration option (2.50 KB, patch)
2017-08-04 06:52 UTC, Fan, Chun-wei
none Details | Review
meson: Force-include msvc_recommended_pragmas.h on Visual Studio (2.15 KB, patch)
2017-08-04 07:01 UTC, Fan, Chun-wei
none Details | Review
meson: Check for lrint() and round() (2.00 KB, patch)
2017-08-04 07:23 UTC, Fan, Chun-wei
none Details | Review
meson: Build the resource (.rc) file on Windows (1.89 KB, patch)
2017-08-04 07:56 UTC, Fan, Chun-wei
none Details | Review
tests/pixbuf-short-gif-write.c: Fix on pre-C99 compilers (2.13 KB, patch)
2017-08-04 07:57 UTC, Fan, Chun-wei
none Details | Review
tests/pixbuf-short-gif-write.c: Fix on pre-C99 compilers (take ii) (2.19 KB, patch)
2017-08-07 04:28 UTC, Fan, Chun-wei
none Details | Review
build: Check for sys/resource.h in Meson (811 bytes, patch)
2017-08-15 02:08 UTC, Fan, Chun-wei
committed Details | Review
build: Check for sys/time.h (1.70 KB, patch)
2017-08-15 02:09 UTC, Fan, Chun-wei
committed Details | Review
build: Fix gen-thumbnailer.py on Windows (1.19 KB, patch)
2017-08-15 02:11 UTC, Fan, Chun-wei
committed Details | Review
build: Fix build of built-in loaders (1.81 KB, patch)
2017-08-15 02:12 UTC, Fan, Chun-wei
committed Details | Review
build: Add Mesib build option for relocatable builds (1.73 KB, patch)
2017-08-15 02:16 UTC, Fan, Chun-wei
none Details | Review
build: Allow 'all' shorthand for builtin_loaders build option (2.13 KB, patch)
2017-08-15 02:18 UTC, Fan, Chun-wei
none Details | Review
build: Force-include msvc_recommended_pragmas.h (2.48 KB, patch)
2017-08-15 02:19 UTC, Fan, Chun-wei
committed Details | Review
build: Check for lrint() and round() (1.99 KB, patch)
2017-08-15 02:20 UTC, Fan, Chun-wei
committed Details | Review
build Build the .rc files on Windows (2.42 KB, patch)
2017-08-15 02:23 UTC, Fan, Chun-wei
committed Details | Review
tests: Fix build on pre-C99 (2.17 KB, patch)
2017-08-15 02:24 UTC, Fan, Chun-wei
committed Details | Review
build: Add Meson build option for relocatable builds (take ii) (1.70 KB, patch)
2017-08-17 07:31 UTC, Fan, Chun-wei
committed Details | Review
build: Allow 'all' shorthand for builtin_loaders build option (take ii) (2.71 KB, patch)
2017-08-17 07:32 UTC, Fan, Chun-wei
committed Details | Review
build: Find loader deps on MSVC with cc.has_header() and cc.find_library() as a fallback (6.72 KB, patch)
2017-08-17 09:15 UTC, Fan, Chun-wei
none Details | Review
build: Improve loader dependency discovery process on Visual Studio (6.81 KB, patch)
2017-08-18 07:16 UTC, Fan, Chun-wei
needs-work Details | Review
build: Improve loader dependency discovery process on Visual Studio (7.36 KB, patch)
2017-08-21 08:18 UTC, Fan, Chun-wei
none Details | Review
build: Allow native Windows image loaders to build in Meson builds (7.37 KB, patch)
2017-08-21 10:25 UTC, Fan, Chun-wei
none Details | Review
build: Allow native Windows image loaders to build in Meson builds (take ii) (7.38 KB, patch)
2017-08-21 15:48 UTC, Fan, Chun-wei
none Details | Review
build: Improve jpeg library detection on Meson/MSVC builds (1.34 KB, patch)
2017-08-23 07:06 UTC, Fan, Chun-wei
committed Details | Review
build: Fix libjasper detection/usage on Meson/MSVC builds (3.26 KB, patch)
2017-08-23 07:09 UTC, Fan, Chun-wei
none Details | Review
build: Add fallback dependency detection on Meson/MSVC builds (3.40 KB, patch)
2017-08-23 07:13 UTC, Fan, Chun-wei
none Details | Review
build: Allow native Windows image loaders to build in Meson builds (take iii) (8.17 KB, patch)
2017-08-24 08:40 UTC, Fan, Chun-wei
committed Details | Review
build: Fix libjasper detection/usage on Meson/MSVC builds (take ii) (3.95 KB, patch)
2017-08-24 08:55 UTC, Fan, Chun-wei
committed Details | Review
build: Add fallback dependency detection on Meson/MSVC builds (take ii) (3.26 KB, patch)
2017-09-07 10:44 UTC, Fan, Chun-wei
committed Details | Review

Description Fan, Chun-wei 2017-08-03 10:09:43 UTC
Hi,

Recently there is an addition of a Meson build system, which is intended to replace the autotools builds in the longrun.  However, there are some things that will need improvements before such builds are usable on Windows, which this bug will try to address.

I will attach patches for this purpose shortly.

With blessings, thank you!
Comment 1 Fan, Chun-wei 2017-08-03 10:15:33 UTC
Created attachment 356833 [details] [review]
meson: Check for sys/time.h and sys/resources.h

Hi,

First up is the patch to check for more system headers, which may not be universally shipped with the compiler toolset, namely sys/time.h and sys/resource.h.  Also update the tests not to include sys/time.h unconditionally as it may not be available.

There may be others around that we want to check, but I think we can leave them for other patches to address.

With blessings, thank you!
Comment 2 Fan, Chun-wei 2017-08-03 10:19:56 UTC
Created attachment 356834 [details] [review]
gen-thumbnailer.py: Fix running on Windows.

Hi,

The MSVC linker (not sure about MinGW/mingw-w64) does not support things like rpath, so we need to prepend PATH so that the main GDK-Pixbuf DLL we just built can be loaded when we run this (wrapper) script during the build.

With blessings, thank you!
Comment 3 Fan, Chun-wei 2017-08-03 10:22:10 UTC
Created attachment 356835 [details] [review]
meson: Fix build of built-in loaders

Hi,

The -DINCLUDE_xxx macros were not correctly applied due to a typo, so correct it and apply it one-by-one to each built-in loader in addition to applying it to the CFLAGS of GDK-Pixbuf (et al).

With blessings, thank you!
Comment 4 Fan, Chun-wei 2017-08-03 10:28:16 UTC
Created attachment 356836 [details] [review]
meson: Add option for relocatablilty

Hi,

In the autotools builds there is an option for relocatability so that we can do builds where the library (and loaders) can be moved from their installed locations.  Add such an option, and always enable it for Windows since it is normally the case that the GDK-Pixbuf (and related) DLLs are not residing in the locations where the build is being done.

With these patches I was able to get GDK-Pixbuf to build on Visual Studio 2015, but the built binaries are not yet OK as they crash with
---
(gtk3-demo.exe:2588): GLib-GObject-WARNING **: cannot register existing type 'GdkPixbuf'

(gtk3-demo.exe:2588): GLib-GObject-CRITICAL **: g_type_add_interface_static: assertion 'G_TYPE_IS_INSTANTIATABLE (instance_type)' failed

(gtk3-demo.exe:2588): GLib-GObject-CRITICAL **: g_type_add_interface_static: assertion 'G_TYPE_IS_INSTANTIATABLE (instance_type)' failed

(gtk3-demo.exe:2588): GLib-CRITICAL **: g_once_init_leave: assertion 'result != 0' failed

(gtk3-demo.exe:2588): GLib-GObject-CRITICAL **: g_object_new_valist: assertion 'G_TYPE_IS_OBJECT (object_type)' failed
---

So, I need to look into this further.  Besides, the other things remaining include:

-Check for rint()/round() (unless we are going to make this C99 in this cycle)
-Link in the .rc file on Windows, so that we get the version info for the
 GDK-Pixbuf DLL.

With blessings, thank you!
Comment 5 Fan, Chun-wei 2017-08-03 10:57:44 UTC
Hi,

Sorry, please scratch out this part:

(In reply to Fan, Chun-wei from comment #4)
> With these patches I was able to get GDK-Pixbuf to build on Visual Studio
> 2015, but the built binaries are not yet OK as they crash with
> ---
> (gtk3-demo.exe:2588): GLib-GObject-WARNING **: cannot register existing type
> 'GdkPixbuf'
> 
> (gtk3-demo.exe:2588): GLib-GObject-CRITICAL **: g_type_add_interface_static:
> assertion 'G_TYPE_IS_INSTANTIATABLE (instance_type)' failed
> 
> (gtk3-demo.exe:2588): GLib-GObject-CRITICAL **: g_type_add_interface_static:
> assertion 'G_TYPE_IS_INSTANTIATABLE (instance_type)' failed
> 
> (gtk3-demo.exe:2588): GLib-CRITICAL **: g_once_init_leave: assertion 'result
> != 0' failed
> 
> (gtk3-demo.exe:2588): GLib-GObject-CRITICAL **: g_object_new_valist:
> assertion 'G_TYPE_IS_OBJECT (object_type)' failed
> ---
> 
> So, I need to look into this further...

It turns out that I needed to re-build librsvg with the rsvg pixbuf loader.  DOH! :|

So, the gdk-pixbuf build with Meson is working fine.

With blessings, thank you!
Comment 6 Fan, Chun-wei 2017-08-04 06:49:45 UTC
Created attachment 356911 [details] [review]
gen-thumbnailer.py: Fix running on Windows (take ii)

Hi,

Re-basing the gen-thumbnailer.py patch due to its move to build-aux/...
Comment 7 Fan, Chun-wei 2017-08-04 06:52:40 UTC
Created attachment 356912 [details] [review]
meson: Add a buildin_all_loaders configuration option

Hi,

This adds a configuration option to build all the loaders (that can be built) shipped with GDK-Pixbuf into the main GDK-Pixbuf library/DLL, to simplify the process where people might want to do so to ease packaging, instead of finding out the names of all the loaders and passing them in one-by-one.

With blessings, thank you!
Comment 8 Fan, Chun-wei 2017-08-04 07:01:07 UTC
Created attachment 356913 [details] [review]
meson: Force-include msvc_recommended_pragmas.h on Visual Studio

Hi,

We can clean things up a little bit on Visual Studio builds by force-including msvc_recommended_pragmas.h, since we are always building against GLib.

With blessings, thank you!
Comment 9 Fan, Chun-wei 2017-08-04 07:23:37 UTC
Created attachment 356915 [details] [review]
meson: Check for lrint() and round()

Hi,

This checks for lrint() and round() instead of hardcoding their (un-)availability depending on compiler, since Visual Studio 2013 and later do provide these functions.

With blessings, thank you!
Comment 10 Fan, Chun-wei 2017-08-04 07:56:02 UTC
Created attachment 356917 [details] [review]
meson: Build the resource (.rc) file on Windows

Hi,

The MSVC builds and autotools/Windows builds both build the .rc file into the main GDK-Pixbuf DLL, so do likewise on Meson builds, so that people can determine the GDK-Pixbuf DLL version more easily.

With blessings, thank you!
Comment 11 Fan, Chun-wei 2017-08-04 07:57:03 UTC
Created attachment 356918 [details] [review]
tests/pixbuf-short-gif-write.c: Fix on pre-C99 compilers

Hi,

This fixes the tests/pixbuf-short-gif-write.c test program to build on pre-C99 compilers.

With blessings, thank you!
Comment 12 Daniel Boles 2017-08-05 23:58:17 UTC
Review of attachment 356918 [details] [review]:

Again, this should be an insta-commit, however blank lines between the declarations and the start of following statements are conventional.
Comment 13 Fan, Chun-wei 2017-08-07 04:28:11 UTC
Created attachment 357079 [details] [review]
tests/pixbuf-short-gif-write.c: Fix on pre-C99 compilers (take ii)

Hi Daniel,

Thanks for the notes, fixed with your suggestions on the formatting.

With blessings, thank you!
Comment 14 Bastien Nocera 2017-08-14 18:13:50 UTC
Review of attachment 356833 [details] [review]:

Could you please split this into 2 patches? One to re-add the sys/resource.h check that's in configure.ac, another one to add the sys/time.h check.
Comment 15 Bastien Nocera 2017-08-14 18:17:10 UTC
Review of attachment 356835 [details] [review]:

The commit subject prefix is just "build: ". The patch could do with some more in-depth explanations.
Comment 16 Bastien Nocera 2017-08-14 18:18:21 UTC
Review of attachment 356836 [details] [review]:

Any reason why this should be an option, and not always enabled on Windows?
Comment 17 Bastien Nocera 2017-08-14 18:20:28 UTC
Review of attachment 356911 [details] [review]:

Again, should be "build: " as a subject prefix.

::: build-aux/gen-thumbnailer.py
@@ +25,3 @@
+if os.name == 'nt':
+    gdk_pixbuf_dll_buildpath = os.path.dirname(args.pixdata)
+    newenv['PATH'] = gdk_pixbuf_dll_buildpath + os.pathsep + newenv['PATH']

+= to avoid the "+ newenv['PATH']" at the end?
Comment 18 Bastien Nocera 2017-08-14 18:23:25 UTC
Review of attachment 356912 [details] [review]:

The word is "built-in". You can also write "builtin" in code.

::: meson_options.txt
@@ +36,3 @@
        value: false)
+option('buildin_all_loaders',
+       description: 'Whether to build in all buildable loaders',

Whether to include all selected loaders in the main gdk-pixbuf library
Comment 19 Bastien Nocera 2017-08-14 18:24:53 UTC
Review of attachment 356913 [details] [review]:

Subject prefix again.

Can you also link to equivalent GLib code?
Comment 20 Bastien Nocera 2017-08-14 18:29:06 UTC
Review of attachment 356915 [details] [review]:

Commit subject prefix.

::: meson.build
@@ +81,3 @@
+]
+
+foreach func : check_functions

Is the for loop really necessary? Please unroll this into 2 times 3 lines of code.
Comment 21 Bastien Nocera 2017-08-14 18:31:14 UTC
Review of attachment 356917 [details] [review]:

Commit subject prefix.

::: gdk-pixbuf/meson.build
@@ +43,3 @@
 gdkpixbuf_features_conf.set('GDK_PIXBUF_VERSION', meson.project_version())
+gdkpixbuf_features_conf.set('GDK_PIXBUF_API_VERSION', gdk_pixbuf_api_version)
+gdkpixbuf_features_conf.set('LT_CURRENT_MINUS_AGE', '0')

That should be the same calculation as in configure.ac:
  84 LT_CURRENT_MINUS_AGE=m4_eval(lt_current - lt_age)•

@@ +90,3 @@
 gdkpixbuf_enum_h = gdkpixbuf_enums[1]
 
+if host_system == 'windows'

Is the check for windows, or MSVC? Is it needed for msys?
Comment 22 Bastien Nocera 2017-08-14 18:31:57 UTC
Review of attachment 357079 [details] [review]:

"tests: "

::: tests/pixbuf-short-gif-write.c
@@ +39,2 @@
     GIOChannel* channel = g_io_channel_new_file (g_test_get_filename (G_TEST_DIST, "test-animation.gif", NULL), "r", NULL);
+

White space change here.
Comment 23 Fan, Chun-wei 2017-08-15 02:08:05 UTC
Created attachment 357587 [details] [review]
build: Check for sys/resource.h in Meson

Hi,

This is the patch for the Meson build files to check for sys/resource.h as we did in the autools builds...
Comment 24 Fan, Chun-wei 2017-08-15 02:09:06 UTC
Created attachment 357588 [details] [review]
build: Check for sys/time.h

Hi,

This is the patch to check for sys/time.h in the autotools builds as well as the Meson builds, and to not include it unconditionally as it is not shipped with all the compilers that we support...
Comment 25 Fan, Chun-wei 2017-08-15 02:11:38 UTC
Created attachment 357589 [details] [review]
build: Fix gen-thumbnailer.py on Windows

Hi Bastien,

If I am not mistaken '+=' means appending, but we need to prepend PATH, so that the GDK-Pixbuf DLL that we just built is found first.  I fixed the commit comments though.

With blessings, thank you!
Comment 26 Fan, Chun-wei 2017-08-15 02:12:50 UTC
Created attachment 357590 [details] [review]
build: Fix build of built-in loaders

Hi Bastien,

Hopefully the commit comments for this is now clearer.

With blessings, thank you!
Comment 27 Fan, Chun-wei 2017-08-15 02:16:21 UTC
Created attachment 357591 [details] [review]
build: Add Mesib build option for relocatable builds

Hi Bastien,

It does seem to me in the code that relocatable builds are not just for Windows only, but other platforms as well[1], so hence this option.  On Windows however, this is always enabled.

[1]: Please see gdk-pixbuf/gdk-pixbuf-io.c

With blessings, thank you!
Comment 28 Fan, Chun-wei 2017-08-15 02:18:12 UTC
Created attachment 357592 [details] [review]
build: Allow 'all' shorthand for builtin_loaders build option

Hi,

I decided that instead of adding a build option, we could just allow an 'all' shorthand for the builtin_loaders build option to do the same thing...
Comment 29 Fan, Chun-wei 2017-08-15 02:19:19 UTC
Created attachment 357594 [details] [review]
build: Force-include msvc_recommended_pragmas.h

Hi Bastien,

Did I understand you correctly about this part?

With blessings, thank you!
Comment 30 Fan, Chun-wei 2017-08-15 02:20:18 UTC
Created attachment 357595 [details] [review]
build: Check for lrint() and round()

Hi,

This is the new non-loop patch for checking for round() and lrint()...
Comment 31 Fan, Chun-wei 2017-08-15 02:23:23 UTC
Created attachment 357596 [details] [review]
build Build the .rc files on Windows

Hi Bastien,

I hope I understood you correctly as well here.  Note that this applies to MSYS builds as well (please see the autotools builds), i.e. all Windows builds.

With blessings, thank you!
Comment 32 Fan, Chun-wei 2017-08-15 02:24:15 UTC
Created attachment 357597 [details] [review]
tests: Fix build on pre-C99

Hi,

This is the updated patch comments about fixing pre-C99 builds of the test program.

With blessings, thank you!
Comment 33 Bastien Nocera 2017-08-16 11:31:26 UTC
Review of attachment 357587 [details] [review]:

Yes.
Comment 34 Bastien Nocera 2017-08-16 11:36:34 UTC
Review of attachment 357588 [details] [review]:

Sure.
Comment 35 Bastien Nocera 2017-08-16 11:39:33 UTC
Review of attachment 357589 [details] [review]:

::: build-aux/gen-thumbnailer.py
@@ +23,3 @@
 newenv['GDK_PIXBUF_PIXDATA'] = args.pixdata
 newenv['GDK_PIXBUF_MODULE_FILE'] = args.loaders
+if os.name == 'nt':

Add a comment above this that explains that "nt" is Windows from version XX and above.
Comment 36 Bastien Nocera 2017-08-16 11:40:27 UTC
Review of attachment 357590 [details] [review]:

Alright.
Comment 37 Bastien Nocera 2017-08-16 11:46:49 UTC
Review of attachment 357591 [details] [review]:

::: meson.build
@@ +265,3 @@
+relocatable = get_option('enable_relocatable')
+if not relocatable
+  if host_system == 'windows'

if host_system == 'windows'
  relocatable = true
else
  relocatable = get_option(...
endif
Comment 38 Bastien Nocera 2017-08-16 11:49:45 UTC
Review of attachment 357592 [details] [review]:

::: meson.build
@@ +213,3 @@
 builtin_loaders = get_option('builtin_loaders').split(',')
 
+# If 'all' is specified for builtin_loaders, build all

You need to explain this in the option description as well.
Comment 39 Bastien Nocera 2017-08-16 11:51:04 UTC
Review of attachment 357594 [details] [review]:

Sure.
Comment 40 Bastien Nocera 2017-08-16 11:51:37 UTC
Review of attachment 357595 [details] [review]:

Looks good.
Comment 41 Bastien Nocera 2017-08-16 11:53:26 UTC
Review of attachment 357596 [details] [review]:

Looks good.
Comment 42 Bastien Nocera 2017-08-16 11:54:00 UTC
Review of attachment 357597 [details] [review]:

Yes.
Comment 43 Fan, Chun-wei 2017-08-17 07:31:12 UTC
Created attachment 357769 [details] [review]
build: Add Meson build option for relocatable builds (take ii)

Hi,

This is the new patch for enabling relocatable builds in Meson, which is cleaned up as suggested.

With blessings, thank you!
Comment 44 Fan, Chun-wei 2017-08-17 07:32:27 UTC
Created attachment 357771 [details] [review]
build: Allow 'all' shorthand for builtin_loaders build option (take ii)

Hi Bastien,

Thanks for catching this, indeed this description should be added.  Here's the new patch for it.

With blessings, thank you!
Comment 45 Fan, Chun-wei 2017-08-17 07:39:07 UTC
Hi,

For records, these patches have been pushed as follows:

(Attachment: first 7 chars of commit hash)
==========================================
357587: e041ecd
357588: dc85e7d
357589: 82f40a6 (With comment on 'nt' added, meaning NT-based Windows)
357590: 6ce0b0b
357594: 0b98f7e
357595: 480e483
357596: c1a8ca5
357597: 7eeab9f

With blessings, thank you!
Comment 46 Fan, Chun-wei 2017-08-17 09:15:39 UTC
Created attachment 357777 [details] [review]
build: Find loader deps on MSVC with cc.has_header() and cc.find_library() as a fallback

Hi,

This is to update the Meson build files to find libpng and libtiff using cc.has_header() and cc.find_library() on Visual Studio as a fallback as it is often the case that their pkg-config files are not available, since their build systems for Visual Studio does not generate them.  This also means if one is to use Meson's dependency() function to find these libraries, one must manually craft pkg-config files for them, which can be error-prone.

The .lib files that we want to look for for libpng and libtiff corresponds to the generated .lib files that libpng's Visual Studio project and libtiff's NMake Makefiles generate during the build.

There is also some improvements to how libjasper is looked for, but it will require Meson support to pass extra CFlags to cc.has_header() as Visual Studio builds using libjasper requires JAS_WIN_MSVC_BUILD to be defined for libjasper/libjasper.h to be included properly, but otherwise the other needed parts have been added for this.

Note that the GNOME dependencies have their pkg-config files generated via their Meson or Visual Studio build systems, so we don't need to add such fallbacks for them.

With blessings, thank you!
Comment 47 Fan, Chun-wei 2017-08-18 07:16:33 UTC
Created attachment 357864 [details] [review]
build: Improve loader dependency discovery process on Visual Studio

Hi,

It seems that we could use the prefix: '...' argument in has_header() to define JAS_WIN_MSVC_BUILD so that jasper/jasper.h can be included correctly on Visual Studio builds, so this is the updated patch for it.

With blessings, thank you!
Comment 48 Emmanuele Bassi (:ebassi) 2017-08-18 11:19:51 UTC
Review of attachment 357864 [details] [review]:

Thanks; looks generally good.

::: meson.build
@@ +224,3 @@
+        have_png_h = cc.has_header('png.h')
+        if cc.get_id() == 'msvc'
+          if have_png_h

These two conditions should be moved in together:

  if cc.get_id() == 'msvc' and have_png_h
    png_dep = cc.find_library(png, required: false)
  endif

@@ +244,3 @@
+        zlib_dep = cc.find_library('zlib', required: false)
+      endif
+        # If we still can't find libpng, try looking for the static libpng.lib,

This branch will happen outside of the 'msvc' check, which means it'll break elsewhere.

Additionally, it should all be gated through a msvc check, since a static libpng is definitely not common outside of Windows, and a non-pkg-config based detection should have succeeded in the check above.

  # If we couldn't find a shared libpng, look for a static one, with the
  # accompanying zlib
  if cc.get_id() == 'msvc' and have_png_h
    png_dep = cc.find.library('libpng', required: false)
    zlib_dep = cc.find_library('zlib', required: false)
    if png_dep.found() and zlib_dep.found()
      enabled_loaders += 'png'
      loaders_deps += [ png_dep, zlib_dep ]
    endif
  endif

@@ +271,3 @@
+    # Fallback when no pkg-config file is found for libtiff on MSVC, which is quite normal
+    if cc.has_header('tiff.h')
+      if cc.get_id() == 'msvc'

Same as above, these conditions should be coalesced and everything gated on msvc:

  if cc.get_id() == 'msvc' and cc.has_header('tiff.h')
    tiff_dep = cc.find_library('libtiff_i', required: false)
    if not tiff_dep
      tiff_dep = cc.find_library('libtiff', required: false)
    endif
  endif

@@ +273,3 @@
+      if cc.get_id() == 'msvc'
+        # First look for the DLL builds of libtiff, then the static builds
+	    tiff_dep = cc.find_library('libtiff_i', required: false)

Indentation.

@@ +290,3 @@
+jasper_extra_cflags = []
+
+          tiff_dep = cc.find_library('libtiff', required: false)

This block should go into the get_option('enable_jasper') check; only the `jasper_extra_cflags` declaration should go outside.
Comment 49 Emmanuele Bassi (:ebassi) 2017-08-18 11:20:45 UTC
Review of attachment 357769 [details] [review]:

Looks good
Comment 50 Emmanuele Bassi (:ebassi) 2017-08-18 11:22:45 UTC
Review of attachment 357771 [details] [review]:

Looks good

::: meson.build
@@ +207,3 @@
+# buildable loaders into gdk-pixbuf
+builtin_all_loaders = false
+if builtin_loaders == [ 'all' ]

This could also be:

  if builtin_loaders.contains('all')
Comment 51 Fan, Chun-wei 2017-08-21 08:13:30 UTC
Review of attachment 357769 [details] [review]:

Hi Emmanuele,

Thanks, I pushed the patch as 64ac9d7.

With blessings, thank you!
Comment 52 Fan, Chun-wei 2017-08-21 08:15:23 UTC
Review of attachment 357771 [details] [review]:

Hi Emmanuele,

Thanks, I pushed the patch as-is as 98ebded, since I think the 'all' should stand out on its own, so that people are clear about using it. :)

With blessings, thank you!
Comment 53 Fan, Chun-wei 2017-08-21 08:18:02 UTC
Created attachment 358056 [details] [review]
build: Improve loader dependency discovery process on Visual Studio

Hi Emmanuele

(In reply to Emmanuele Bassi (:ebassi) from comment #48)
> Review of attachment 357864 [details] [review] [review]:
> 
> This branch will happen outside of the 'msvc' check, which means it'll break
> elsewhere.

Oops, I missed this...

Here comes the new patch that should cover the points you mentioned.

With blessings, thank you!
Comment 54 Fan, Chun-wei 2017-08-21 10:25:32 UTC
Created attachment 358066 [details] [review]
build: Allow native Windows image loaders to build in Meson builds

Hi,

There is a GDI+-based image loader that deals with BMP, EMF, ICO, GIF, JPEG, TIFF and WMF images, which helps to reduce dependencies of the loaders, namely libjpeg-turbo/IJG JPEG and libtiff.  This will serve as a basis for improvements or for moving on to more modern Windows APIs/Frameworks such as WIC (Windows Imaging Component).

Note that this loader does not support PNG (due to limitations in its support in GDI+) and JPEG2000, so support for these formats are still done through libpng and libjasper respectively.  Also, the open-source libraries (and in-house support) still provide better support for formats such as JPEG, TIFF and ICO (so hence ANI), so this can serve as a base for improvements, although this is the way that EMF and WMF images get supported by GDK-Pixbuf without outside modules on Windows.

With blessings, thank you!
Comment 55 Emmanuele Bassi (:ebassi) 2017-08-21 10:45:48 UTC
Review of attachment 358066 [details] [review]:

Thanks for working on this; looks generally okay to me.

::: gdk-pixbuf/meson.build
@@ +134,3 @@
+# Build the loaders using native Windows components as static modules, if requested
+if enable_native_windows_loaders
+  builtin_loaders.contains('windows') or builtin_all_loaders

I think this is missing an if/endif block

::: meson.build
@@ +265,3 @@
+    use_libjpeg = false
+    use_libtiff = false
+  # Disable the libjpeg-turbo/IJG JPEG-based and libtiff loaders when using Windows system components

Style: indendation
Comment 56 Fan, Chun-wei 2017-08-21 15:48:13 UTC
Created attachment 358084 [details] [review]
build: Allow native Windows image loaders to build in Meson builds (take ii)

Hi Emmanuele,

Thanks for the review!  So here's the new patch that addressed the issues.  I will look into the part tomorrow where gdk-pixbuf-query-loaders should be run to create the loaders.cache file when we have loaders built here as DLLs on Windows, since somehow that was not done right.

(In reply to Emmanuele Bassi (:ebassi) from comment #55)
> Review of attachment 358066 [details] [review] [review]:
> ::: gdk-pixbuf/meson.build
> I think this is missing an if/endif block

Oops, clearly it was missing... doh.

With blessings, thank you!
Comment 57 Bastien Nocera 2017-08-22 12:07:26 UTC
Review of attachment 358056 [details] [review]:

> Also fix and improve the libjasper detection on Visual Studio:

Please split those patches up. There's no need to bundle all those changes into one patch, they fix separate bugs.
Comment 58 Bastien Nocera 2017-08-22 12:15:31 UTC
Review of attachment 358084 [details] [review]:

This is getting far too complicated for its own good.

I think that the easiest way to fix this would be to replace the enable_png, enable_tiff, enable_jpeg and enable_jasper with a single "loaders" options, and have each "gdi" loader have its own name, even if they're all built in the same .so. You'd pass something like "png,tiff,jpeg,gdiplus_bmp" to that option.
Comment 59 Fan, Chun-wei 2017-08-22 16:17:23 UTC
Hi Bastien,

I'm afraid I have to disagree with you with some parts of this, please pardon my ignorance if it does come to you, so please bear with me...

(In reply to Bastien Nocera from comment #58)
> Review of attachment 358084 [details] [review] [review]:
> 
> I think that the easiest way to fix this would be to replace the enable_png,
> enable_tiff, enable_jpeg and enable_jasper with a single "loaders" options,
> and have each "gdi" loader have its own name, even if they're all built in
> the same .so. You'd pass something like "png,tiff,jpeg,gdiplus_bmp" to that
> option.

This is not going to work, IMHO, unless we change gdk-pixbuf-io.c and most probably some other things, which will make the situation much more complex, at least when we are building the GDI+ loaders into the main GDK-Pixbuf DLL (since this will always be about Windows).

The current GDI+ loader is an "all-or-nothing" approach, so if we enable that, it will cover every format that loader will cover, i.e. BMP, EMF, ICO, JPEG, TIFF, WMF and GIF.  There is no INCLUDE_gdiplus_xxx macro, but only INCLUDE_gdiplus for instance; and the autotools build system, when building the GDI+ loaders as a dynamic module builds all the GDI+ loader DLLs when one selects to build the GDI+ loaders.

Note also that the autotools builds (and so MSVC projects) will block out any loaders that use an in-house implementation or using an opensource library for any format that is covered by the GDI+ loader, so I don't think we want to do different things here as a result--the enable_jpeg and enable_tiff options are by default enabled, and we want to disable them when we enable the GDI+ loaders so that we don't need to spend time configuring for them, since they aren't used.

Plus, if we try to enable that and still enable the in-house or opensource library-using loaders for these formats that are covered by the GDI+, we get multiple calls of the same load_one_builtin_module (<format>) calls in gdk_pixbuf_io_init(), which probably is not good news, when we try to build in the loaders into GDK-Pixbuf.

Once again, the GDI+ loaders are meant to be used as a basis to reduce dependencies, or as a base to improve the loader there or move on to more modern Windows APIs for image loading and handling (such as WIC), and so, this is made to be an option on Windows builds, *not* the default, since the in-house/opensource library-using loaders tend to work better at this point.

Since GDK-Pixbuf without the JPEG, PNG and TIFF loaders in any form will probably not be useful, at least in the case of using GTK+, I think it would be better to leave them enabled by default, and adjust accordingly when the GDI+ loader is enabled.

Please let me know if there are any other suggestions on this regard, and thanks for bearing with me--I will re-visit the issues you mentioned in the last two comments tomorrow, since it is quite late here in this part of the world.

Thanks though, with blessings!
Comment 60 Fan, Chun-wei 2017-08-23 07:06:56 UTC
Created attachment 358207 [details] [review]
build: Improve jpeg library detection on Meson/MSVC builds

Hi,

As suggested, this is the patch to improve jpeg library detection on Meson/MSVC builds, as one could be using either libjpeg-turbo (jpeg.lib) or the IJG JPEG library (libjpeg.lib)

With blessings, thank you!
Comment 61 Fan, Chun-wei 2017-08-23 07:09:23 UTC
Created attachment 358208 [details] [review]
build: Fix libjasper detection/usage on Meson/MSVC builds

Hi,

This is the patch to fix the detection of jasper/jasper.h as well as building io-jasper.c on Visual Studio with Meson, as we need to ensure JAS_WIN_MSVC_BUILD is defined.

With blessings, thank you!
Comment 62 Fan, Chun-wei 2017-08-23 07:13:06 UTC
Created attachment 358209 [details] [review]
build: Add fallback dependency detection on Meson/MSVC builds

Hi,

libpng and libtiff ship build systems for Visual Studio, but they do not generate the pkg-config files that is needed by Meson to do xxx_dep = depedency(...), so we need to find those dependencies manually using cc.has_header() and cc.find_library() on Visual Studio in those cases where their pkg-config files cannot be found.  This will also avoid the need for one to craft pkg-config files for them, which can be error-prone.

For both libraries, we will look for the DLL + import .lib builds before trying the static .lib builds, which these libraries both support building.

With blesings, thank you!
Comment 63 Fan, Chun-wei 2017-08-24 08:40:28 UTC
Created attachment 358300 [details] [review]
build: Allow native Windows image loaders to build in Meson builds (take iii)

Hi,

This is the slightly-revised patch for the option to enable the GDI+ loader for Windows build, which hopefully makes the situation clearer.  The key thing about this option is that if it is used, the GDI+ loaders will be built to handle BMP, EMF, GIF, ICO, JPEG, TIFF and WMF images, and disabling their in-house (or opensource library-using) counterparts, if any, which is the same thing that is done in the autotools (and Visual Studio projects) builds.

Since the GDI+ loaders is done in an "all-or-nothing" approach, this set of GDI+ loaders has to be taken as a full package, not individually, plus the GDI+ loaders share the same MIME types, signatures and extensions with their respective counterparts that are done in-house or using opensource libraries.

Please let me know whether there are any ways to simplify this, as this is AFAIK the furthest I could do with this, given that this should strictly be an option as builds without the GDI+ loaders currently work better at this point.

With blessings, thank you!
Comment 64 Fan, Chun-wei 2017-08-24 08:55:50 UTC
Created attachment 358301 [details] [review]
build: Fix libjasper detection/usage on Meson/MSVC builds (take ii)

Hi,

I am re-uploading the patch for libjasper detection/usage on MSVC builds as I forgot to apply the needed cflag (-DJAS_WIN_MSVC_BUILD) when building io-jasper.c as a dynamic loader DLL module.

With blessings, thank you!

p.s. Note that this patch will come before attachments 358209 and 358300.
Comment 65 Ignacio Casal Quinteiro (nacho) 2017-09-07 07:58:18 UTC
Review of attachment 358301 [details] [review]:

See the comment though

::: meson.build
@@ -257,1 +259,4 @@
-  if cc.has_header('jasper/jasper.h')
+  has_jasper_header = false
+
+  if cc.get_id() == 'msvc'
+    # We must define JAS_WIN_MSVC_BUILD before including jasper/jasper.h on MSVC

out of curiosity, why?
Comment 66 Ignacio Casal Quinteiro (nacho) 2017-09-07 08:03:03 UTC
Review of attachment 358207 [details] [review]:

Fine for me
Comment 67 Ignacio Casal Quinteiro (nacho) 2017-09-07 08:18:44 UTC
Review of attachment 358300 [details] [review]:

Fine for me, just remove the empty line.

::: gdk-pixbuf/meson.build
@@ +23,3 @@
   [ 'jasper', [ 'io-jasper.c' ], enabled_loaders.contains('jasper'), jasper_extra_cflags ],
   [ 'qtif', [ 'io-qtif.c' ] ]
+

no need for this line
Comment 68 Fan, Chun-wei 2017-09-07 10:26:08 UTC
Hi Nacho,

Thanks, the patches were pushed as:
-attachment 358207 [details] [review]: 10883be
-attachment 358300 [details] [review]: 7386e44 (removed extra line; please see notes below)
-attachment 358301 [details] [review]: 8595819 (please see notes below)

For libjasper, especially when we want to support builds using older Visual Studio (pre-C99), the jasper/jasper_config.h will include jasper/jas_config2.h, which is essentially a predefined jasper/jasper_config.h meant for Visual Studio builds when JAS_WIN_MSVC_BUILD is defined, so that is why that macro is required.  For the libjasper header test, Meson does a small test program to see whether jasper/jasper.h can be properly included, and that is where that macro comes in as well; otherwise on Visual Studio libjasper cannot be properly detected (and the loader cannot be properly built).

Since attachment 358300 [details] [review] is pushed before 358209 (for external libraries out of our control where we don't have .pc files for them in their MSVC builds), the final pushed patch has to accomodate for that, for records.

With blessings, thank you!
Comment 69 Fan, Chun-wei 2017-09-07 10:44:18 UTC
Created attachment 359331 [details] [review]
build: Add fallback dependency detection on Meson/MSVC builds (take ii)

Hi,

I am re-uploading this patch as it has to be rebased.

With blessings, thank you!
Comment 70 Ignacio Casal Quinteiro (nacho) 2017-09-08 08:16:54 UTC
Review of attachment 359331 [details] [review]:

Finally got some time to review this. It looks good to me.
Comment 71 Fan, Chun-wei 2017-09-08 08:26:32 UTC
Review of attachment 359331 [details] [review]:

Hi Nacho,

Thanks a bunch, GDK-Pixbuf is now fully buildable out of the box with Meson using Visual Studio 2010+.  Patch was pushed as cf2f77c.

I will close this bug shortly.

With blessings, thank you!
Comment 72 Fan, Chun-wei 2017-09-08 08:28:21 UTC
Review of attachment 359331 [details] [review]:

Hi Nacho,

Thanks a bunch, GDK-Pixbuf is now fully buildable out of the box with Meson using Visual Studio 2010+.  Patch was pushed as cf2f77c.

I will close this bug shortly.

With blessings, thank you!
Comment 73 Fan, Chun-wei 2017-09-08 08:30:48 UTC
Review of attachment 359331 [details] [review]:

Hi Nacho,

Thanks a bunch, GDK-Pixbuf is now fully buildable out of the box with Meson using Visual Studio 2010+.  Patch was pushed as cf2f77c.

I will close this bug shortly.

With blessings, thank you!