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 784995 - meson: some Windows improvements
meson: some Windows improvements
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: build
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks: 790954
 
 
Reported: 2017-07-16 11:07 UTC by Christoph Reiter (lazka)
Modified: 2018-05-20 08:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
meson: error out in case of a static build (780 bytes, patch)
2017-07-16 11:08 UTC, Christoph Reiter (lazka)
none Details | Review
meson: build windows resource files (7.70 KB, patch)
2017-07-16 11:08 UTC, Christoph Reiter (lazka)
none Details | Review
meson: build windows resource files (7.68 KB, patch)
2017-07-16 13:03 UTC, Christoph Reiter (lazka)
none Details | Review
meson: Fix static build under Windows (5.04 KB, patch)
2017-07-16 14:43 UTC, Christoph Reiter (lazka)
none Details | Review
meson: define G_PID_FORMAT (1.42 KB, patch)
2017-07-17 16:36 UTC, Christoph Reiter (lazka)
committed Details | Review
meson: use set_quoted() instead of quoting manually (9.12 KB, patch)
2017-07-17 19:32 UTC, Christoph Reiter (lazka)
committed Details | Review
meson: set glib_extension in glibconfig.h to match the autotools output (1.18 KB, patch)
2017-07-17 19:42 UTC, Christoph Reiter (lazka)
committed Details | Review
meson: Add 'with-threads' option to allow building with posix threads on Windows (2.35 KB, patch)
2017-07-18 12:36 UTC, Christoph Reiter (lazka)
none Details | Review
meson: Define GLIB_EXTRA_CFLAGS and G_LIBS_EXTRA (1.50 KB, patch)
2017-07-19 14:36 UTC, Christoph Reiter (lazka)
none Details | Review
meson: Define GLIB_EXTRA_CFLAGS and G_LIBS_EXTRA (1.50 KB, patch)
2017-07-19 15:18 UTC, Christoph Reiter (lazka)
none Details | Review
meson: Set PCRE_REQUIRES (1019 bytes, patch)
2017-07-20 11:45 UTC, Christoph Reiter (lazka)
none Details | Review
meson: build windows resource files (take ii) (8.17 KB, patch)
2017-07-25 08:08 UTC, Fan, Chun-wei
none Details | Review
meson: build Windows resource files (8.18 KB, patch)
2017-12-14 13:34 UTC, Christoph Reiter (lazka)
none Details | Review
meson: fix static build under Windows (5.35 KB, patch)
2017-12-14 13:34 UTC, Christoph Reiter (lazka)
committed Details | Review
meson: add 'threads_implementation' option to allow building with posix threads on Windows (2.79 KB, patch)
2017-12-14 13:35 UTC, Christoph Reiter (lazka)
none Details | Review
meson: define GLIB_EXTRA_CFLAGS and G_LIBS_EXTRA on Windows (1.56 KB, patch)
2017-12-14 13:35 UTC, Christoph Reiter (lazka)
none Details | Review
meson: add 'threads_implementation' option to allow building with posix threads on Windows (3.14 KB, patch)
2018-01-02 20:05 UTC, Christoph Reiter (lazka)
none Details | Review
meson: define GLIB_EXTRA_CFLAGS and G_LIBS_EXTRA on Windows (1.49 KB, patch)
2018-01-02 20:05 UTC, Christoph Reiter (lazka)
none Details | Review
meson: build Windows resource files (8.32 KB, patch)
2018-01-04 21:08 UTC, Christoph Reiter (lazka)
committed Details | Review
meson: add 'force_posix_threads' option to allow building with posix threads on Windows (3.11 KB, patch)
2018-01-07 08:14 UTC, Christoph Reiter (lazka)
none Details | Review
meson: define GLIB_EXTRA_CFLAGS and G_LIBS_EXTRA on Windows (2.20 KB, patch)
2018-01-07 08:17 UTC, Christoph Reiter (lazka)
none Details | Review
Meson: add 'force_posix_threads' option (2.73 KB, patch)
2018-05-15 19:21 UTC, Xavier Claessens
none Details | Review
Meson: Remove FIXME about COCOA_LIBS and CARBON_LIBS (1.29 KB, patch)
2018-05-15 19:21 UTC, Xavier Claessens
committed Details | Review
Meson: Add missing flags on Windows (2.75 KB, patch)
2018-05-15 19:21 UTC, Xavier Claessens
committed Details | Review
Meson: add 'force_posix_threads' option (2.80 KB, patch)
2018-05-16 14:24 UTC, Xavier Claessens
committed Details | Review

Description Christoph Reiter (lazka) 2017-07-16 11:07:46 UTC
My first time using meson, so I might be missing things..
Comment 1 Christoph Reiter (lazka) 2017-07-16 11:08:15 UTC
Created attachment 355701 [details] [review]
meson: error out in case of a static build

glib currently uses shared_library() everywhere and doesn't
produce any static libs. This error makes clearer that static
currently isn't supported.
Comment 2 Christoph Reiter (lazka) 2017-07-16 11:08:52 UTC
Created attachment 355702 [details] [review]
meson: build windows resource files

configure_file() forces utf-8 atm but .rc files are not utf-8.
To work around the issue just remove the only non-ASCII char.
Comment 3 Emmanuele Bassi (:ebassi) 2017-07-16 12:43:31 UTC
Review of attachment 355702 [details] [review]:

