GNOME Bugzilla – Bug 793729
gitlab-ci: Add Windows MinGW support
Last modified: 2018-04-27 10:29:19 UTC
For more info see https://gitlab.gnome.org/Infrastructure/GitLab/issues/141
Created attachment 368785 [details] [review] tests/date.c: include config.h to expose Vista+ macros date.c uses SUBLANG_LITHUANIAN_LITHUANIA which is Vista+ Include config.h so that _WIN32_WINNT is defined and the newer macros are exposed. This fixes the build under MinGW.
Created attachment 368786 [details] [review] meson: Disable -Wformat-nonliteral for the embedded gnulib glib enables -Werror=format-nonliteral by default which is triggered by the embedded gnulib (in vasnprintf.c). Disable that warning for gnulib alone. The gnulib code is there to handle user provided format strings, so the warning doesn't add anything anyway. This fixes the build under MinGW.
Created attachment 368787 [details] [review] gitlab-ci: Add 32bit/64bit MingGW jobs using MSYS2 This builds glib using meson/ninja/ccache with mingw-w64 on a Windows machine. The CI scripts expect a gitlab runner to exist with the "win32" tag which uses the default "cmd" shell by default. Before running the tests pacman is invoked to update the system (potentially including bash etc, thus the extra step) Then a login shell is started with CHERE_INVOKING to not change the cwd and finally the test script is executed.
Created attachment 368788 [details] [review] gitlab-ci: Add 32bit/64bit MingGW jobs using MSYS2 This builds glib using meson/ninja/ccache with mingw-w64 on a Windows machine. The CI scripts expect a gitlab runner to exist with the "win32" tag which uses the default "cmd" shell by default. Before running the tests pacman is invoked to update the system (potentially including bash etc, thus the extra step) Then a login shell is started with CHERE_INVOKING to not change the cwd and finally the test script is executed.
For the Windows runner to be registered with the glib repo it either has to be registered as a global shared one or one of the glib maintainers has to tell me the glib CI token and after I register it enable it for the repo.
The runner now resets the MSYS2 environment between jobs, so the ccache cache needs to be moved to the gitlab-ci cache like with Docker. I'll wait until glib allows merge requests before continuing here.
Emmanuele?
Review of attachment 368785 [details] [review]: Looks good.
Review of attachment 368786 [details] [review]: ::: glib/gnulib/meson.build @@ +1,1 @@ +extra_gnulib_args = [] Could do with a little comment above this block which explains why it’s needed (basically what you have in the commit message).
Review of attachment 368788 [details] [review]: ::: .gitlab-ci/test-msys2.sh @@ +23,3 @@ + mingw-w64-$MSYS2_ARCH-zlib + +meson _build Are there any configure options we want to enable here, like `--buildtype debug --werror` (as in .gitlab-ci.yml for Linux)? @@ +25,3 @@ +meson _build +cd _build +ninja Should probably also run `meson test` after `ninja`.
(In reply to Philip Withnall from comment #10) > Review of attachment 368788 [details] [review] [review]: > > ::: .gitlab-ci/test-msys2.sh > @@ +23,3 @@ > + mingw-w64-$MSYS2_ARCH-zlib > + > +meson _build > > Are there any configure options we want to enable here, like `--buildtype > debug --werror` (as in .gitlab-ci.yml for Linux)? Re werror: There are multiple warnings with the current build, I'd like to get this in first before looking into that. Re buildtype=debug: I don't know what that would add test wise. > @@ +25,3 @@ > +meson _build > +cd _build > +ninja > > Should probably also run `meson test` after `ninja`. Not sure if it will pass, I haven't tried yet. ---- Do you know if there is any plan to allow glib merge requests on gitlab.gnome.org?
(In reply to Christoph Reiter (lazka) from comment #11) > (In reply to Philip Withnall from comment #10) > > Review of attachment 368788 [details] [review] [review] [review]: > > > > ::: .gitlab-ci/test-msys2.sh > > @@ +23,3 @@ > > + mingw-w64-$MSYS2_ARCH-zlib > > + > > +meson _build > > > > Are there any configure options we want to enable here, like `--buildtype > > debug --werror` (as in .gitlab-ci.yml for Linux)? > > Re werror: There are multiple warnings with the current build, I'd like to > get this in first before looking into that. OK. Perhaps add a `FIXME: Add --werror` so we don’t forget? > Re buildtype=debug: I don't know what that would add test wise. It disables optimisations and ensures debug info is included: http://mesonbuild.com/Running-Meson.html. > > @@ +25,3 @@ > > +meson _build > > +cd _build > > +ninja > > > > Should probably also run `meson test` after `ninja`. > > Not sure if it will pass, I haven't tried yet. Similarly, perhaps add a FIXME for it. > Do you know if there is any plan to allow glib merge requests on > gitlab.gnome.org? Once we move the open bugs over from Bugzilla. I don’t want to have two ways of submitting changes to GLib allowed at the same time.
Comment on attachment 368785 [details] [review] tests/date.c: include config.h to expose Vista+ macros Now that we have branched for glib-2-56, I have pushed the accepted patch. The other patches still need some rework. Attachment 368785 [details] pushed as 8e315bd - tests/date.c: include config.h to expose Vista+ macros
Any updates on this Christoph?
(In reply to Philip Withnall from comment #12) > Once we move the open bugs over from Bugzilla. I don’t want to have two ways > of submitting changes to GLib allowed at the same time. Allowing MRs doesn't necessarily mean that you have to still accept BZ patches as well. You can move those things before the actual bugs get moved. But that's of course your decision. (In reply to Philip Withnall from comment #14) > Any updates on this Christoph? Thanks for the reminder. Just tried again and there is a build problem with master now, see https://bugzilla.gnome.org/show_bug.cgi?id=794555#c4. I'll look into it.
(In reply to Christoph Reiter (lazka) from comment #11) > (In reply to Philip Withnall from comment #10) > > Should probably also run `meson test` after `ninja`. > > Not sure if it will pass, I haven't tried yet. $ time meson test [...] OK: 105 FAIL: 14 SKIP: 1 TIMEOUT: 7 1m52.479s
Created attachment 371364 [details] [review] meson: Disable -Wformat-nonliteral for the embedded gnulib
Created attachment 371365 [details] [review] meson: pass -fno-builtin when testing whether stpcpy is a builtin In https://bugzilla.gnome.org/show_bug.cgi?id=794555 the tests for posix_memalign and stpcpy were extended to catch the case where the compiler provides an incomplete builtin. Under MSYS2 the example code still compiles and links while the real usage of stpcpy fails to build. To prevent the MSYS2 gcc from using the builtin versions pass -fno-builtin.
Created attachment 371366 [details] [review] gitlab-ci: Add 32bit MinGW jobs using MSYS2
Here is an example run: https://gitlab.gnome.org/creiter/glib/-/jobs/26826 I've removed the 64bit variant for now to now slow CI down too much.
Review of attachment 371364 [details] [review]: ::: glib/gnulib/meson.build @@ +2,3 @@ +# needs to handle user provided format strings. +extra_gnulib_args = [] +if cc.get_id() != 'msvc' Do we need to test for msvc here? Surely the get_supported_arguments() call will just return an empty list for msvc if it doesn’t support -Wno-format-nonliteral.
Review of attachment 371364 [details] [review]: Please also make the equivalent changes in the autotools build system; until we drop autotools, we need to keep feature parity between the two systems.
Review of attachment 371365 [details] [review]: This looks reasonable, but it seems like the kind of thing which Meson should be doing for us. Nirbheek?
Review of attachment 371366 [details] [review]: Emmanuele, how does this look to you? I’m not massively familiar with this stuff. ::: .gitlab-ci/test-msys2.sh @@ +1,1 @@ +#!/bin/bash This file should probably have a copyright/licensing header. @@ +27,3 @@ +export CCACHE_DIR="${CCACHE_BASEDIR}/_ccache" + +# FIXME: Add --werror We’d need to enable --werror and the test suite pretty quickly after merging this, otherwise the Windows CI is not actually telling us anything. The only reason for not enabling them immediately would be to avoid lots of e-mails to other patch authors about their patches ‘breaking the build’ when they’re actually just experiencing an existing failure.
(In reply to Christoph Reiter (lazka) from comment #15) > (In reply to Philip Withnall from comment #12) > > Once we move the open bugs over from Bugzilla. I don’t want to have two ways > > of submitting changes to GLib allowed at the same time. > > Allowing MRs doesn't necessarily mean that you have to still accept BZ > patches as well. You can move those things before the actual bugs get moved. > But that's of course your decision. I think it would complicate things too much to have patches in one place and bugs in another, and people would continue to attach patches to Bugzilla anyway. We’ve had Bugzilla for years; another few weeks can’t hurt. :-) > (In reply to Philip Withnall from comment #14) > > Any updates on this Christoph? > > Thanks for the reminder. Just tried again and there is a build problem with > master now, see https://bugzilla.gnome.org/show_bug.cgi?id=794555#c4. I'll > look into it. I think that became attachment #371365 [details].
Review of attachment 371366 [details] [review]: It looks generally good to me. We don't really have copyright/license headers for build scripts, as every file in the project automatically acquires the license of the project unless stated otherwise; if that didn't apply, we'd have to add a copyright/license header to everything, including Meson and Autotools files. I'd enable the --werror and tests bits as soon as we have a reliable build that does not break CI, and before actually merging these patches, but since there is a noticeable delay between fixing the build on Windows and applying this patch from Bugzilla to Git, I understand the FIXMEs. I'd land this as is, and turn --werror and the tests on after we move code submissions to GitLab.
About attachment 371364 [details] [review]: Could you show the warning that is fixed by this? This could be related to bug 725470. About attachment 371365 [details] [review]: My original patch worked for me, but then my version of gcc is somewhat old. If an extra flag fixes this for other people, i don't see anything wrong with that.
(In reply to LRN from comment #27) > About attachment 371364 [details] [review] [review]: > Could you show the warning that is fixed by this? This could be related to > bug 725470. https://bpaste.net/show/d5c8cb78ba8a (In reply to Philip Withnall from comment #21) > Review of attachment 371364 [details] [review] [review]: > > ::: glib/gnulib/meson.build > @@ +2,3 @@ > +# needs to handle user provided format strings. > +extra_gnulib_args = [] > +if cc.get_id() != 'msvc' > > Do we need to test for msvc here? Surely the get_supported_arguments() call > will just return an empty list for msvc if it doesn’t support > -Wno-format-nonliteral. I don't think it is strictly needed but it's what I saw most meson projects doing (glib included). It adds some documentation value in that it specifies for which compiler it's used for and doesn't result in random flags being passed to msvc (and shown in the meson output). But I don't think it changes anything for the build.
(In reply to Christoph Reiter (lazka) from comment #28) > (In reply to LRN from comment #27) > > About attachment 371364 [details] [review] [review] [review]: > > Could you show the warning that is fixed by this? This could be related to > > bug 725470. > > https://bpaste.net/show/d5c8cb78ba8a Okay, that's definitely something i haven't seen before. The fix to make gcc ignore that warning is correct - the code there is written in such a way that it does, indeed, pass non-literal strings to sprintf. As for the method for ignoring that warning, perhaps a simple >#if defined __GNUC__ ># pragma GCC diagnostic push ># pragma GCC diagnostic ignored "-Wno-format-nonliteral" >#endif >... >#if defined __GNUC__ ># pragma GCC diagnostic pop >#endif in vasnprintf.c (with appropriate additions for clang, if it has such a warning) would be better suited?
(In reply to LRN from comment #29) > in vasnprintf.c (with appropriate additions for clang, if it has such a > warning) would be better suited? From the README I see that this file is just a copy. Wouldn't such changes make future updates/syncs harder?
(In reply to Christoph Reiter (lazka) from comment #30) > (In reply to LRN from comment #29) > > in vasnprintf.c (with appropriate additions for clang, if it has such a > > warning) would be better suited? > > From the README I see that this file is just a copy. Wouldn't such changes > make future updates/syncs harder? I have a feeling that > #include "glib/galloca.h" in that file was not there in the original gnulib sources...
(In reply to Christoph Reiter (lazka) from comment #30) > (In reply to LRN from comment #29) > > in vasnprintf.c (with appropriate additions for clang, if it has such a > > warning) would be better suited? > > From the README I see that this file is just a copy. Wouldn't such changes > make future updates/syncs harder? You are right. I would prefer to keep the delta from upstream gnulib minimal here; so would prefer the solution from attachment #371364 [details]. (Actually, the best thing here would be to get a fix into upstream gnulib and then backport it, but that’s a little outside the scope of this task.)
Created attachment 371379 [details] [review] meson: Disable -Wformat-nonliteral for the embedded gnulib removed the msvc guard
Created attachment 371380 [details] [review] ci: fix warnings and enable --werror for the mingw build Fix various warnings regarding unused variables, duplicated branches etc by adjusting the ifdeffery and some missing casts. gnulib triggers -Wduplicated-branches in one of the copied files, disable as that just makes updating the code harder. The warning indicating missing features are made non fatal through pragmas. They still show but don't abort the build.
last run: https://gitlab.gnome.org/creiter/glib/-/jobs/26888
Review of attachment 371379 [details] [review]: ++
Review of attachment 371380 [details] [review]: Seems reasonable, thanks!
Thanks! I've rebased and tested again and pushed everything to master.
(In reply to Christoph Reiter (lazka) from comment #38) > Thanks! I've rebased and tested again and pushed everything to master. Great! Can you file one or more follow-up bugs for fixing up the tests and (eventually) removing the `|| true` from `meson test`? I also wonder if there should be an `artifacts` section in the job `msys2-mingw32` description so we can extract test logs on failure.
(In reply to Philip Withnall from comment #39) > (In reply to Christoph Reiter (lazka) from comment #38) > > Thanks! I've rebased and tested again and pushed everything to master. > > Great! Can you file one or more follow-up bugs for fixing up the tests and > (eventually) removing the `|| true` from `meson test`? > > I also wonder if there should be an `artifacts` section in the job > `msys2-mingw32` description so we can extract test logs on failure. See bug 795569
(In reply to Philip Withnall from comment #23) > Review of attachment 371365 [details] [review] [review]: > > This looks reasonable, but it seems like the kind of thing which Meson > should be doing for us. Nirbheek? Yes, meson should handle this even though it's a toolchain bug. However, the better fix is to use has_function() with `#include <theheader.h>` in the `prefix:` argument. That should be always correct. If not, that's a bug in Meson.
(In reply to Nirbheek Chauhan from comment #41) > (In reply to Philip Withnall from comment #23) > > Review of attachment 371365 [details] [review] [review] [review]: > > > > This looks reasonable, but it seems like the kind of thing which Meson > > should be doing for us. Nirbheek? > > Yes, meson should handle this even though it's a toolchain bug. > > However, the better fix is to use has_function() with `#include > <theheader.h>` in the `prefix:` argument. That should be always correct. If > not, that's a bug in Meson. I think that was deliberately avoided in commit dcc452b5f90f003d7b5569cd8f260f7cff0954ba / bug #794555.