GNOME Bugzilla – Bug 783270
Improve Visual Studio support for Meson builds
Last modified: 2017-10-10 11:50:06 UTC
Hi, There are currently WIP Meson build files in the wip/meson branch, but it could have some more improvements to be able to build GLib out-of-the-box using Visual Studio as the compiler. Note that some changes are needed in Meson itself so that Visual Studio versions that require embedding of SxS manifests in DLLs/EXEs (e.g. 2005/2008) in the Ninja build files, which is not covered in this bug report. With blessings, thank you!
Created attachment 352923 [details] [review] meson.build: Only detect va_copy() on Visual Studio 2013 and later Hi, In $(srcroot)/meson.build, va_copy() is only provided in Visual Studio 2013 and later, and somehow the current detection scheme's compile tests pass even on pre-2013 Visual Studio versions, which will cause the compile to fail. This patch disables the va_copy() checks on Visual Studio 2012 and earlier, and hence forces the builds to use a fallback implementation for G_VA_COPY, and also force-includes msvc_recommended_pragmas.h to catch some possible problems in the code when building for Windows. With blessings, thank you!
Created attachment 352924 [details] [review] gobject/meson.build: Define FFI_BUILDING for Visual Studio Hi, The libffi build files on Visual Studio generate a static lib for libffi, which clearly does not use things such as __declspec(dllimport), which will then break linking on Visual Studio builds. Fix this for now by defining FFI_BUILDING when building for GObject (this is done in the Visual Studio project files)--the optimal fix is to fix libffi, and only use __declspec(dllimport) when using libffi as a DLL. With blessings, thank you!
Just for context, in GStreamer (where this GLib port originates from) our supported baseline was Windows 7 and Visual Studio 2015 IIRC. Earlier versions just don't provide the C features we want, and it's too much hassle to support (for us anyway).
We also build against https://github.com/centricular/libffi
(In reply to Fan, Chun-wei from comment #0) > Note that some changes are needed in Meson itself so that Visual Studio > versions that require embedding of SxS manifests in DLLs/EXEs (e.g. > 2005/2008) in the Ninja build files, which is not covered in this bug report. > The oldest version of Visual Studio that Meson supports is VS 2010, and I highly doubt that any older versions will ever be supported. It's a pain to test and there is no demand for it. Does anyone actually use glib on VS 2005/2008 and can't move to 2010? (In reply to Fan, Chun-wei from comment #1) > Created attachment 352923 [details] [review] [review] > meson.build: Only detect va_copy() on Visual Studio 2013 and later > > In $(srcroot)/meson.build, va_copy() is only provided in Visual Studio 2013 > and later, and somehow the current detection scheme's compile tests pass > even on pre-2013 Visual Studio versions, which will cause the compile to > fail. > > This patch disables the va_copy() checks on Visual Studio 2012 and earlier, > and hence forces the builds to use a fallback implementation for G_VA_COPY, Why not fix the check to work on VS 2012 and earlier too? Generally it is better to have the compiler check do the feature check rather than checking a version and correlating. Also, how are you testing with VS 2012 and VS 2013? Meson doesn't have backends for those yet (although they could be added by deriving from the 2010 backend with not much work). > and also force-includes msvc_recommended_pragmas.h to catch some possible > problems in the code when building for Windows. > This makes sense, thanks! (In reply to Fan, Chun-wei from comment #2) > Created attachment 352924 [details] [review] [review] > gobject/meson.build: Define FFI_BUILDING for Visual Studio > > The libffi build files on Visual Studio generate a static lib for libffi, > which clearly does not use things such as __declspec(dllimport), which will > then break linking on Visual Studio builds. > > Fix this for now by defining FFI_BUILDING when building for GObject (this is > done in the Visual Studio project files)--the optimal fix is to fix libffi, > and only use __declspec(dllimport) when using libffi as a DLL. > I would not recommend using upstream libffi directly since they don't support building with Visual Studio 32-bit, and the 64-bit support that they have added is not tested since they have no CI for Visual Studio. As you found out, they also have a really hacky/broken build system for Visual Studio which uses autotools with a shell wrapper `msvcc.sh`. As Tim said, please use the libffi meson port[1] which I regularly test on Windows, and will eventually add CI for to build all of glib with Visual Studio (no MinGW anywhere). As such, I don't think we should add this unless you strongly feel we should support building with upstream libffi on Windows. 1. https://github.com/centricular/libffi/ Thanks for looking into this! With some luck and lots of work we'll soon have most of the GNOME foundation libraries building on Windows natively with Visual Studio.
FWIW, the steps I use to build glib natively on Windows are: 1. Install Meson using pip3 2. Download and place Ninja in PATH 3. Clone https://github.com/centricular/glib 4. Inside a Visual Studio prompt, meson _build && ninja -C _build You can also build with the VS backend which produces Visual Studio files: 1. Install Meson using pip3 2. Clone https://github.com/centricular/glib 3. Inside a Visual Studio prompt, meson --backend=vs _build && cd _build && msbuild glib.sln In either case, meson will initialize the subprojects necessary for building glib (proxy-libintl, zlib, and libffi).
Forgot to mention: I use centricular/glib because it's based on top of the stable 2.52.2 release, and has one commit that's needed to bootstrap glib on Windows from scratch: https://github.com/centricular/glib/commit/e065e139e25c48f62f48829068e3f8e46bffa655 With that commit, you should be able to do the same with wip/meson.
Hi Nirbheek, (In reply to Nirbheek Chauhan from comment #5) > > Does anyone actually use glib on VS 2005/2008 and can't move to 2010? > I understand this... One of the main reasons why I was looking into Visual Studio 2008 support is due to the support of Python 2.7.x in various modules, such as GObject-Introspection (g-ir-scanner) and PyGObject (which, the official Python binaries/installers are built with Visual Studio 2008). > > Why not fix the check to work on VS 2012 and earlier too? Generally it is > better to have the compiler check do the feature check rather than checking > a version and correlating. I will look into this. > > Also, how are you testing with VS 2012 and VS 2013? Meson doesn't have > backends for those yet (although they could be added by deriving from the > 2010 backend with not much work). I actually built with the Ninja backend, like what you described in comment #6. > > > and also force-includes msvc_recommended_pragmas.h to catch some possible > > problems in the code when building for Windows. > > > > This makes sense, thanks! The advantage of being the maintainer of the Visual Studio projects of the GTK+ stack 8) > As Tim said, please use the libffi meson port[1]... > 1. https://github.com/centricular/libffi/ Agreed. Perhaps this should be reflected in the readme's as well. With blessings, thanks for the feedback and suggestions!
Created attachment 355726 [details] [review] meson.build: Update check for va_copy()/__va_copy() Hi, Sorry it took a while for me to get back to this, so this patch is to update the check for va_copy()/__va_copy() It seems that for compiling the checks, we actually get a C4013 warning ('<function>' undefined; assuming extern returning int) when __va_copy() and va_copy() is not present. So, we could just include the existing msvc_recommended_pragmas.h in the check program when we are using Visual Studio so that we bail out of the check program compilation when we hit a C4013 warning. This also makes sure we always include msvc_recommended_pragmas.h and so we could get rid of /wd4244 and /wd4305 as it covers this for us, and updated the comments on dirent.h and sys/time.h on Visual Studio, as Visual Studio does not ship with these headers by default. I also checked with 2015 and the check for va_copy() did succeed as it should have been. I managed to successfully build, but not run, a build of GLib using Visual Studio 2010, as it does seem that we need to check further to see whether we need to use the items in glib/gnulib and glib/libcharset as the standard *printf() etc. functions in the pre-2015 (and so 2017) Visual Studio CRT is not enough for our purposes here. Thanks though, with blessings!
Review of attachment 355726 [details] [review]: LGTM!
Comment on attachment 355726 [details] [review] meson.build: Update check for va_copy()/__va_copy() Hi Nirbheek, Thanks! I pushed the patch as 5ba3b40. With blessings, thank you!
Created attachment 355735 [details] [review] Meson: Check for HAVE_GOOD_PRINTF Hi, This determines HAVE_GOOD_PRINTF in our configuration process by relying on the results of HAVE_C99_VSNPRINTF and HAVE_C99_SNPRINTF instead of hardcoding it to 1. This fixes the situation on pre-Visual Studio 2015 builds where GLib-using programs trigger a CRT abort() when attempting to use the g_*print* functions, at least. Note that the HAVE_UNIX98_PRINTF also may have something to do with HAVE_GOOD_PRINTF, but I won't be able to check and test for that, since I am not that knowledgeable in that platform. With blessings, thank you!
Created attachment 355837 [details] [review] Meson: Set _WIN32_WINNT Hi, This sets _WIN32_WINNT as 0x0501 (XP) as default and offers an option to set it as 0x0600 (Vista), so that we can ensure that the functions we need from the Windows SDK (or MinGW/mingw-w64) can be enabled from the headers during our build. As if_nametoindex() and if_indextoname() are provided on Vista and later, check for them by linking test programs against iphlpapi.lib (-liphlpapi) when we disable XP support (and don't bother checking for them on XP). Check for them as we used to on other platforms, though. With blessings, thank you!
Review of attachment 355735 [details] [review]: ::: glib/meson.build @@ +6,2 @@ subdir('libcharset') +subdir('gnulib') I think we should subdir into gnulib and link to gnulib_lib only if we actually need the things in it. Otherwise we build the static lib for no reason. This is also what Autotools does. Setting gnulib_lib = [] will work in the "not-needed" case. ::: meson.build @@ +643,3 @@ + # Our printf is 'good' only if vsnpintf()/snprintf() supports C99 well enough + glib_conf.set('HAVE_GOOD_PRINTF', 1) # FIXME +endif Can you remove the "FIXME" if it's no longer needed? Also, what about HAVE_VASPRINTF?
To be honest, I think GLib should just drop support for everything < Win7, and also older toolchains than MSVC 2015. GStreamer certain has/will. Something for GLib maintainers to decide of course ;)
Review of attachment 355837 [details] [review]: ::: meson_options.txt @@ +9,3 @@ description : 'path where systemtap tapsets are installed') +option('use-nt6-api', type : 'boolean', value : false, + description : 'Disable Windows XP support and use Vista+ APIs') Do we want this as the default? The vast majority of people use Windows 7 and later, and making them use legacy APIs is not the best approach. You also cannot have legacy support if you want to build for the Universal Windows Platform (Windows Mobile, or Windows Store). I'm guessing that eventually we want to drop XP support too. Perhaps a good step towards that is by making it not the default?
Created attachment 355915 [details] [review] Meson: Check for HAVE_GOOD_PRINTF (take ii) Hi Nirbheek, Thanks for the feedback! I fixed what you mentioned, but kept the FIXME for HAVE_PRINTF_UNIX98, as I won't be able to check nor fix for that, so someone more knowledgeable in the platform will need to determine this. So here is the new patch... --- Hi Tim-Philipp, (In reply to Tim-Philipp Müller from comment #15) > Something for GLib maintainers to decide of course ;) Unfortunately it turns out that the C99 *printf() implementation in Visual Studio 2015 seems not to be good enough, as the test-printf.c test program will still fail (crash) when we build GLib using only the system *printf(), although the one with the gnulib stuff built in will succeed, although we do get further than the standard pre-2015 CRTs. So, I updated the check and comments about the checks for the good C99 vsnprintf() and snprintf(). Again, it depends on the main GLib maintainers to decide which toolsets we want to continue to support, but before we do decide so, I can maintain the pre-2015 support fr Visual Studio as well. I am all for dropping pre-Windows Vista/7 support though :), as probably people have seen me trying to advocate in this regard. With blessings, thank you!
Created attachment 355916 [details] [review] Meson: Set _WIN32_WINNT (take ii) Hi Nirbheek, This patch sets _WIN32_WINNT as 0x0600 by default, and gives an option to enable XP support (which I am all for forgoing :)). With blessings, thank you!
(In reply to Fan, Chun-wei from comment #8) > > Does anyone actually use glib on VS 2005/2008 and can't move to 2010? > > > > I understand this... One of the main reasons why I was looking into Visual > Studio 2008 support is due to the support of Python 2.7.x in various > modules, such as GObject-Introspection (g-ir-scanner) and PyGObject (which, > the official Python binaries/installers are built with Visual Studio 2008). I'm not entirely sure about gobject-introspection; the only reason Python 2.7 is still supported is because of RHEL, which is not shipping with Python 3 out of the box. PyGObject supporting Python 2.7 is still kind of important, given that there are still Python 2 applications. Additionally, it's still going to be possible to build GLib with Autotools for at least a few cycles (see above, re: RHEL), so we still have that fallback. Personally, though, I think it's time to drop Windows XP from GLib. We've been talking about doing it for the past two/three years, and I think we should finally let it go.
Created attachment 356102 [details] [review] Windows: Compile .rc files Hi, This ensures that the .rc files are compiled and built into the various DLLs that we built for GLib, so that one can determine the version info of the DLLs easier. With blessings, thank you!
Review of attachment 356102 [details] [review]: This has already been addressed in bug 784995, attachment 355706 [details] [review].
Hi Emmanuele, (In reply to Emmanuele Bassi (:ebassi) from comment #21) > Review of attachment 356102 [details] [review] [review]: > > This has already been addressed in bug 784995, attachment 355706 [details] [review] > [review]. Ah, I see... However, it does seem that is not getting the version info in there, and I think it is still better to save the rc.in as utf-8. My take at it, thanks! With blessings, and cheers!
Created attachment 356334 [details] [review] Meson: Check for HAVE_GOOD_PRINTF (take iii) Hi, This patch needs to be updated due to rebasing on the check of the system's included PCRE. p.s. Another patch may be required if the system's PCRE installation is built as a DLL instead of a static lib on Windows/Visual Studio due to symbol decoration in PCRE's headers, otherwise GLib will fail to link. With blessings, thank you!
Created attachment 356335 [details] [review] Meson: Set _WIN32_WINNT (take iii) Hi, As per Emmanuele's views, I think we should just set _WIN32_WINNT to be 0x0601 (Windows 7), but retain the previous Winsock2 function check to check at 0x0600 (along with the if_nametoindex() and if_indextoname() checks, since 0x0600 is enough for them). I am not touching the autotools/Visual Studio projects yet for this, but if the consensus is to do so, I think we can open up another bug to deal with that. With blessings, thank you!
Comment on attachment 356102 [details] [review] Windows: Compile .rc files Hi, (For records, see bug 784995 for this item).
Created attachment 356343 [details] [review] Meson: Don't hardcode -DPCRE_STATIC when building GLib Hi, The PCRE build that is existing on the Windows system where the build is carried out could be a static .lib build or a DLL + import .lib build, and the PCRE_STATIC is really something that looks for a static build of PCRE. This means that the GLib DLL will fail to link since we always assume a static build of PCRE (with -DPCRE_STATIC in glib/meson.build). Note that PCRE_STATIC is only meaningful on Windows. Since we have no way by just looking at the PCRE .lib file to determine whether the it is a static or DLL build, we need to check whether it is a static build by seeing whether a test program with pcre_free() links with PCRE_STATIC defined. If it does, then we build GLib with PCRE_STATIC enabled, and same goes when the internal/bundled PCRE copy is used; otherwise (and on non-Windows), we build without PCRE_STATIC. With blessings, thank you!
Created attachment 356461 [details] [review] meson: Don't define intmax_t on Visual Studio if it is not found Hi, For Visual Studio builds, since intmax_t is used to build the gnulib implementation of the *printf() functions only, we must not define intmax_t ourselves when intmax_t is not found (on Visual Studio 2008, where there is no stdint.h), otherwise we will break the build due to conflicting definitions, as the gnulib sources already defines intmax_t for us in such cases. With blessings, thank you!
Created attachment 356462 [details] [review] meson: Fix "installation" in Visual Studio builds Hi, We need to exclude items in the install which are not relevant/usable under Visual Studio builds, as: -They cannot be used with our builds under Visual Studio, and are irrelevant. -They cause breakage during the "install" phase, as the tools required during post-installation is not available normally. With blessings, thank you!
Review of attachment 356462 [details] [review]: ::: gio/meson.build @@ +694,3 @@ #endif +if cc.get_id() != 'msvc' I think using the compiler here to detect the platform is a bit wrong. Meson should let you distinguish cygwin/msys2 cases where you have Bash installed. Maybe this should be gated on finding bash installed, e.g.: if find_program('bash', required: false).found() # install completion files endif ::: glib/meson.build @@ +280,3 @@ +if cc.get_id() != 'msvc' + install_data('gtester-report', install_dir : get_option('bindir')) gtester-report is something that does not relate to the compiler, and I expect Windows users to be able to use it. @@ +281,3 @@ +if cc.get_id() != 'msvc' + install_data('gtester-report', install_dir : get_option('bindir')) + install_data('glib_gdb.py', install_dir : join_paths(glib_pkgdatadir, 'gdb')) This should be gated on having GDB installed. ::: gobject/meson.build @@ +106,3 @@ +if cc.get_id() != 'msvc' + install_data('gobject_gdb.py', install_dir : join_paths(glib_pkgdatadir, 'gdb')) Same as above: you should check if GDB is installed. ::: meson.build @@ +1567,2 @@ # Install glib-gettextize executable +if cc.get_id() != 'msvc' Same as above: using the compiler used to build GLib to detect the features GLib exposes feels wrong.
Review of attachment 356343 [details] [review]: ::: meson.build @@ +1407,3 @@ + return 0; + }''', + pcre_static = cc.links('''#define PCRE_STATIC This should be `dependencies: pcre`.
Created attachment 356493 [details] [review] meson: Only install items relevant to the host system Hi, This checks for the presence of sh, bash, Valgrind and GDB to determine whether items that depend on these programs and tools should be installed. This will fix installation errors that are caused by the inability to run the post-install items due to the tools needed being unavailable, and these items would have not been usable anyways. With blessings, thank you!
Created attachment 356497 [details] [review] Meson: Don't hardcode -DPCRE_STATIC when building GLib (take ii) Hi Emmanuele, I hope I understood you correctly for attachment 356343 [details] [review], as this check is essentially on how we compile the PCRE-using code on Windows so that it links correctly, if we are using an existing installation of it. With blessings, thank you!
Created attachment 357612 [details] [review] Meson: Use find_library() on Visual Studio for ZLib, PCRE and DBus Hi, Many of the dependencies build system for Visual Studio do not provide a pkg-config file, so we use find_library() for them when building against Visual Studio, which would simplify the process for people so that they won't have to manually-craft pkg-config files for them for them to be found, which could be error-prone. For those that do provide a pkg-config file, we leave them as is. With blessings, thank you!
Comment on attachment 357612 [details] [review] Meson: Use find_library() on Visual Studio for ZLib, PCRE and DBus I don't know if we should make assumptions on the presence/absence of pkg-config files based on the compiler ID? Also, is the 'version: '>= 1.2.3' kwarg meaningful in combination with cc.find_library() ?
Hi Tim-Philip, I agree with you on this, but since this is from my regular builds of the dependencies sources using the standard build files for Visual Studio, this would be the best way I could think of about this situation, as most packages (u.e. Non-QT) would either support a combination of autotools + MSVC projects/NMake Makefiles, or use CMake, which most probably does not support generating pkg-config files. For those that generate the .pc files, I would still go by the pkg-config route. The version kwarg is probably not meaningful, but since it is accepted, I think I will just leave it in there in case that it someday does. With blessings, thank you!
(In reply to Fan, Chun-wei from comment #27) > Created attachment 356461 [details] [review] [review] > meson: Don't define intmax_t on Visual Studio if it is not found > > For Visual Studio builds, since intmax_t is used to build the gnulib > implementation of the *printf() functions only, we must not define intmax_t > ourselves when intmax_t is not found (on Visual Studio 2008, where there is > no stdint.h), otherwise we will break the build due to conflicting > definitions, as the gnulib sources already defines intmax_t for us in such > cases. > I thought we decided to not support Visual Studio 2008? :) Meson cannot guarantee that it will work reliably with that version of VS since we don't have any CI for it. I think we should find some other solution for this. (In reply to Fan, Chun-wei from comment #35) > The version kwarg is probably not meaningful, but since it is accepted, I > think I will just leave it in there in case that it someday does. > The version: kwarg will never be meaningful with cc.find_library() simply because there is no way to figure out the version of a project from the shared/static library. The DLL version is not necessarily the same as the project version, for instance. Also starting with meson 0.42.0 we warn about unknown kwargs, and this will become an error in the future.
Hi Nirbheek, (In reply to Nirbheek Chauhan from comment #36) > (In reply to Fan, Chun-wei from comment #27) > I thought we decided to not support Visual Studio 2008? :) > > Meson cannot guarantee that it will work reliably with that version of VS > since we don't have any CI for it. I think we should find some other > solution for this. I see, so probably that's why we will stick on to the Visual Studio projects, so I think I will pull that patch. > (In reply to Fan, Chun-wei from comment #35) > > The version: kwarg will never be meaningful with cc.find_library() simply > because there is no way to figure out the version of a project from the > shared/static library. The DLL version is not necessarily the same as the > project version, for instance. I see. Perhaps the better way is to try to look for the .pc files first, and use find_library() as a fallback on Visual Studio? Any other things I could do for the other patches? With blessings, thank you!
Created attachment 357699 [details] [review] Meson: Use find_library() on Visual Studio for ZLib, PCRE and DBus if needed Hi, This is another attempt to streamline finding the dependencies that are initially found using pkg-config, due to the situation that those .pc files are often not generated/installed during a standard Visual Studio build of these dependencies, namely ZLib, PCRE and DBus. We will first still look for pkg-config files in all cases, and fallback to using find_library() if the pkg-config file for the dependency cannot be found. This is structured in a way so that if fallbacks are needed on other platforms/compilers we may support, they could be added as well. This makes the comments more clear on how we obtain the names of the .lib files. p.s. I think we need to ensure that the Meson builds of ZLib does copy the ZLib DLL, headers and import library, when the fallback/bundled ZLib is used, otherwise GIO-using programs will not run due to a missing ZLib DLL. With blessings, thank you!
Created attachment 357703 [details] [review] meson: Install msvc_recommended_pragmas.h on Windows builds Hi, This installs the msvc_recommended_pragmas.h helper header on Windows builds so that people using those builds in Visual Studio can force-include it to silence unwanted compiler noise, as well as flagging potentially-problematic issues in the code. With blessings, thank you!
Created attachment 357704 [details] [review] Meson: Use find_library() on Visual Studio for ZLib, PCRE and DBus if needed (re-upload) Hi, Sorry, I uploaded the wrong patch for "use find_library() as a fallback on Visual Studio builds for ZLib, PCRE and DBus". With blessings, thank you!
(In reply to Fan, Chun-wei from comment #37) > (In reply to Nirbheek Chauhan from comment #36) > > I thought we decided to not support Visual Studio 2008? :) > > > > Meson cannot guarantee that it will work reliably with that version of VS > > since we don't have any CI for it. I think we should find some other > > solution for this. > > I see, so probably that's why we will stick on to the Visual Studio > projects, so I think I will pull that patch. > Yes, I think it makes sense to stick with the existing VS projects for older VS versions, and work on fixing our stack to not need those.
Review of attachment 357704 [details] [review]: LGTM
Review of attachment 357703 [details] [review]: Good idea; should be useful for gstreamer too.
Review of attachment 356493 [details] [review]: ::: glib/meson.build @@ +293,3 @@ + install_dir: join_paths(get_option('datadir'), 'gdb/auto-load' + glib_libdir) + ) +endif What's the harm in installing these? gdb may not be available in PATH. ::: meson.build @@ +1596,3 @@ + install_data('glib.supp', + install_dir : join_paths(get_option('datadir'), 'glib-2.0', 'valgrind')) +endif Same, valgrind may not be in PATH, and this is a useful file to have.
Review of attachment 356497 [details] [review]: Out of interest, why would someone prefer using their own static pcre instead of the built-in one?
Review of attachment 356334 [details] [review]: It's quite unfortunate that this is still a problem with VS 2017. Do you think you could figure out and document exactly what is missing so we can move away from this in the future? Or is that too much work?
Review of attachment 356335 [details] [review]: ::: meson.build @@ +346,3 @@ + return 0; + }''', + args: '-liphlpapi', Is it necessary to do these checks manually or would the following also work? cc.has_function('if_nametoindex', prefix : '#define _WIN32_WINNT 0x0600\n[...]', dependencies : cc.find_library('iphlapi'))
Review of attachment 356334 [details] [review]: Hi Nirbheek, Thanks, I pushed the patch as 7252893, with the parts missing in Visual Studio 2015/2017's CRT for their *printf() implementations. Namely, from glib/tests/test-printf.c: -The %n format specifier crashes, probably meaning the %n format specifier is not supported or Visual Studio expects it to be in another format. -The positional parameters are supported via different functions[1], which may or may not be enough for our purposes. [1]: https://msdn.microsoft.com/en-us/library/bt7tawza.aspx With blessings, thank you!
Review of attachment 356497 [details] [review]: Hi Nirbheek, Vanilla PCRE on Visual Studio is built via CMake, which defaults to a static PCRE unless the shared/DLL build option is enabled for the build, and the PCRE that is bundled with the GLib sources is probably not up-to-date. Another reason is that there is an initiative to drop the built-in PCRE eventually, please see bug 740573. With blessings, thank you!
Review of attachment 357703 [details] [review]: Hi Nirbheek, Thanks, I pushed the patch as 79b84ba. With blessings, thank you!
Review of attachment 357704 [details] [review]: Hi Nirbheek, Thanks, I pushed the patch as 32d6a76. With blessings, thank you!
Created attachment 357789 [details] [review] Meson: Set _WIN32_WINNT (take iv) Hi Nirbheek, Indeed what you suggested does work, so I re-worked things a bit here. With blessings, thank you!
Review of attachment 356497 [details] [review]: Thanks for the clarification!
Review of attachment 357789 [details] [review]: ::: meson.build @@ +339,3 @@ + cc.has_function('if_indextoname', + prefix : '#define _WIN32_WINNT 0x0601\n#include <winsock2.h>\n#include <iphlpapi.h>', + dependencies : iphlpapi_dep) You still have to set the configuration data when you do this :) See the `foreach f : functions` block below.
Review of attachment 356497 [details] [review]: Hi Nirbheek, Thanks, I pushed the patch as ea6ac5f. With blessings, thank you!
Created attachment 357791 [details] [review] Meson: Set _WIN32_WINNT (take v) Hi Nirbheek, Oops, I missed that! :) With blessings, thank you!
Review of attachment 357791 [details] [review]: LGTM!
Review of attachment 357791 [details] [review]: Hi Nirbheek, Thanks, I pushed the patch as 54aee1f. With blessings, thank you!
Created attachment 358216 [details] [review] build: Don't hardcode G_HAVE_GNUC_VARARGS in glib/glibconfig.h.in Hi, Not all compilers that we support support GNU-style vararg macros, and we already check for them during configuration, so we ought to use the results of the checks instead of assuming that it is always available. We still guard this macro (if defined) to be used when we are not on Visual Studio, as there are people using msys2 builds of GLib in their MSVC projects. With blessings, thank you!
Created attachment 358217 [details] [review] build: Don't hardcode G_HAVE_GNUC_VISIBILITY in glib/glibconfig.h.in Hi, Like the previous patch, we need to check whether the compiler supports the -fvisibility=hidden compiler flag before we enable G_HAVE_GNUC_VISIBILITY in glib/glibconfig.h, instead of assuming that it is always there. This will fix GTK+-2.24.x on Visual Studio that makes use of the Meson-built GLib. Again, like the previous patch, we only enable G_HAVE_GNUC_VISIBILITY when we are not on Visual Studio, as people may well use MSYS2 builds of GLib in MSVC projects. With blessings, thank you!
Hi Nirbheek, (In reply to Nirbheek Chauhan from comment #44) > Review of attachment 356493 [details] [review] [review]: > > ::: glib/meson.build > @@ +293,3 @@ > + install_dir: join_paths(get_option('datadir'), 'gdb/auto-load' + > glib_libdir) This install_dir becomes a problem on Visual Studio builds, which is done in a form of cmd.exe... Windows does not allow a ":" in paths, which glib_libdir will clearly contain, causing the install to fail with WinError 123. I am doubtful that GDB helpers would be useful for Visual Studio builds, so I am open to ideas on what is the best way to deal with this in Visual Studio builds, like only install when we are not on Visual Studio (which probably is not the best way? With blessings, thank you!
Review of attachment 358216 [details] [review]: ::: meson.build @@ +1184,3 @@ +#ifndef _MSC_VER +# define G_HAVE_GNUC_VARARGS 1 +#endif''') Since this is a compile-time check anyway, why not put it directly in glibconfig.h.in?
Review of attachment 358217 [details] [review]: ::: meson.build @@ +124,3 @@ +# define G_HAVE_GNUC_VISIBILITY 1 +#endif''') + endif Again, this can go directly into glibconfig.h.in
(In reply to Fan, Chun-wei from comment #61) > Hi Nirbheek, > > (In reply to Nirbheek Chauhan from comment #44) > > Review of attachment 356493 [details] [review] [review] [review]: > > > > ::: glib/meson.build > > @@ +293,3 @@ > > + install_dir: join_paths(get_option('datadir'), 'gdb/auto-load' + > > glib_libdir) > > This install_dir becomes a problem on Visual Studio builds, which is done in > a form of cmd.exe... Windows does not allow a ":" in paths, which > glib_libdir will clearly contain, causing the install to fail with WinError > 123. How does glib_libdir contain `:`? I am confused. `:` is not allowed in paths on Linux either. > I am doubtful that GDB helpers would be useful for Visual Studio > builds, so I am open to ideas on what is the best way to deal with this in > Visual Studio builds, like only install when we are not on Visual Studio > (which probably is not the best way? > The PDB file format has been documented now by the clang folks[1], so it is quite possible that GDB will add support for it. I must admit that it is probably not something to worry about, but I would like to understand what is going wrong while installing a bunch of Python files before we disable it. 1. http://blog.llvm.org/2017/08/llvm-on-windows-now-supports-pdb-debug.html
Hi Nirbheek, (In reply to Nirbheek Chauhan from comment #64) > > How does glib_libdir contain `:`? I am confused. `:` is not allowed in paths > on Linux either. It seems to me that glib_libdir is constructed with the prefix that we pass in, so on cmd.exe builds (and most probably Visual Studio project builds), we would likely do something like --prefix=c:/foo, which means that the ':' will end up in glib_libdir, which would cause WinError 123 from Python (invalid path). This would not happen on *NIX because paths on *NIX don't go in this pattern, i.e. /foo. I have no idea on how the GDB helpers work, so I am not sure whether we could just do like (not necessarily correct Meson syntax here :)): join_paths(get_option('datadir'), 'gdb/auto-load' + glib_libdir.replace(':', '/'). p.s. For the Valgrind part, we could go ahead to install those helpers, but they won't be useful on Windows for the moment since Windows is not supported by it. My take on this. With blessings, thank you!
Created attachment 359700 [details] [review] glib/glibconfig.h.in: Define G_HAVE_GNUC_VARARGS and G_HAVE_GNUC_VISIBILITY on non-MSVC only Hi, This updates glib/glibconfig.h to only define G_HAVE_GNUC_VARARGS and G_HAVE_GNUC_VISIBILIY when we are not on Visual Studio, since these are for GCC/CLang only (and possibly other supported compilers). With blessings, thank you!
Created attachment 359701 [details] [review] build: Dist the other required Meson build files Hi, (Disclaimer: I am a bit unsure, as there are some files listed under EXTRA_DIST for the Meson files, but did not end up in the tarball, somehow...) There were some Meson-related files that were needed that were actually not shipped with the tarball generated with 'make dist', namely (the latter 3 are in the EXTRA_DIST, but were somehow not found in the tarball): glib/glibconfig.h.in glib/gnulib/meson.build tests/meson.build tests/gobject/meson.build tests/refcount/meson.build With blessings, thank you!
Review of attachment 359700 [details] [review]: LGTM
(In reply to Fan, Chun-wei from comment #67) > Created attachment 359701 [details] [review] [review] > build: Dist the other required Meson build files > > Hi, > > (Disclaimer: I am a bit unsure, as there are some files listed under > EXTRA_DIST for the Meson files, but did not end up in the tarball, > somehow...) > > There were some Meson-related files that were needed that were actually not > shipped with the tarball generated with 'make dist', namely (the latter 3 > are in the EXTRA_DIST, but were somehow not found in the tarball): > > glib/glibconfig.h.in > glib/gnulib/meson.build > tests/meson.build > tests/gobject/meson.build > tests/refcount/meson.build > There's a missing \ in the list, which is why the last three were missing: gthread/meson.build \ po/meson.build tests/refcount/meson.build \ tests/meson.build \ tests/gobject/meson.build \ Could you also add that change to this patch?
(In reply to Fan, Chun-wei from comment #65) > Hi Nirbheek, > > (In reply to Nirbheek Chauhan from comment #64) > > > > How does glib_libdir contain `:`? I am confused. `:` is not allowed in paths > > on Linux either. > > It seems to me that glib_libdir is constructed with the prefix that we pass > in, so on cmd.exe builds (and most probably Visual Studio project builds), > we would likely do something like --prefix=c:/foo, which means that the ':' Ah, I missed that there was string concatenation used there. That will definitely lead to the wrong path being used. The code should be: join_paths(get_option('datadir'), 'gdb', 'auto-load', get_option('libdir')) get_option('xxxdir') will always return a value relative to the prefix, which is actually what you want here. glib_libdir contains prefix, which makes the current path wrong on all platforms (even Linux). > > p.s. For the Valgrind part, we could go ahead to install those helpers, but > they won't be useful on Windows for the moment since Windows is not > supported by it. > Yes, we should skip the Valgrind bits on Windows (not just MSVC).
Review of attachment 359700 [details] [review]: Hi Nirbheek, The patch was pushed as 4ba41db (glib-2-54) and 4c417c4 (master). Thanks for the review. With blessings!
Created attachment 359750 [details] [review] build: Fix dist of Meson build files Hi, This is to remedy for the missed Meson build files that are required for the build. With blessings, thank you!
Created attachment 359751 [details] [review] meson: Only install items relevant to the host system, and fix the installation Hi, This is the updated patch to fix the "install" stage. With blessings, thank you!
Review of attachment 359751 [details] [review]: LGTM, thanks :)
Review of attachment 359750 [details] [review]: LGTM, with this we can close this bug, I think?
Hi Nirbheek, Thanks, I pushed the patches as follows: -Attachment 359750 [details]: fcf26cb (glib-2-54), 65d6990 (master) -Attachment 359751 [details]: 915ab35 (glib-2-54), a7a6449 (master) (In reply to Nirbheek Chauhan from comment #75) > LGTM, with this we can close this bug, I think? I will close this bug shortly; however, I think Christoph has some more items that are Windows-related at bug 784995 (.rc files and MSYS2-related items; the MSYS2 items are items that I am not that familiar with) that would also look for reviews/suggestions. With blessings, thank you!
(In reply to Fan, Chun-wei from comment #76) > Hi Nirbheek, > > Thanks, I pushed the patches as follows: > -Attachment 359750 [details]: fcf26cb (glib-2-54), 65d6990 (master) > -Attachment 359751 [details]: 915ab35 (glib-2-54), a7a6449 (master) > > (In reply to Nirbheek Chauhan from comment #75) > > LGTM, with this we can close this bug, I think? > > I will close this bug shortly; however, I think Christoph has some more > items that are Windows-related at bug 784995 (.rc files and MSYS2-related > items; the MSYS2 items are items that I am not that familiar with) that > would also look for reviews/suggestions. > Sure, I'll take a look at that. You should just CC me on bugs that you think I should look at and I'll get to them eventually. :)