::: gio/gio.rc.in
@@ +18,3 @@
 	VALUE "FileVersion", "@GLIB_VERSION@.0"
 	VALUE "InternalName", "libgio-2.0-@LT_CURRENT_MINUS_AGE@"
+	VALUE "LegalCopyright", "Copyright (c) 2006-2011 Red Hat, Inc. and others."

Just remove (c) and/or ©. It means "copyright", so the line would read "Copyright copyright".

::: glib/glib.rc.in
@@ +18,3 @@
 	VALUE "FileVersion", "@GLIB_VERSION@.0"
 	VALUE "InternalName", "libglib-2.0-@LT_CURRENT_MINUS_AGE@"
+	VALUE "LegalCopyright", "Copyright (c) 1995-2011 Peter Mattis, Spencer Kimball, Josh MacDonald and others."

Same as above

::: gmodule/gmodule.rc.in
@@ +18,3 @@
 	VALUE "FileVersion", "@GLIB_VERSION@.0"
 	VALUE "InternalName", "libgmodule-2.0-@LT_CURRENT_MINUS_AGE@"
+	VALUE "LegalCopyright", "Copyright (c) 1998-2011 Tim Janik and others."

Same as above.

::: gobject/gobject.rc.in
@@ +21,1 @@
 	VALUE "OriginalFilename", "libgobject-2.0-@LT_CURRENT_MINUS_AGE@.dll"

Same as above.

