GNOME Bugzilla – Bug 711047
Enable the build of the various test programs on Windows/MSVC
Last modified: 2013-11-26 11:57:49 UTC
Hi, As neither Han's NMake Makefiles nor the Visual Studio projects build the test programs of GLib (GLib, GObject and GIO included), I thought it might be a good idea to enable the building of those test programs, so that one may be able to run the test programs against Visual Studio builds of GLib to see how the builds fare there. So, I am doing some NMake Makefiles for this purpose, as I didn't want to crowd up the Project Files and probably not all would like to see a bunch of .exe's being "installed" after the build of GLib. People can then use the NMake Makefiles to build them, if they need to. Plus, I am also proposing some updates to some of the test programs as some of them unconditionally include unistd.h, which is unfortunately not available on Visual Studio, and also other various updates/fixes. With blessings, thank you!
Created attachment 258422 [details] [review] Add Visual Studio Makefiles for the test programs This adds a set of NMake Makefiles to be used by Visual Studio to build the various test programs supplied by GLib. There are some tests that cannot be built by Visual C++ as they define/use empty structs (VLA feature?) or have tokens which have lengths beyond what Visual C++ likes.
Created attachment 258423 [details] [review] Test Programs: Don't unconditionally include unistsd.h There were some test programs that included unistd.h unconidtionally, which is not available on Visual C++, so update the code to only include that when: -We aren't on Windows -or- -We aren't using Visual C++
Created attachment 258429 [details] [review] tests: Fix up the expected messages There were also some messages that don't have the same format on Windows, so update the expected messages to accommodate for them while testing for the same expected error/warning scenarios.
Created attachment 258431 [details] [review] Fix the gsubprocess test on Windows The gsubprocess test program was checking for the output of hello world! in test_echo1(), but the test fails on Windows as Windows uses "\r\n" as line ending as opposed to "\n" on other systems, so account for that. Also, as the cat-eof and multi1 tests seems to be Unixy, judging by their use of the cat utility (which had trouble running on Windows), probably we should skip these on Windows as well.
Created attachment 258433 [details] [review] Skip the spawn-script test on Windows It also seems that the spawn-script test is Unixy to me, as I had trouble running it even under an MSYS shell, so probably we want to skip that too on Windows.
Created attachment 258434 [details] [review] Fix the tests using close() on fd's for newer Microsoft CRTs There were some tests that called close() on a fd variable more than once, which the paranoid Microsoft CRTs (2005/8.0+) doesn't like, which causes the programs to crash due to an internal call on abort() from the CRT. Also, file-test.c also involved calls to close() on invalid fd's, which the newer Microsoft CRTs also does not like unless we define an empty invalid parameter handler to deal with this. So this patch addresses these issue by defining an empty invalid parameter function when necessary and using a GSList to store up the fd's to pass to close() and close the fd's at the end by running through that GSList.
Created attachment 258436 [details] [review] Update the function prototypes in tests/libmoduletestplugins_*.c This fixes the build of tests/libmoduletestplugins_a.c and tests/libmoduletestplugins_b.c on Visual C++ as their function declarations did not match the corresponding prototypes (as the prototypes did not have a D_MODULE_EXPORT annotation), which the compiler does not like.
Created attachment 258437 [details] [review] Fix the gresource test on Windows Sorry, The D_MODULE_EXPORT should read G_MODULE_EXPORT (the patch there has that correct). This is the fix to the GResource test on Windows as we need to use g_content_type_get_mime_type() to lookup the mime type of the file from the registry as g_file_info_get_content_type() does not acquire the mime type for the caller on Windows. This will enable the GResource test program to pass on Windows. This is the last of my proposed patch in this bug for now. With blessings, thank you!
Comment on attachment 258423 [details] [review] Test Programs: Don't unconditionally include unistsd.h >+#if !defined (G_OS_WIN32) || !defined (_MSC_VER) > #include <unistd.h> >+#elif defined (_MSC_VER) >+#include <basetsd.h> >+typedef SSIZE_T ssize_t; >+#endif bug 710519 has a patch (attachment 257722 [details] [review], although it probably depends on the earlier patches in that bug) to remove HAVE_UNISTD_H and change all unistd.h includes to be #ifdef G_OS_UNIX. In general, I think it's cleanest to completely ignore unistd.h under mingw, and to use whatever native Windows headers are appropriate on all Windows builds, both MSC and mingw. Often, this ends up being: #ifdef G_OS_UNIX #include <unistd.h> #endif #ifdef G_OS_WIN32 #include <io.h> #endif (So, eg, in the quoted part above, it would be better to include some header that typedefs ssize_t for you rather than doing it manually.)
Comment on attachment 258429 [details] [review] tests: Fix up the expected messages > g_test_expect_message (G_LOG_DOMAIN, G_LOG_LEVEL_WARNING, >- "*g_credentials_get_native: Trying to get*" >- "credentials but*no support*"); >+ "*g_credentials_get_native: Trying to get " >+ "credentials *but*no support*"); oops. I tested that under mingw but I must have reworded the error message after that... > g_test_expect_message (G_LOG_DOMAIN, G_LOG_LEVEL_CRITICAL, >- "g_markup_parse_context_end_parse: assertion 'context->state != STATE_ERROR' failed"); >+ "*: assertion 'context->state != STATE_ERROR' failed"); what does it print under MSC?
Comment on attachment 258434 [details] [review] Fix the tests using close() on fd's for newer Microsoft CRTs We should not be close()ing the same fd more than once on unix either. If the test program does that then it's a mistake.
Comment on attachment 258422 [details] [review] Add Visual Studio Makefiles for the test programs It's unfortunate to have all this duplication... it would be better if, say, there was a script that could generate the file lists in the nmake files from the Makefile.am's... >+# gvariant$(EXEEXT) \ #does not compile on MSVC (use of empty structure) where is the empty struct? empty structs are a gcc-ism. we should be getting rid of them anyway >+# mem-overflow$(EXEEXT) \ #does not compile on MSVC (use of empty structure) We apparently have an open bug about this, bug 641350. The test should be #ifdef __GNUC__ >+# strfuncs$(EXEEXT) \ #does not compile on MSVC (line 986: C1064 token overflow internal buffer) I'd just make that one particular test be #ifndef MSC, not skip the whole program >+# param$(EXEEXT) \ #does not compile on MSVC (use of empty structure) should be fixed
(everything else looks good as far as i can tell)
Hi Dan, Thanks for the reviews and suggestions. (In reply to comment #11) > We should not be close()ing the same fd more than once on unix either. If the > test program does that then it's a mistake. Apparently I didn't do a good job conveying the message for the fd issue... What was happening on msvcr80(+).dll (the Microsoft 8.0/2005 CRT with its debug variants and later versions of it) is, suppose I am doing this, for example: int fd = open ("NUL:", O_RDONLY); ... /* some operations on the fd */ close (fd); /* this will do ok */ fd = open ("NUL:", O_WRONLY); ... /* some other operations on the fd */ close (fd); /* this will cause the Microsoft CRT to abort the program */ What I meant was, the CRT does not like, for some reason being paranoid due to security reasons, calling close on the fd variable even though it might hold a legitimate value for the close() call. It also (for more plausible reasons), doen't like it when an invalid fd value is passed to close(). What happens as a result is the CRT internally calls abort(), which causes the program to crash. That was the case for gspawn-win32-helper.c, so that had to be fixed for items like glib-compile-resources.exe to run when built against newer Microsoft CRTs, which was in commit c7996825. This is also why an empty invalid parameter handler needs to be defined to prevent the program from aborting when an illegit fd was passed in or when _get_osfhandle() is used to check the validity of the fd as the fd passed into _get_osfhandle() may well be invalid. So it could be so in the test program where the empty invalid parameter handler is defined means that it passed in an illegit fd to close() by mistake (or I messed up in the conversion to use the data structures to stash the fd's). I will look closer in your other suggestions in the next few days though. Thank you! With blessings.
(In reply to comment #14) > What I meant was, the CRT does not like, for some reason being paranoid due to > security reasons, calling close on the fd variable even though it might hold a > legitimate value for the close() call. Sorry again (probably getting late here in this part of this world) :|, the "calling close on the fd variable" should read "calling close() on the same fd variable". DOH. With blessings, thank you!
Hi Dan, (In reply to comment #9) > In general, I think it's cleanest to completely ignore unistd.h under mingw, > and to use whatever native Windows headers are appropriate on all Windows > builds, both MSC and mingw. Often, this ends up being: > > #ifdef G_OS_UNIX > #include <unistd.h> > #endif > #ifdef G_OS_WIN32 > #include <io.h> > #endif > > (So, eg, in the quoted part above, it would be better to include some header > that typedefs ssize_t for you rather than doing it manually.) I agree with you on the unistd.h regard, so I think maybe it might be the best to have a small private header that is included after including the GLib headers to include unistd.h when G_OS_UNIX is defined and include io.h and process.h when G_OS_WIN32 is defined. However, it's a yes and no situation in terms of ssize_t: MinGW (at least in MinGW64, but also most probably in stock MinGW) defines ssize_t for you in its io.h, but in Visual C++ it does not define ssize_t for you (as far as the shipped headers in Visual C++ and the Windows SDKs is concerned), but rather it defines SSIZE_T (i.e. long/int on Win32/x86 and __int64 for Win64, which is identical to how MinGW does it) for you in basetsd.h, which can be used in turn to typedef ssize_t from it. So, I think it might be best that we only define ssize_t when it is not defined yet in that small private header by checking for SSIZE_T_DEFINED. Some of my thoughts about this. With blessings, thank you!
Created attachment 258512 [details] [review] Clean up the inclusion of unistd.h in glib/tests Hi, This is the patchset for cleaning of the inclusion of unistd.h as per Dan suggestions in comment #9. With blessings, thank you!
Created attachment 258513 [details] [review] Clean up the inclusion of unistd.h in glib/tests Sorry, Reattaching here as I forgot to roll back the previous changes to glib/tests/Makefile.am...
Created attachment 258515 [details] [review] Clean up the inclusion of unistd.h for gio/tests Hi, This is the patchset for cleaning of the inclusion of unistd.h as per Dan suggestions in comment #9 for gio/tests. I did not update the *NIX-only tests which had instances of unistd.h as they needed to include unistd.h anyways. With blessings, thank you!
Created attachment 258518 [details] [review] Clean up the inclusion of unistd.h in tests/ Hi, This is the patchset for cleaning of the inclusion of unistd.h as per Dan suggestions in comment #9 for gio/tests.
Created attachment 258520 [details] [review] Clean up the inclusion of unistd.h in tests/refcount Hi, This is the patchset for cleaning of the inclusion of unistd.h as per Dan's suggestions in comment #9 for tests/refcount. This builds upon the previous patch.
Created attachment 258521 [details] [review] Fix tests/testglib.c and tests/file-test.c for newer Microsoft CRTs Hi, This patch addresses the issues pointed out in comments #11, #14 (and #15) regarding the call on close(fd). Please see these comments for a more detailed explanation about this. This also fixes the file-test.c test program so that it will not try to close a fd whose value is -1 (which is not valid for close()). With blessings, thank you!
Hello Dan, These are what the code prints out from a Visual Studio build: (In reply to comment #10) > (From update of attachment 258429 [details] [review]) > > g_test_expect_message (G_LOG_DOMAIN, G_LOG_LEVEL_WARNING, > >- "*g_credentials_get_native: Trying to get*" > >- "credentials but*no support*"); > >+ "*g_credentials_get_native: Trying to get " > >+ "credentials *but*no support*"); (credentials.exe:1172): GLib-GIO-WARNING **: Did not see expected message WARNIN G **: *g_credentials_get_native: Trying to get*credentials but*no support* (credentials.exe:1172): GLib-GIO-WARNING **: g_credentials_get_native: Trying to get credentials of type G_CREDENTIALS_TYPE_LINUX_UCRED but there is no support for GCredentials on this platform. > > g_test_expect_message (G_LOG_DOMAIN, G_LOG_LEVEL_CRITICAL, > >- "g_markup_parse_context_end_parse: assertion 'context->state != STATE_ERROR' failed"); > >+ "*: assertion 'context->state != STATE_ERROR' failed"); > > what does it print under MSC? (markup-collect.exe:3804): GLib-CRITICAL **: Did not see expected message CRITIC AL **: g_markup_parse_context_end_parse: assertion 'context->state != STATE_ERRO R' failed (markup-collect.exe:3804): GLib-CRITICAL **: file ..\..\..\glib\gmarkup.c: line 1755: assertion 'context->state != STATE_ERROR' failed Hope this is clear here. With blessings, thank you! p.s. I will look into autogenerating the file listings from the various Makefile.am's in the next few days or so.
(In reply to comment #23) > (markup-collect.exe:3804): GLib-CRITICAL **: file ..\..\..\glib\gmarkup.c: line > 1755: assertion 'context->state != STATE_ERROR' failed Hm... ok, next question is... why? The gmessages macros use G_STRFUNC, and gmacros.h defines G_STRFUNC as "((const char*) (__FUNCTION__))" in Visual C, and the docs (http://msdn.microsoft.com/en-us/library/b0084kay.aspx) say that should expand to the function name.
(In reply to comment #14) > int fd = open ("NUL:", O_RDONLY); > ... /* some operations on the fd */ > close (fd); /* this will do ok */ > > fd = open ("NUL:", O_WRONLY); > ... /* some other operations on the fd */ > close (fd); /* this will cause the Microsoft CRT to abort the program */ How could that possibly be considered anything but a bug in the CRT? Why is it that only gspawn-helper and the test programs run into this problem? If things actually work this way (open() reuses file descriptors, but close() aborts when it sees a reused file descriptor), then you would expect every long-running glib-based program to eventually crash. Eg, all the GLocalFile* classes in gio use g_open()... why don't we get crashes in programs that use those classes? Likewise, g_io_channel_new_file(), g_mkstemp(), g_file_open_tmp(), etc.
Created attachment 258714 [details] [review] tests: Avoid calling close() on the fd when the fd == -1 Hello Dan, (In reply to comment #25) > How could that possibly be considered anything but a bug in the CRT? In fact, you are right-considering the other parts of you mentioned are running OK on my builds. The reason why I saw this as an over-zealous act of the newer Microsoft CRTs is because the same code ran ok in the MinGW builds that Tarnyko built. So, I think this is the better fix for all parties. Sorry for the ignorance, and thanks for this pointer. With blessings, thank you!
Hello Dan, (In reply to comment #24) > > Hm... ok, next question is... why? It seems that the message is generated from g_return_val_if_fail (context->state != STATE_ERROR, FALSE); (gmarkup.c line 1755), and g_return_val_if_fail() expands to: #define g_return_val_if_fail(expr, val) G_STMT_START{ \ if (expr) { } else \ { \ g_log (G_LOG_DOMAIN, \ G_LOG_LEVEL_CRITICAL, \ "file %s: line %d: assertion '%s' failed", \ __FILE__, \ __LINE__, \ #expr); \ return (val); \ }; }G_STMT_END It doesn't seem to me that G_STRFUNC is used in any part of this (nor the function called in the macro). My thoughts on this. With blessings.
Created attachment 258715 [details] [review] Clean up glib/gspawn-win32-helper.c Hello Dan, I thought, as per your last comment (comment #25), I thought it also may be a good idea to clean up gspawn-win32-helper.c a bit-this might be a little bit out-of-place in this bug, but anyways. But since we still need to check the validity of the fd's to close when we do the loop for when __argv[ARG_CLOSE_DESCRIPTORS][0] == 'y', we still need the invalid parameter handler as we use _get_osfhandle() to check whether the fd is legit, as _get_osfhandle() will abort on illegit fd's otherwise[1]. Thanks again for the points, with blessings. [1]: http://msdn.microsoft.com/zh-tw/library/ks2530z6%28v=vs.80%29.aspx
Created attachment 258718 [details] [review] glib/tests/gvariant.c: Avoid GCCism Hello Dan, (In reply to comment #12) > >+# gvariant$(EXEEXT) \ #does not compile on MSVC (use of empty structure) > > where is the empty struct? > > empty structs are a gcc-ism. we should be getting rid of them anyway In serialise_tree(): GVariantSerialised empty = { }; This is the patch that addresses this for glib/tests/gvariant.c, I tested it successfully under Visual Studio builds in x86 and x64 flavors, plus under Fedora 19 x64. It also seems that the inf/-inf/nan tests cannot be run under Visual C++ builds, so probably we should skip it? With blesings, thank you!
Created attachment 258720 [details] [review] Fix tests on non-GCC Hello Dan, (In reply to comment #12) > empty structs are a gcc-ism. we should be getting rid of them anyway > > >+# mem-overflow$(EXEEXT) \ #does not compile on MSVC (use of empty structure) > > >+# strfuncs$(EXEEXT) \ #does not compile on MSVC (line 986: C1064 token overflow internal buffer) > > >+# param$(EXEEXT) \ #does not compile on MSVC (use of empty structure) This is my proposed fix... With blessings, thank you!
(In reply to comment #27) > It seems that the message is generated from g_return_val_if_fail > (context->state != STATE_ERROR, FALSE); (gmarkup.c line 1755), > and g_return_val_if_fail() expands to: OK, so the problem is that g_return_val_if_fail(), etc, are defined differently for gcc and non-gcc. The gcc versions use __PRETTY_FUNCTION__, and the non-gcc ones use __FILE__ and __LINE__. We should just change the gcc versions to use G_STRFUNC instead of __PRETTY_FUNCTION__, and then make everything use those and get rid of the other definitions. And then you won't need the change to glib/tests/markup-collect.c (though you'll still need the change to gio/tests/credentials.c, since that test was just wrong before).
Comment on attachment 258431 [details] [review] Fix the gsubprocess test on Windows >+#ifdef G_OS_WIN32 >+#define LINEEND "\r\n" >+#else >+#define LINEEND "\n" >+#endif kind of surprising we don't define that somewhere publicly already... >+#ifdef G_OS_UNIX > g_test_add_func ("/gsubprocess/cat-eof", test_cat_eof); > g_test_add_func ("/gsubprocess/multi1", test_multi_1); >+#endif test_cat_eof() seems like it ought to theoretically be applicable to windows, with a little bit of rewriting. Maybe test_multi_1() too? If the test *could* be ported to Windows, and just hasn't yet, it might be better to leave the test compiled in, but in test_cat_eof() itself, do #ifdef G_OS_WIN32 g_test_skip ("not yet ported to win32"); return; #endif or something
Comment on attachment 258433 [details] [review] Skip the spawn-script test on Windows >+#if !defined (G_OS_WIN32) > g_test_add_func ("/gthread/spawn-script", test_spawn_script); >+#endif Is g_spawn() expected to be able to spawn batch files? If so, you could rename "echo-script" to "echo-script.bat" and then run this on windows as well. (Unix doesn't care what the script is called, so it's fine for it to have the .bat extension there too.)
Comment on attachment 258513 [details] [review] Clean up the inclusion of unistd.h in glib/tests Hm... I don't like defining the same test-common.h in multiple different directories. I had been thinking more along the lines of just doing the G_OS_UNIX/G_OS_WIN32 #ifdefs inline (as already happens in many non-test files, eg, gfileutils.c). If we weren't going to do that, I'd suggest having an (uninstalled) "gunistd.h" in glib/, and just include that from everywhere that needs it. But I'm not sure I like that approach... Additionally (and I should have noticed this before) any uses of ssize_t in glib should be replaced with gssize, so then we don't need the SSIZE_T stuff.
Comment on attachment 258714 [details] [review] tests: Avoid calling close() on the fd when the fd == -1 > if (fd != -1) >+ { > g_warning ("g_mkstemp works even if template doesn't contain XXXXXX"); indentation needs to be fixed up (here and below)
Comment on attachment 258715 [details] [review] Clean up glib/gspawn-win32-helper.c you ended up accidentally turning a bunch of LFs into CRLFs in this patch > if (i != child_err_report_fd && i != helper_sync_fd) >+ { > if (_get_osfhandle (i) != -1) indentation needs to be fixed
Comment on attachment 258718 [details] [review] glib/tests/gvariant.c: Avoid GCCism >+#ifndef _MSC_VER > /* inf/nan mini test */ If this doesn't work, I would expect the inf/nan tests in test_strtod() in glib/tests/strfuncs.c to also fail. But you didn't comment those out... so g_ascii_strtod() should be working, in which case this test should be working too. Where is it failing exactly?
Comment on attachment 258720 [details] [review] Fix tests on non-GCC >+#ifndef _MSC_VER > d = 179769313486231570814527423731704356798070567525844996598917476803157260780028538760589558632766878171540458953514382464234321326889464182768467546703537516986049910576551282076245490090389328944075868508455133942304583236903222948165808559332123348274797826204144723168738177180919299881250404026184124858368.0; maybe add a comment explaining that Visual C can't parse that constant. >- /* 'a-' */ { }, >+ /* 'a-' */ { 0, }, are these actually needed? I thought empty struct initializers were fine, it's just empty struct definitions that are non-portable...
Review of attachment 258436 [details] [review]: Hi, This patch is committed as 20f873a0 Thanks for the review. With blessings.
Review of attachment 258437 [details] [review]: Hi, This patch was committed as 39a62a06. With blessings, thanks for the review!
Hello Dan, (In reply to comment #38) > >+#ifndef _MSC_VER > ... > maybe add a comment explaining that Visual C can't parse that constant. Will do that soonish. > >- /* 'a-' */ { }, > >+ /* 'a-' */ { 0, }, > > are these actually needed? I thought empty struct initializers were fine, it's > just empty struct definitions that are non-portable... Uh-uh, these don't work there-it ends up with a C2059 syntax error. With blessings, thank you!
(In reply to comments #35 and #36) > you ended up accidentally turning a bunch of LFs into CRLFs in this patch > indentation needs to be fixed Double oops... Will fix that soonish. Thanks for the notes. With blessings.
Created attachment 258897 [details] [review] glib/test/fileutils.c: Include unistd.h on *NIX only and fix on non-English Windows Hi, I have split out the fix for glib/tests/fileutils.c as it involved more than only including unistd.h on *NIX. The other issue here is, gettext on Windows does not look at setlocale (LC_ALL, "C") (the default CRT behavior), but instead looks at user's environment and the thread's locale on what translation to use/load. So, in my case, if I have this: (program_dir) bin intl.dll (which is loaded by this build of GLib) share locale zh_TW LC_MESSAGES glib20.mo the translated messages will be loaded even when LC_ALL == "C" on Windows (Chinese-Traditional), so the test_format_size_for_display tests will fail. So, we need to use SetThreadLocale()[1][2] to set the thread's locale to en-US, so that the original/English messages will be displayed as expected. With blessings, thank you! [1]: http://msdn.microsoft.com/en-us/library/windows/desktop/dd374051%28v=vs.85%29.aspx [2]: http://stackoverflow.com/questions/628557/using-gnu-gettext-on-win32
Created attachment 258899 [details] [review] glib/tests: Only include unistd.h on *NIX Hi, This is the updated fix to only include unistd.h when we are on *NIX for glib/tests. (In reply to comment #43) > (program_dir) > bin > intl.dll (which is loaded by this build of GLib) > share > locale > zh_TW > LC_MESSAGES > glib20.mo Should read: (program_dir) bin <libintl DLL> (which is loaded by this build of GLib) <GLib DLL> share locale zh_TW LC_MESSAGES glib20.mo With blessings, thank you!
Hello Dan, (In reply to comment #33) > Is g_spawn() expected to be able to spawn batch files? If so, you could rename > "echo-script" to "echo-script.bat" and then run this on windows as well. (Unix > doesn't care what the script is called, so it's fine for it to have the .bat > extension there too.) You couldn't directly, as there are some gotchas: -There needs to be an "@echo off" at the top if we only want to see the results of the script being output (else the script itself gets printed in the process). The shell does not understand that AFAIK, and you can't doing something like #ifdef... there-correct me if I am wrong. -In the shell, if you do echo "echo" you get echo -In a standard cmd.exe shell, if you do echo "echo" you get "echo" Let me know if you think it's good that we still want to do the .bat thing anyways, but checking for that output would also involve checking for line endings (\n vs \r\n) too. With blessings!
Created attachment 258904 [details] [review] gio/tests: Cleanup inclusion of unistd.h (take ii) Hi, This makes the inclusion of unistd.h only be done on *NIX, defines STDOUT_FILENO where needed and replace uses of ssize_t with gssize. With blessings, thank you!
(In reply to comment #45) > Let me know if you think it's good that we still want to do the .bat thing > anyways, but checking for that output would also involve checking for line > endings (\n vs \r\n) too. Ah, right. Well, in that case, it would be good to add a separate script for windows, and have appropriate windows-specific checks in the test program.
Comment on attachment 258904 [details] [review] gio/tests: Cleanup inclusion of unistd.h (take ii) >+#ifdef G_OS_WIN32 >+#ifndef STDOUT_FILENO >+#define STDOUT_FILENO 1 I think in other places we've just dropped the use of STDOUT_FILENO and just used "1" directly instead. Feel free to do that if you'd rather.
Review of attachment 258897 [details] [review]: Committed as 1079d30e
Review of attachment 258899 [details] [review]: Committed as d262b6fe.
Review of attachment 258904 [details] [review]: Committed as f4931142. I will look at the other issues in the next few days. Thanks for the reviews. With blessings.
Created attachment 258977 [details] [review] glib/gmessages.h: Unify messages under various compilers (In reply to comment #31) > We should just change the gcc versions to use > G_STRFUNC instead of __PRETTY_FUNCTION__, and then make everything use those > and get rid of the other definitions. Here we go...
Created attachment 258978 [details] [review] tests/: Avoid calling close() on invalid fd's (take ii) Hi, This fixes the indentation issues and is a but more careful about checking whether the fd is not -1 before attempting to call close() on it. With blessings, thank you!
Created attachment 258985 [details] [review] glib/gspawn-win32-helper.c: Clean up a bit (take ii) Hello Dan, (In reply to comment #36) > you ended up accidentally turning a bunch of LFs into CRLFs in this patch > indentation needs to be fixed Hopefully this new version of the patch addresses these issues. With blessings, thank you!
Created attachment 258987 [details] [review] tests/: Include unistd.h on *NIX only (take ii) Hi, This is the patch to include unistd.h on *NIX for the remaining files in tests/ and tests/refcount. With blessings, thank you!
Created attachment 258997 [details] [review] gio/test/gsubprocess.c: Fix on Windows Hello Dan, (In reply to comment #32) > test_cat_eof() seems like it ought to theoretically be applicable to windows, > with a little bit of rewriting. Maybe test_multi_1() too? If the test *could* > be ported to Windows, and just hasn't yet, it might be better to leave the test > compiled in, but in test_cat_eof() itself, do > > #ifdef G_OS_WIN32 > g_test_skip ("not yet ported to win32"); > return; > #endif I have re-attached the patch for gio/test/gsubprocess.c, and added the g_test_skip () line for Windows for the test test_cat_eof() because: -cat is not normally available for Windows -If I use an external cat.exe from MSYS or Cygwin, the test hangs as cat is waiting for user input (stdin), which my gut guess leads me to believe that there is a mechanism that's probably not working for this test to redirect stdin to feed the test. I thought I would not skip test_multi_1() as I get: ** GLib-GIO:ERROR:gsubprocess.c:500:test_multi_1: assertion failed (local_error == NULL): Failed to execute helper program (Invalid argument) (g-exec-error-quark, 19) So probably an investigation needs to be done on why this call fails, as the other tests calling gsubprocess-testprog.exe with "cat" as argument succeeds. With blessings, thank you!
Created attachment 259055 [details] [review] Improve the glib\tests\spawn-singlethread test on Windows Hi, This makes the test_spawn_script() test account for Windows-style line endings on Windows. Use a Windows-style .bat script to be used for the test, at least when this test is built with Visual C++. With blessings, thank you!
Created attachment 259056 [details] [review] glib/tests: Fix and skip tests that Visual C++ does not support Hello Dan, (In reply to comment #37) > If this doesn't work, I would expect the inf/nan tests in test_strtod() in > glib/tests/strfuncs.c to also fail. But you didn't comment those out... so > g_ascii_strtod() should be working... In fact they do fail there, because the inf/nan/hex strings are C99 features that Visual C++ does not support, even up to Visual C++ 2013. strtod() and atof() returns 0 when they encounter NANs[1], and the special macro INFINITY, instead of the special value NAN, even up to Visual C++ 2013, which should have much improved support for C99. I was more worried at the time I submitted the patch to get the code to build for Visual C++. ;) With blessings, thank you! [1]: http://msdn.microsoft.com/zh-tw/library/kxsfc1ab.aspx
Created attachment 259057 [details] [review] Fix tests on non-GCC (take ii) Hi, I am refactoring this patch a bit as I re-did the strfuncs.c patch in another patchset. With blessings, thank you!
Created attachment 259058 [details] [review] gio/tests/credentials.c: Fix expected message Hello Dan, (In reply to comment #31) > And then you won't need the change to > glib/tests/markup-collect.c (though you'll still need the change to > gio/tests/credentials.c, since that test was just wrong before). This is the patch for gio/tests/credentials.c. With blessings, thank you!
Created attachment 259242 [details] [review] Add NMake Makefiles to build the GLib unit tests (take ii, automation added) Hi, (In reply to comment #12) > It's unfortunate to have all this duplication... it would be better if, say, > there was a script that could generate the file lists in the nmake files from > the Makefile.am's... I came up with some Python scripts that is used to generate the file lists to be consumed by the NMake Makefile templates (*_vc.makin) so that the complete NMake Makefiles can be generated during 'make dist'-this is adapated from Shixin Zeng's setup.py script, which is found in the build/win32 directory/folder of a GLib git checkout. I have checked the scripts under both Python 2.7.x and Python 3.3 (which I have in my Fedora 19 system), which works under both flavors. They would acquire the source listings by reading from the Makefile.am's using Python's regex capabilities. Note that there are quite a bit of additional items in the *.py scripts as I do intend (if this patch is deemed acceptable) to use those scripts to generate the Visual Studio Projects for GLib, GObject and GIO as well, which are currently done in the Makefile.am's of glib/, gobject/ and gio/, because: -It's probably better that these items be separate from the these main Makefile.am's (so they may be cleaner), by isolating these items mainly in build/, as Behdad mentioned to me before (and probably I could improve on my old patches for this thing[1]) -It simplifies the process for one to build GLib on Windows/MSVC from a git checkout (instead of the complicated process to setup a Linux env. for this) -It makes updates to the completion of the Visual Studio projects easier to maintain (at least for me, as I am not an autotools expert at any means) -It also splits some items from Shixin's scripts and enhances it in some ways, notably being able to handle the "+=" items in the Makefile.am's. With blessings, thank you! [1]: https://bugzilla.gnome.org/show_bug.cgi?id=678334
Comment on attachment 258985 [details] [review] glib/gspawn-win32-helper.c: Clean up a bit (take ii) awesome
Comment on attachment 258987 [details] [review] tests/: Include unistd.h on *NIX only (take ii) i didn't look carefully but i assume this is fine
Comment on attachment 259056 [details] [review] glib/tests: Fix and skip tests that Visual C++ does not support >Skip the tests on inf/nan strings for the gvariant and strfuncs tests, and >skip the hex strings for the strtod tests in strfuncs as they are C99 >features that are not yet supported by Visual C++ (even 2013). Hm... we don't g_ascii_strtod() to behave differently in MSC than with unix/mingw though... so maybe we need to implement support for hex strings and nan/inf by hand there (the same way we don't use the system sprintf() under MSC). I'm not sure how much work that would take; if you want to get the tests passing now, feel free to just commit this patch for now, and file a bug to fix g_ascii_strtod() later.
Hi, Attachment 258977 [details] was pushed as 172aaa3a Attachment 258978 [details] was pushed as ccba409d Attachment 258985 [details] was pushed as 5fd3c63a Attachment 258987 [details] was pushed as fd41363e Attachment 258997 [details] was pushed as b27a2d43 Attachment 259055 [details] was pushed as a7707ec6 Attachment 259057 [details] was pushed as c58a7b8c Attachment 259058 [details] was pushed as 0212ab61 Thanks for the reviews, with blessings.
Hi Dan, (In reply to comment #64) > Hm... we don't g_ascii_strtod() to behave differently in MSC than with > unix/mingw though... so maybe we need to implement support for hex strings and > nan/inf by hand there (the same way we don't use the system sprintf() under > MSC). I have committed the patch (attachment 259056 [details] [review]) as f038c629for now, so I will file another bug tomorrow (i.e. Taipei time) for doing a strtod() like how it is done in *NIX/MinGW. Will investigate for that though. p.s. Just a quick question: AFAIK the distributions of the GLib tarballs are done on Linux/*NIX-is Python normally shipped as standard in those systems? Just wondering about this as I am (as in comment #61) looking towards moving the MSVC Makefile/Project File completion to Python, for the other parts of the GTK+/Clutter stack, if that is feasible for all sides. With blessings, thank you!
Created attachment 259853 [details] [review] gio/tests/test-memory-output-stream.c: Avoid an unintialized variable Hi, Some CRTs are more unforgiving about passing an uninitialized primitive-type variable into a function, even when passing it by reference. As a result, random and bad stuff gets written into that variable when it used by the function. This simple patch fixes this issue, as it caused the test to fail when the test and GLib was built under Visual Studio 2012. With blessings, thank you!
Hi, As this bug is getting quite big, I am closing this in favor of opening 2 other bugs, namely: -719344, for the initiative to fix the GLib test programs and GLib itself running on Windows. -719346, for switching the MSVC build files generation to Python 2/3 scripts, which includes the NMake Makefiles for building the test programs that is in this bug. With blessings, thank you!