GNOME Bugzilla – Bug 784995
meson: some Windows improvements
Last modified: 2018-05-20 08:39:49 UTC
My first time using meson, so I might be missing things..
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.
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.
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.
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().
(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?
(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().
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.
Created attachment 355712 [details] [review] meson: Fix static build under Windows Turns out that defining GLIB/GOBJECT_STATIC_COMPILATION is all that's needed.
Created attachment 355769 [details] [review] meson: define G_PID_FORMAT
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.
Created attachment 355776 [details] [review] meson: use set_quoted() instead of quoting manually
Created attachment 355779 [details] [review] meson: set glib_extension in glibconfig.h to match the autotools output
Created attachment 355824 [details] [review] meson: Add 'with-threads' option to allow building with posix threads on Windows
Review of attachment 355706 [details] [review]: Okay.
Review of attachment 355712 [details] [review]: Assuming somebody will ever CI it, sounds okay to me.
Review of attachment 355776 [details] [review]: Looks good.
Review of attachment 355779 [details] [review]: Okay
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.
Review of attachment 355769 [details] [review]: Okay.
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
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.
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
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
Created attachment 356025 [details] [review] meson: Set PCRE_REQUIRES gets used in glib-2.0.pc.in
Comment on attachment 356025 [details] [review] meson: Set PCRE_REQUIRES patch needs an update for the recent changes on master
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!
Thanks! related: The next meson should print warnings when variables are missing: https://github.com/mesonbuild/meson/pull/2094
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.
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`."
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.
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.
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.
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
(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!
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.
I believe auto is needed because POSIX is not the default everywhere. For instance when building with MSVC you only have win32.
Another possibility would be a boolean "force_posix_threads"
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?
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.
(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
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?
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.
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.
Created attachment 366218 [details] [review] meson: define GLIB_EXTRA_CFLAGS and G_LIBS_EXTRA on Windows Rebase on master
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?
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?
(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?
(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 :-)
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).
(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?
(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.
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?
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.
Created attachment 366330 [details] [review] meson: build Windows resource files
Review of attachment 366330 [details] [review]: Looks good to me.
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
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)
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.
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.
(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
If you're wondering about stpcpy(), read here[0] [0] https://sourceforge.net/p/mingw-w64/mailman/message/31522173/
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.
Forgive me I was reading this and posted before I noticed that check was already in place.
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.
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.
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.
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?
(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.
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 :-)
Review of attachment 372086 [details] [review]: Works for me.
Review of attachment 372084 [details] [review]: Works for me.
Review of attachment 372085 [details] [review]: OK.
Review of attachment 372086 [details] [review]: OK.
Review of attachment 372084 [details] [review]: Why do you need the option? Why not just always build with POSIX threads if building using MSYS2?
(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.
(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.
(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?
(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.
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.
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
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.
Attachment 372124 [details] pushed as 2477c7b - Meson: add 'force_posix_threads' option
(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.
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