::: gthread/meson.build
@@ +5,3 @@
+if host_system == 'windows'
+  gthread_win_rc = configure_file(
+    input: 'gthread.rc.in',

Do we actually need this, still?

GLib includes all the GThread API, these days, and it requires all threading support. The shared library is still there for backward compatibility reasons.
Comment 4 Emmanuele Bassi (:ebassi) 2017-07-16 12:44:55 UTC
Review of attachment 355701 [details] [review]:

I'd be okay with this. Or, alternatively, to replace all `shared_library()` uses with `library()`, which does the right thing depending on `--default-library`.

Nevertheless, nobody is actually testing GLib's functionality with static compilation, so at the very least it'd warrant a message().
Comment 5 Christoph Reiter (lazka) 2017-07-16 12:55:42 UTC
(In reply to Emmanuele Bassi (:ebassi) from comment #3)
> Review of attachment 355702 [details] [review] [review]:
> 
> ::: gio/gio.rc.in
> @@ +18,3 @@
>  	VALUE "FileVersion", "@GLIB_VERSION@.0"
>  	VALUE "InternalName", "libgio-2.0-@LT_CURRENT_MINUS_AGE@"
> +	VALUE "LegalCopyright", "Copyright (c) 2006-2011 Red Hat, Inc. and others."
> 
> Just remove (c) and/or ©. It means "copyright", so the line would read
> "Copyright copyright".
> 
> ::: glib/glib.rc.in
> @@ +18,3 @@
>  	VALUE "FileVersion", "@GLIB_VERSION@.0"
>  	VALUE "InternalName", "libglib-2.0-@LT_CURRENT_MINUS_AGE@"
> +	VALUE "LegalCopyright", "Copyright (c) 1995-2011 Peter Mattis, Spencer
> Kimball, Josh MacDonald and others."
> 
> Same as above
> 
> ::: gmodule/gmodule.rc.in
> @@ +18,3 @@
>  	VALUE "FileVersion", "@GLIB_VERSION@.0"
>  	VALUE "InternalName", "libgmodule-2.0-@LT_CURRENT_MINUS_AGE@"
> +	VALUE "LegalCopyright", "Copyright (c) 1998-2011 Tim Janik and others."
> 
> Same as above.
> 
> ::: gobject/gobject.rc.in
> @@ +21,1 @@
>  	VALUE "OriginalFilename", "libgobject-2.0-@LT_CURRENT_MINUS_AGE@.dll"
> 
> Same as above.

ok, thanks

> ::: gthread/meson.build
> @@ +5,3 @@
> +if host_system == 'windows'
> +  gthread_win_rc = configure_file(
> +    input: 'gthread.rc.in',
> 
> Do we actually need this, still?
> 
> GLib includes all the GThread API, these days, and it requires all threading
> support. The shared library is still there for backward compatibility
> reasons.

No idea. Is that a good enough reason to diverge from the autotools build?
Comment 6 Christoph Reiter (lazka) 2017-07-16 12:59:39 UTC
(In reply to Emmanuele Bassi (:ebassi) from comment #4)
> Review of attachment 355701 [details] [review] [review]:
> 
> I'd be okay with this. Or, alternatively, to replace all `shared_library()`
> uses with `library()`, which does the right thing depending on
> `--default-library`.

I've tried this and the build fails on both Windows/Linux.

On Windows, at least all the hackery around OS_WIN32_AND_DLL_COMPILATION, GOBJECT_STATIC_COMPILATION, GLIB_STATIC_COMPILATION, GLIB_WIN32_STATIC_COMPILATION_DEFINE is missing, so that's expected.

> Nevertheless, nobody is actually testing GLib's functionality with static
> compilation, so at the very least it'd warrant a message().
Comment 7 Christoph Reiter (lazka) 2017-07-16 13:03:59 UTC
Created attachment 355706 [details] [review]
meson: build windows resource files

configure_file() forces utf-8 atm but .rc files are not utf-8.
To work around the issue just remove the only non-ASCII char.
Comment 8 Christoph Reiter (lazka) 2017-07-16 14:43:44 UTC
Created attachment 355712 [details] [review]
meson: Fix static build under Windows

Turns out that defining GLIB/GOBJECT_STATIC_COMPILATION is all that's needed.
Comment 9 Christoph Reiter (lazka) 2017-07-17 16:36:35 UTC
Created attachment 355769 [details] [review]
meson: define G_PID_FORMAT
Comment 10 Emmanuele Bassi (:ebassi) 2017-07-17 16:48:18 UTC
Review of attachment 355769 [details] [review]:

::: meson.build
@@ +913,3 @@
 if host_system == 'windows'
   glibconfig_conf.set('g_pid_type', 'void*')
+  glibconfig_conf.set('g_pid_format', '"p"')

Probably want to use:

  glibconfig_conf.set_quoted('g_pid_format', 'p')

@@ +921,3 @@
 else
   glibconfig_conf.set('g_pid_type', 'int')
+  glibconfig_conf.set('g_pid_format', '"i"')

Same as above:

  glibconfig_conf.set_quoted('g_pid_format', 'i')

But moving to set_quoted() should likely go as a separate patch, so that more quoted values are moved over.
Comment 11 Christoph Reiter (lazka) 2017-07-17 19:32:38 UTC
Created attachment 355776 [details] [review]
meson: use set_quoted() instead of quoting manually
Comment 12 Christoph Reiter (lazka) 2017-07-17 19:42:00 UTC
Created attachment 355779 [details] [review]
meson: set glib_extension in glibconfig.h to match the autotools output
Comment 13 Christoph Reiter (lazka) 2017-07-18 12:36:05 UTC
Created attachment 355824 [details] [review]
meson: Add 'with-threads' option to allow building with posix threads on Windows
Comment 14 Emmanuele Bassi (:ebassi) 2017-07-19 14:08:23 UTC
Review of attachment 355706 [details] [review]:

Okay.
Comment 15 Emmanuele Bassi (:ebassi) 2017-07-19 14:09:24 UTC
Review of attachment 355712 [details] [review]:

Assuming somebody will ever CI it, sounds okay to me.
Comment 16 Emmanuele Bassi (:ebassi) 2017-07-19 14:12:40 UTC
Review of attachment 355776 [details] [review]:

Looks good.
Comment 17 Emmanuele Bassi (:ebassi) 2017-07-19 14:13:53 UTC
Review of attachment 355779 [details] [review]:

Okay
Comment 18 Emmanuele Bassi (:ebassi) 2017-07-19 14:14:51 UTC
Review of attachment 355824 [details] [review]:

Okay.

::: meson_options.txt
@@ +8,3 @@
 option('tapset-install-dir', type : 'string', value : '',
   description : 'path where systemtap tapsets are installed')
+option('with-threads', type : 'combo', choices : ['auto', 'posix', 'win32'], value : 'auto')

Would be nice to have a description field.
Comment 19 Emmanuele Bassi (:ebassi) 2017-07-19 14:16:15 UTC
Review of attachment 355769 [details] [review]:

Okay.
Comment 20 Christoph Reiter (lazka) 2017-07-19 14:36:11 UTC
Created attachment 355951 [details] [review]
meson: Define GLIB_EXTRA_CFLAGS and G_LIBS_EXTRA

GLIB_EXTRA_CFLAGS gets used globally as cflag and exposed in the .pc file
G_LIBS_EXTRA gets exposed in the .pc file
Comment 21 Emmanuele Bassi (:ebassi) 2017-07-19 14:48:10 UTC
Review of attachment 355951 [details] [review]:

::: meson.build
@@ +1415,3 @@
+if host_system == 'windows' and cc.get_id() != 'msvc'
+  glib_conf.set('GLIB_EXTRA_CFLAGS', '-mms-bitfields')
+  add_global_arguments(glib_conf.get('GLIB_EXTRA_CFLAGS'), language : 'c')

This should be `add_project_arguments()`, to allow using GLib as a sub-project.
Comment 22 Christoph Reiter (lazka) 2017-07-19 15:18:09 UTC
Created attachment 355958 [details] [review]
meson: Define GLIB_EXTRA_CFLAGS and G_LIBS_EXTRA

GLIB_EXTRA_CFLAGS gets used globally as cflag and exposed in the .pc file
G_LIBS_EXTRA gets exposed in the .pc file
Comment 23 Christoph Reiter (lazka) 2017-07-19 15:20:16 UTC
To add some context to the last one:

GLIB_EXTRA_CFLAGS is defined here: https://git.gnome.org/browse/glib/tree/configure.ac#n318

G_LIBS_EXTRA here: https://git.gnome.org/browse/glib/tree/configure.ac#n2422
Comment 24 Christoph Reiter (lazka) 2017-07-20 11:45:03 UTC
Created attachment 356025 [details] [review]
meson: Set PCRE_REQUIRES

gets used in glib-2.0.pc.in
Comment 25 Christoph Reiter (lazka) 2017-07-22 07:02:49 UTC
Comment on attachment 356025 [details] [review]
meson: Set PCRE_REQUIRES

patch needs an update for the recent changes on master
Comment 26 Fan, Chun-wei 2017-07-25 08:08:50 UTC
Created attachment 356338 [details] [review]
meson: build windows resource files (take ii)

Hi,

I am re-uploading this patch as there were some version info in the .rc files that were not covered, namely the one that is supposed to be represented by @GLIB_VERSION@, so we need to put glib_version into glibconfig_conf.

We don't have the @LT_CURRENT_MINUS_AGE@ filled in, but since this is not libtool, I think we can just ignore it for now.

With blessings, thank you!
Comment 27 Christoph Reiter (lazka) 2017-07-25 08:19:12 UTC
Thanks!

related: The next meson should print warnings when variables are missing: https://github.com/mesonbuild/meson/pull/2094
Comment 28 Xavier Claessens 2017-11-29 18:18:18 UTC
Review of attachment 355712 [details] [review]:

::: glib/glibconfig.h.in
@@ +20,3 @@
 
+#mesondefine GLIB_STATIC_COMPILATION
+#mesondefine GOBJECT_STATIC_COMPILATION

You should also remove @glib_static_compilation@ from that file, it's causing a warning in meson configure.
Comment 29 Xavier Claessens 2017-11-29 18:22:35 UTC
Review of attachment 355824 [details] [review]:

Looks like the guideline says we should drop with- and enable- prefix on configurations. See bug #790837, ebassi suggested there: "This should probably be renamed to `threads_implementation` or `threads_impl`."
Comment 30 Christoph Reiter (lazka) 2017-12-14 13:34:05 UTC
Created attachment 365536 [details] [review]
meson: build Windows resource files

configure_file() forces utf-8 atm but .rc files are not utf-8.
To work around the issue just remove the only non-ASCII char.
Comment 31 Christoph Reiter (lazka) 2017-12-14 13:34:37 UTC
Created attachment 365537 [details] [review]
meson: fix static build under Windows

Properly define GLIB/GOBJECT_STATIC_COMPILATION when static build is enabled.
Use library() instead of shared_library() to allow selecting static builds.
Comment 32 Christoph Reiter (lazka) 2017-12-14 13:35:09 UTC
Created attachment 365538 [details] [review]
meson: add 'threads_implementation' option to allow building with posix threads on Windows

MSYS2 builds with posix threads by default, which is why making
this configurable is needed.

Defaults to win32 on Windows and posix everywhere else, as before.
Comment 33 Christoph Reiter (lazka) 2017-12-14 13:35:39 UTC
Created attachment 365539 [details] [review]
meson: define GLIB_EXTRA_CFLAGS and G_LIBS_EXTRA on Windows

GLIB_EXTRA_CFLAGS gets used globally as cflag and exposed in the .pc file
G_LIBS_EXTRA gets exposed in the .pc file
Comment 34 Christoph Reiter (lazka) 2017-12-14 13:37:07 UTC
(In reply to Xavier Claessens from comment #28)
> Review of attachment 355712 [details] [review] [review]:
> 
> ::: glib/glibconfig.h.in
> @@ +20,3 @@
>  
> +#mesondefine GLIB_STATIC_COMPILATION
> +#mesondefine GOBJECT_STATIC_COMPILATION
> 
> You should also remove @glib_static_compilation@ from that file, it's
> causing a warning in meson configure.

done

(In reply to Xavier Claessens from comment #29)
> Review of attachment 355824 [details] [review] [review]:
> 
> Looks like the guideline says we should drop with- and enable- prefix on
> configurations. See bug #790837, ebassi suggested there: "This should
> probably be renamed to `threads_implementation` or `threads_impl`."

Sure, I've used "threads_implementation", and adjusted the variable name in the meson file as well.

Thanks!
Comment 35 Xavier Claessens 2017-12-14 15:07:58 UTC
Review of attachment 365538 [details] [review]:

::: meson_options.txt
@@ +11,3 @@
 option('charsetalias-dir', type : 'string', value : '',
   description : 'directory for charset.alias file (libdir by default)')
+option('threads_implementation', type : 'combo', choices : ['auto', 'posix', 'win32'], value : 'auto',

I think we are trying to avoid "auto" options. I would remove 'auto' from the choices and set posix as default. If you want win32 implementation opt-in explicitly. Emmanuele should confirm this.
Comment 36 Nirbheek Chauhan 2017-12-14 15:53:20 UTC
I believe auto is needed because POSIX is not the default everywhere. For instance when building with MSVC you only have win32.
Comment 37 Christoph Reiter (lazka) 2017-12-27 23:35:59 UTC
Another possibility would be a boolean "force_posix_threads"
Comment 38 Xavier Claessens 2018-01-02 16:30:14 UTC
Review of attachment 365538 [details] [review]:

I've got a question, in the case we use win32 threads, should we still call thread_dep = dependency('threads') ? If yes, then how does meson knows the choice we made?
Comment 39 Xavier Claessens 2018-01-02 16:36:38 UTC
Review of attachment 365537 [details] [review]:

IMO, this could already be merged, but I'm not glib maintainer.

::: meson.build
@@ +125,3 @@
+    glibconfig_conf.set('GOBJECT_STATIC_COMPILATION', '1')
+endif
+

Hmm. I'm adding default_library='both' option (meaning it compiles once and create both static and shared libs from object files). But that option should be forbidden on windows then.

See https://github.com/mesonbuild/meson/pull/2711.

I guess that's why Cerbero (GStreamer build tool) does not pass --enable-static when building for windows.
Comment 40 Christoph Reiter (lazka) 2018-01-02 17:20:53 UTC
(In reply to Xavier Claessens from comment #38)
> Review of attachment 365538 [details] [review] [review]:
> 
> I've got a question, in the case we use win32 threads, should we still call
> thread_dep = dependency('threads') ? If yes, then how does meson knows the
> choice we made?

dependency('threads') just adds "-pthread" when gcc/clang is used and nothing in case of MSVC. So at least for the default builds we care about (msys2 and msvc) this should just work. In case of mingw+win32 threads this might not work, but imo this is something meson should document how to handle.

Related: https://github.com/mesonbuild/meson/issues/553
Comment 41 Christoph Reiter (lazka) 2018-01-02 17:33:02 UTC
We could also tie the thread implementation used to the compiler used (win32 if cc.get_id() == 'msvc') and not add a threads_implementation option for now. Any thoughts?
Comment 42 Xavier Claessens 2018-01-02 18:10:31 UTC
My understanding is that dependency('threads') is really an helper for the pthread flavours. In the case you're using something other than pthread (win32, others?) then you're on your own and you shouldn't call dependency('threads') otherwise it could add '-pthread' flag on platforms that supports pthread but you choosed to use another one.
Comment 43 Christoph Reiter (lazka) 2018-01-02 20:05:02 UTC
Created attachment 366217 [details] [review]
meson: add 'threads_implementation' option to allow  building with posix threads on Windows

Makes sense.

I've changed it now to only use dependency('threads') in case of posix threads and to use dependency('', required : false) as a dummy dependency otherwise, as is suggested in the meson documentation.
Comment 44 Christoph Reiter (lazka) 2018-01-02 20:05:31 UTC
Created attachment 366218 [details] [review]
meson: define GLIB_EXTRA_CFLAGS and G_LIBS_EXTRA on  Windows

Rebase on master
Comment 45 Xavier Claessens 2018-01-02 20:20:42 UTC
Review of attachment 366217 [details] [review]:

What about removing 'auto' and use that kind of code? 

if cc.get_id() == 'msvc'
  threads_implementation = 'win32'
elif host_system == 'windows'
  threads_implementation = get_option('threads_implementation')
else
  threads_implementation = 'posix'
endif

Does it make sense?
Comment 46 Xavier Claessens 2018-01-02 20:32:58 UTC
Hmm, reading again what configure.ac does, it seems broken there...

if test "x$with_threads" = xyes || test "x$with_threads" = xwin32; then
        case $host in
                *-*-mingw*)
                have_threads=win32
                ;;
        esac
fi

If I understand correctly it uses win32 threads only on mingw. Is that right?
Comment 47 Christoph Reiter (lazka) 2018-01-02 21:01:07 UTC
(In reply to Xavier Claessens from comment #45)
> Review of attachment 366217 [details] [review] [review]:
> 
> What about removing 'auto' and use that kind of code? 
> 
> if cc.get_id() == 'msvc'
>   threads_implementation = 'win32'
> elif host_system == 'windows'
>   threads_implementation = get_option('threads_implementation')
> else
>   threads_implementation = 'posix'
> endif
> 
> Does it make sense?

It does. But it would be a bit confusing imo as the value shown by meson config wouldn't match what is actually used in some cases. I'd rename the option to "mingw_thread_implementation" then to avoid that confusion.

I'd prefer a "force_posix_threads" over this as it's easier to describe and just a boolean.

I can also remove the option for now and just patch it in msys2. There are quite a few patches in msys2 already, so one more wont be that bad. My current goal is to make the meson build on msys2 match the autotools one to reduce the chance of regressions and then try to reduce the number of patches.

(In reply to Xavier Claessens from comment #46)
> If I understand correctly it uses win32 threads only on mingw. Is that right?

What else is there on Windows using autotools besides mingw?
Comment 48 Xavier Claessens 2018-01-02 21:21:57 UTC
(In reply to Christoph Reiter (lazka) from comment #47)
> (In reply to Xavier Claessens from comment #46)
> > If I understand correctly it uses win32 threads only on mingw. Is that right?
> 
> What else is there on Windows using autotools besides mingw?

Right, forget that :-)
Comment 49 Xavier Claessens 2018-01-03 17:06:12 UTC
Review of attachment 366218 [details] [review]:

Once patches from bug #788773 lands, this patch will have to be updated because it won't use glib_conf anymore to generate pc files.

::: meson.build
@@ +1621,3 @@
+if host_system == 'windows' and cc.get_id() != 'msvc'
+  glib_conf.set('GLIB_EXTRA_CFLAGS', '-mms-bitfields')
+  add_project_arguments(glib_conf.get('GLIB_EXTRA_CFLAGS'), language : 'c')

It doesn't seems to be used globally in Makefiles, only glib-2.0.pc files use that value. Is it a bug in Makefile.am ?

This drops the check for gcc2, I guess that's fine, right?

Oh and configure.ac only does that for GCC, looks like you fix the clang case, if I understand correctly.

@@ +1625,3 @@
+
+if host_system == 'windows' and cc.get_id() != 'msvc'
+  glib_conf.set('G_LIBS_EXTRA', '-lws2_32 -lole32 -lwinmm -lshlwapi')

This one is used to build libglib in Makefile.am. Should we use add_project_arguments() here instead?

Note that if you do:
glib_extra_dep=declare_dependency(link_args='-lws2_32 -lole32 -lwinmm -lshlwapi')
And then add that dependency when building libglib, it will get into PC's Libs.private automatically (when using pc generator).
Comment 50 Christoph Reiter (lazka) 2018-01-03 17:32:41 UTC
(In reply to Xavier Claessens from comment #49)
> Review of attachment 366218 [details] [review] [review]:
> 
> Once patches from bug #788773 lands, this patch will have to be updated
> because it won't use glib_conf anymore to generate pc files.
> 
> ::: meson.build
> @@ +1621,3 @@
> +if host_system == 'windows' and cc.get_id() != 'msvc'
> +  glib_conf.set('GLIB_EXTRA_CFLAGS', '-mms-bitfields')
> +  add_project_arguments(glib_conf.get('GLIB_EXTRA_CFLAGS'), language : 'c')
> 
> It doesn't seems to be used globally in Makefiles, only glib-2.0.pc files
> use that value. Is it a bug in Makefile.am ?

There also is CFLAGS="$CFLAGS $msnative_struct" which I think gets used everywhere?

> This drops the check for gcc2, I guess that's fine, right?

GCC 2.95 July 31, 1999

> Oh and configure.ac only does that for GCC, looks like you fix the clang
> case, if I understand correctly.

From what I see GCC is set by AC_PROG_CC and is also set when clang is used, so no difference.

> @@ +1625,3 @@
> +
> +if host_system == 'windows' and cc.get_id() != 'msvc'
> +  glib_conf.set('G_LIBS_EXTRA', '-lws2_32 -lole32 -lwinmm -lshlwapi')
> 
> This one is used to build libglib in Makefile.am. Should we use
> add_project_arguments() here instead?

uh, I guess, thanks.

> Note that if you do:
> glib_extra_dep=declare_dependency(link_args='-lws2_32 -lole32 -lwinmm
> -lshlwapi')
> And then add that dependency when building libglib, it will get into PC's
> Libs.private automatically (when using pc generator).

Thanks for the tip, I'll give that a try. Would that also work if msvc is used?
Comment 51 Xavier Claessens 2018-01-03 18:30:29 UTC
(In reply to Christoph Reiter (lazka) from comment #50)
> There also is CFLAGS="$CFLAGS $msnative_struct" which I think gets used
> everywhere?

Good point, so I think you're right to use add_project_arguments() here.

> > @@ +1625,3 @@
> > +
> > +if host_system == 'windows' and cc.get_id() != 'msvc'
> > +  glib_conf.set('G_LIBS_EXTRA', '-lws2_32 -lole32 -lwinmm -lshlwapi')
> > 
> > This one is used to build libglib in Makefile.am. Should we use
> > add_project_arguments() here instead?
> 
> uh, I guess, thanks.

This one is not global though, only used for libglib.

> > Note that if you do:
> > glib_extra_dep=declare_dependency(link_args='-lws2_32 -lole32 -lwinmm
> > -lshlwapi')
> > And then add that dependency when building libglib, it will get into PC's
> > Libs.private automatically (when using pc generator).
> 
> Thanks for the tip, I'll give that a try. Would that also work if msvc is
> used?

In the case you have no extra flags to you can declare an "empty" dep:
glib_extra_dep=dependency('', required:false)

But maybe I'm over-thinking this, passing it as string to link_args is good enough. It's really similar to export_dynamic_flag I added in bug #788773 comment #34.
Comment 52 Christoph Reiter (lazka) 2018-01-04 16:36:15 UTC
I'm currently waiting on https://github.com/mesonbuild/meson/issues/2872 to be resolved.

Can I push the first two patches in the meantime?
Comment 53 Xavier Claessens 2018-01-04 16:56:35 UTC
Review of attachment 365536 [details] [review]:

I have no idea how resources works on windows, so I trust you your patch actually works. Cannot make it worst anyway :-)

::: gio/meson.build
@@ +396,3 @@
+    configuration: glibconfig_conf,
+  )
+  gio_win_res = import('windows').compile_resources(gio_win_rc)

I would rather import('windows') once in the main meson.build instead of duplicating it in each subdir.

::: gmodule/meson.build
@@ +85,3 @@
 install_headers(['gmodule.h'], subdir : 'glib-2.0')
 
+plat_src = ['gmodule.c']

nitpicking: why calling it plat_src if it contains all sources and not only platform specific sources? I would rather call it gmodule_sources just like there is already glib_sources, gobject_sources, etc.

::: gthread/meson.build
@@ +2,3 @@
 # has been moved into glib now
+
+plat_src = ['gthread-impl.c']

same nitpicking about the name.
Comment 54 Christoph Reiter (lazka) 2018-01-04 21:08:22 UTC
Created attachment 366330 [details] [review]
meson: build Windows resource files
Comment 55 Xavier Claessens 2018-01-04 21:15:10 UTC
Review of attachment 366330 [details] [review]:

Looks good to me.
Comment 56 Christoph Reiter (lazka) 2018-01-07 08:14:47 UTC
Created attachment 366446 [details] [review]
meson: add 'force_posix_threads' option to allow building  with posix threads on Windows

Changed to a non-auto option
Comment 57 Christoph Reiter (lazka) 2018-01-07 08:17:24 UTC
Created attachment 366447 [details] [review]
meson: define GLIB_EXTRA_CFLAGS and G_LIBS_EXTRA on Windows

Added G_LIBS_EXTRA to link_args of glib-2.0 (I've also removed the cygwin case, as I can't test it atm)
Comment 58 LRN 2018-01-13 12:26:20 UTC
If there are plans to have more than 2 (W32 and POSIX) thread backends (that work on Windows) for GThread, then --with-threads is the right kind of options for thread selection. Otherwise --force-posix-threads would be enough. No objections to the name itself.
Comment 59 Philip Withnall 2018-01-18 10:19:29 UTC
As per https://bugzilla.gnome.org/show_bug.cgi?id=685442#c44, there are apparently some problems with detecting stpcpy and posix_memalign when building on Windows with mingw. What’s the best solution for that?

(In reply to Jan-Michael Brummer from bug #685442, comment 43)
> Meson finds stpcpy () and posix_memalign () during configuration but not
> during linking. Both shouldn't be defined as they are not available for
> windows.

(In reply to Jan-Michael Brummer from bug #685442, comment 44)
> Meson build problem is within mingw toolchain: Those functions are exported
> but not provides. Therfore meson is falling back to compiler built-in
> functions of stpcpy() and posix_memalign().
> 
> Solution:
>  - Either set them in meson cross compile script to false:
>     has_function_stpcpy = false
>     has_function_posix_memalign = false
> 
>  - Change glib meson check to include the requested header file.
Comment 60 Christoph Reiter (lazka) 2018-01-18 10:26:32 UTC
(In reply to Philip Withnall from comment #59)
> As per https://bugzilla.gnome.org/show_bug.cgi?id=685442#c44, there are
> apparently some problems with detecting stpcpy and posix_memalign when
> building on Windows with mingw. What’s the best solution for that?

We currently patch this in msys2, but I don't know why it's needed: https://github.com/Alexpux/MINGW-packages/blob/master/mingw-w64-meson/force-func-detect.patch
Comment 61 LRN 2018-01-18 12:55:34 UTC
If you're wondering about stpcpy(), read here[0]

[0] https://sourceforge.net/p/mingw-w64/mailman/message/31522173/
Comment 62 kkartaltepe 2018-02-02 00:22:39 UTC
If --force-posix-threads is accepted then HAVE_PTHREAD_CONDATTR_SETCLOCK check should consider having a configure time check against CLOCK_MONOTONIC. There are windows pthread implementations which offer pthread_condattr_setclock() but only accept CLOCK_REALTIME (notably mingw64). This would result in some unfortunate aborts that could be reasonably detected.
Comment 63 kkartaltepe 2018-02-02 00:28:05 UTC
Forgive me I was reading this and posted before I noticed that check was already in place.
Comment 64 Xavier Claessens 2018-05-15 19:21:07 UTC
Created attachment 372084 [details] [review]
Meson: add 'force_posix_threads' option

This allows building with posix threads on Windows. MSYS2 on Windows
builds with posix threads by default, which is why making this
configurable is needed.
Comment 65 Xavier Claessens 2018-05-15 19:21:20 UTC
Created attachment 372085 [details] [review]
Meson: Remove FIXME about COCOA_LIBS and CARBON_LIBS

They are already handled properly by osx_ldflags. As far as I can tell
it does the same as with autotools.
Comment 66 Xavier Claessens 2018-05-15 19:21:32 UTC
Created attachment 372086 [details] [review]
Meson: Add missing flags on Windows

win32_cflags gets used globally as cflags and exposed in the .pc file.
win32_ldflags gets passed to glib-2.0 and exposed in the .pc file.

This should match what the autotools build is currently doing with
GLIB_EXTRA_CFLAGS and G_LIBS_EXTRA.
Comment 67 Xavier Claessens 2018-05-15 19:27:40 UTC
Slightly reworked the patches that were waiting here, rebased on master. I didn't test those patches on Windows, I just tried to follow exactly what autotools does.

btw, any reason G_LIBS_EXTRA is only for libglib and not global?
Comment 68 LRN 2018-05-15 19:40:21 UTC
(In reply to Xavier Claessens from comment #67)
> btw, any reason G_LIBS_EXTRA is only for libglib and not global?

Why should it be global? If gio and gobject libraries link without G_LIBS_EXTRA, then they don't need G_LIBS_EXTRA. Besides, i think that half of the libs that go into G_LIBS_EXTRA on Windows are already linked by default. Specifically, mingw-w64 gcc always links -ladvapi32 -lshell32 -luser32 -lkernel32, so sayeth gcc -dumpspecs.
Comment 69 Xavier Claessens 2018-05-15 20:40:43 UTC
Ok, I have ~knowledge on windows flags, just wanted to make sure it's intentional and not just copying old code from autotools that could be broken :-)
Comment 70 LRN 2018-05-15 21:29:07 UTC
Review of attachment 372086 [details] [review]:

Works for me.
Comment 71 LRN 2018-05-15 21:29:09 UTC
Review of attachment 372084 [details] [review]:

Works for me.
Comment 72 Philip Withnall 2018-05-16 09:04:32 UTC
Review of attachment 372085 [details] [review]:

OK.
Comment 73 Philip Withnall 2018-05-16 09:06:59 UTC
Review of attachment 372086 [details] [review]:

OK.
Comment 74 Philip Withnall 2018-05-16 09:08:24 UTC
Review of attachment 372084 [details] [review]:

Why do you need the option? Why not just always build with POSIX threads if building using MSYS2?
Comment 75 Nirbheek Chauhan 2018-05-16 09:12:17 UTC
(In reply to Philip Withnall from comment #74)
> Review of attachment 372084 [details] [review] [review]:
> 
> Why do you need the option? Why not just always build with POSIX threads if
> building using MSYS2?

AFAIK, it depends on what will consume the built glib. If your app uses POSIX threads, glib needs to use the same. Otherwise, win32 threads.
Comment 76 Christoph Reiter (lazka) 2018-05-16 09:43:44 UTC
(In reply to Nirbheek Chauhan from comment #75)
> (In reply to Philip Withnall from comment #74)
> > Review of attachment 372084 [details] [review] [review] [review]:
> > 
> > Why do you need the option? Why not just always build with POSIX threads if
> > building using MSYS2?
> 
> AFAIK, it depends on what will consume the built glib. If your app uses
> POSIX threads, glib needs to use the same. Otherwise, win32 threads.

According to https://github.com/Alexpux/MINGW-packages/pull/3546#issuecomment-379482543 and https://stackoverflow.com/a/30390278/3201632 that's not the case.

(In reply to Philip Withnall from comment #74)
> Review of attachment 372084 [details] [review] [review]:
> 
> Why do you need the option? Why not just always build with POSIX threads if
> building using MSYS2?

I think using win32 threads for CI (which uses MSYS2 atm) is better as that gives us better test coverage in that area. The posix backend is already covered by the Fedora build.

----

I don't mind carrying that patch in downstream MSYS2 for now.
Comment 77 Philip Withnall 2018-05-16 10:09:58 UTC
(In reply to Christoph Reiter (lazka) from comment #76)
> (In reply to Philip Withnall from comment #74)
> > Review of attachment 372084 [details] [review] [review] [review]:
> > 
> > Why do you need the option? Why not just always build with POSIX threads if
> > building using MSYS2?
> 
> I think using win32 threads for CI (which uses MSYS2 atm) is better as that
> gives us better test coverage in that area. The posix backend is already
> covered by the Fedora build.

That’s a good justification.

Xavier, can you put that into the commit message please?
Comment 78 LRN 2018-05-16 12:31:52 UTC
(In reply to Philip Withnall from comment #74)
> Review of attachment 372084 [details] [review] [review]:
> 
> Why do you need the option? Why not just always build with POSIX threads if
> building using MSYS2?

POSIX threads work by virtue of a POSIX thread compatibility library (winpthreads in case of mingw-w64). It is not a sure bet that this compatibility library is better than using W32 threads directly from glib (since glib already abstracts away the platform difference). There may also be subtle feature and behaviour differences between glib with W32 threads backend and glib with winpthreads backend. I very much agree that having an option to be able to build with either one is a benefit.

glib-using applications shouldn't care which flavour of threads is used internally by glib, by the way. But if some bugs crop up somewhere, swapping the thread backend is definitely a thing worth trying to do.
Comment 79 Xavier Claessens 2018-05-16 14:02:35 UTC
I'm a bit confused, I though using win32 threads with msys2 was just not working at all, but now I read that it doesn't matter and both thread implementations can be mixed?

So if I understand correctly choosing between win32 or posix thread implementation is just a matter of taste? I guess there could be performance implications but we have no data on that, do we? From a functionality POV we could even drop win32 implementation completely and that wouldn't change anything?

I'm all for giving chose here, since we have 2 implementations available we should be able to switch between them at least to be able to compare their performance, CI testing, etc.
Comment 80 Xavier Claessens 2018-05-16 14:20:11 UTC
Attachment 372085 [details] pushed as cf28bf1 - Meson: Remove FIXME about COCOA_LIBS and CARBON_LIBS
Attachment 372086 [details] pushed as 4b82738 - Meson: Add missing flags on Windows
Comment 81 Xavier Claessens 2018-05-16 14:24:55 UTC
Created attachment 372124 [details] [review]
Meson: add 'force_posix_threads' option

This allows building with posix threads on Windows. It is generally
better to use win32 threads implementation on Windows, but this option
can be used in case it causes issues, or for performance comparison for
example.
Comment 82 Xavier Claessens 2018-05-16 14:25:49 UTC
Attachment 372124 [details] pushed as 2477c7b - Meson: add 'force_posix_threads' option
Comment 83 LRN 2018-05-16 15:16:18 UTC
(In reply to Xavier Claessens from comment #79)
> I'm a bit confused, I though using win32 threads with msys2 was just not
> working at all, but now I read that it doesn't matter and both thread
> implementations can be mixed?
Not mixed. You use either one or the other. Well, i mean, if you have two libraries, one uses POSIX threads via winpthreads, and the other uses plain W32 thread API, that's fine, they won't interfere with each other (although manipulating the same system thread using 2 different APIs might cause issues (most likely - POSIX thread might arrive at an internal state that is inconsistent with the actual state of the thread it represent, since the use of W32 thread API on that thread would circumvent winpthread code and leave it clueless about any changes) - hopefully, no one is stupid enough to do that). But inside glib only one thread backend is used, and that's decided at build time.

> 
> So if I understand correctly choosing between win32 or posix thread
> implementation is just a matter of taste? I guess there could be performance
> implications but we have no data on that, do we? From a functionality POV we
> could even drop win32 implementation completely and that wouldn't change
> anything?

No, we have no data on that. I haven't studied this in-depth to have a strong opinion on how feasible it is to drop the W32 implementation.
However, do note that MSVC builds of glib can most likely use only the W32 backend - they don't have winpthreads (i'm not even sure it builds with MSVC at all), and might not have any other drop-in pthread implementations either (there's pw32, and pthreads-win32 and could be other - i haven't used these in a long time).

> 
> I'm all for giving chose here, since we have 2 implementations available we
> should be able to switch between them at least to be able to compare their
> performance, CI testing, etc.

So am i.
Comment 84 Christoph Reiter (lazka) 2018-05-20 08:39:49 UTC
Thanks Xavier for fixing/pushing the remaining patches!

I've now moved the glib2-git package in MSYS2 to meson: https://github.com/Alexpux/MINGW-packages/pull/3776