GNOME Bugzilla – Bug 719344
Fix the various test programs on Windows
Last modified: 2018-05-24 16:06:46 UTC
Hi, This is meant to be a bug to track the attempts to fix the various test programs (or GLib itself) running on Windows as the original bug (bug 711047, to fix the build of the test programs on Windows/MSVC) has gotten a little bit too big. With blessings, thank you!
Created attachment 262825 [details] [review] gio/tests/memory-output-stream.c: Avoid uninitialized 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!
Created attachment 262826 [details] [review] glib/tests/environment.c: Fix running on Windows Hi, When we run the listenv test, it uses g_get_environ(), which in turn calls GetEnvironmentStringsW() from the Win32 API. At least on Windows 7 and 8, this call also returns some special environmental variables (such as the current path) which have empty strings as the name of the environmental variables. So, we don't want to insert such variables in the hash table used to check for the existing environmental variables, so that the test will not fail on false positives on Windows. With blessings, thank you!
Review of attachment 262825 [details] [review]: Ok.
Review of attachment 262826 [details] [review]: Looks good.
Hi, The 2 patches were committed as: -Attachment 262826 [details]: 29b66e14 -Attachment 262825 [details]: b9322bf9 Sorry, I accidently had the wrong bug number in the commit messages :) Thanks for the reviews, with blessings!
Created attachment 264383 [details] [review] Fix the save test in the keyfile test on Windows Hi, There was a test for saving the keyfile (test_save()) that was recently added to the keyfile test (glib/tests/keyfile.c) that would fail on Windows, as it opens a temp file using g_mkstemp() without closing the fd that is returned by that function before calling g_key_file_save_to_file(), which attempts to open another fd (which Windows does not allow as it places a lock on the fd until the fd is closed) on the temp file that is opened by g_mkstemp(). This attempts to fix the test on Windows by closing the fd returned by g_mkstemp() before trying to call g_key_file_save_to_file(). With blessings, thank you!
Created attachment 265133 [details] [review] Fix the stdio wrappers test on Windows Hi, There was a newly-added test for the stdio wrappers in the fileutils.c test program that would not work on non-Unix platforms, as it tests for things that are applicable to *nix platforms only, plus it needs to be updated for the test program to be build on Windows again. Namely: -fcntl.h needs to be included for Windows as well for O_RDONLY, so probably it's best to include it unconditionally. -struct utimbuf is found in sys/utime.h on Windows, so include that on Windows. -Add definitions for F_OK and S_ISDIR for cases on Windows when needed (i.e. MSVC) -On Windows, the permissions mode argument for g_mkdir() is not honored, due to differences in the way the OS works, so skip the tests where we test for directory permissions stuff (it's not applicable there, causing the tests to fail). -g_creat() does honor the permissions mode on Windows, but with different values from *nix, so use the Windows mode values, so that it is understood by the CRT [1]. With blessings, thank you, and Happy New Year 2014! [1]: http://msdn.microsoft.com/en-us/library/fb347w33%28v=vs.90%29.aspx
Created attachment 265227 [details] [review] Fix the date test on non-English versions of Windows Hi, The names of the months are localized depending on the Windows system locale when run in the system console prompt (cmd.exe), so the parsing of the English "March", for example, will fail unless the Windows SetThreadLocale() (setlocale() is not enough here) API is used to set the locale of the running program to en-US. This fixes the test running on non-English version of Windows by using SetThreadLocale() prior to running the "March" and "Sept" parsing tests. With blessings, thank you!
Created attachment 265228 [details] [review] Fix the giomodule test on non-*nix Hi, The file extension of a GIO module is not necessarily .so on all platforms, so use the GMODULE_SUFFIX macro from glibconfig.h so that we get the correct file extension (e.g. dll on Windows) so that the test will load the test module correctly on the platforms that GLib supports, which will fix the test running on Windows. With blessings, thank you!
Review of attachment 264383 [details] [review]: ok
Review of attachment 265133 [details] [review]: ::: glib/tests/fileutils.c @@ +889,2 @@ ret = g_creat ("test-creat", 0555); +#endif Do you have S_IWUSR, S_IRGRP and so on defined on Windows ? In that case, we could avoid the ifdef here and just always use those constants.
Review of attachment 265227 [details] [review]: ::: glib/tests/date.c @@ +129,3 @@ +#ifdef G_OS_WIN32 + SetThreadLocale (MAKELCID (MAKELANGID (LANG_ENGLISH, SUBLANG_ENGLISH_US), SORT_DEFAULT)); +#endif I think this should go right next to the setlocale call we already have.
Review of attachment 265228 [details] [review]: sure
Created attachment 265283 [details] [review] Fix the date test on non-English versions of Windows Hi The patches were pushed as the following: Attachment 264383 [details]: 3fd6edab Attachment 265228 [details]: 589aed93 (In reply to comment #12) > I think this should go right next to the setlocale call we already have. Agreed, so here comes the patch. With blessings, and thanks for the reviews!
Hi Matthias, (In reply to comment #11) > Do you have S_IWUSR, S_IRGRP and so on defined on Windows ? In that case, we > could avoid the ifdef here and just always use those constants. These are available in sys/stat.h: (Visual Studio 2008+) #define S_IFMT _S_IFMT #define S_IFDIR _S_IFDIR #define S_IFCHR _S_IFCHR #define S_IFREG _S_IFREG #define S_IREAD _S_IREAD #define S_IWRITE _S_IWRITE #define S_IEXEC _S_IEXEC (MinGW64 extending from the above) #define S_IFIFO _S_IFIFO #define S_IFBLK _S_IFBLK #define _S_IRWXU (_S_IREAD | _S_IWRITE | _S_IEXEC) #define _S_IXUSR _S_IEXEC #define _S_IWUSR _S_IWRITE #define S_IRWXU _S_IRWXU #define S_IXUSR _S_IXUSR #define S_IWUSR _S_IWUSR #define S_IRUSR _S_IRUSR #define _S_IRUSR _S_IREAD #define S_ISDIR(m) (((m) & S_IFMT) == S_IFDIR) #define S_ISFIFO(m) (((m) & S_IFMT) == S_IFIFO) #define S_ISCHR(m) (((m) & S_IFMT) == S_IFCHR) #define S_ISBLK(m) (((m) & S_IFMT) == S_IFBLK) #define S_ISREG(m) (((m) & S_IFMT) == S_IFREG) So, we could use S_IWRITE for S_IWUSR but not S_IRGRP, as the notion of group permissions isn't that prevalent in Windows (even from MinGW). With blessings, thank you!
Created attachment 265409 [details] [review] Improve the Fix on keyfile.c by using g_close() Hi, Apparently I didn't realize that there was a g_close() API that was added by Colin last January, and close() required unistd.h on *nix (which broke the build for some, sorry for the troubles!). So, this is the patch that uses g_close() in place of close(), which avoids the inclusion of unistd.h, which is not universally available. With blessings, thank you!
Review of attachment 265283 [details] [review]: ok
Review of attachment 265409 [details] [review]: sure
Created attachment 265509 [details] [review] Fix gtimezone.c on Windows by checking the size of tzi.DaylightName Hi Matthias, The patches were pushed as follows: Attachment 265283 [details]: 8bb81e70 Attachment 265409 [details]: 6bd30a4c I also came up with a small fix for gtimezone.c on Windows that checks for the size of tzi.DaylightName before trying to query the registry using RegQueryValue() for it. The reason behind this is that this will give a clear buffer so that tzi.DaylightName can be acquired without running into ERROR_MORE_DATA, which indicates that the buffer does not have enough room to hold the data for tzi.DaylightName, which is a pitfall of RegQueryValue()[1]. This will enable the equal test in the gdatetime.c test program to pass, along with certain other tests in that test program as well, as least on non-English versions of Windows. There are still however items pointed out in gdatetime.c (the test program) that need to be fixed, specifically on time zone names, for non-English versions of Windows, which I will continue to look into. [1]:http://social.msdn.microsoft.com/Forums/vstudio/en-US/84f90854-e90c-4b63-8fc1-655a0b4645fd/regqueryvalueex-returns-errormoredata With blessings, and thanks for the reviews.
Hi Matthias, (In reply to comment #11) > Do you have S_IWUSR, S_IRGRP and so on defined on Windows ? In that case, we > could avoid the ifdef here and just always use those constants. I have tried further trying to define the S_* constants, as I searched for them. Unfortunately this isn't going to work, as octal 0555 means (S_IRUSR | S_IRGRP | S_IROTH | S_IXUSR | S_IXGRP | S_IXOTH) on *nix, and _wcreat (which g_creat wraps on Windows) only accepts octal 0200 (S_IWRITE/S_IWUSR), octal 0400 (S_IREAD/S_IRUSR) or octal 0600 (S_IWRITE | S_IREAD) as valid inputs for the mode, any other values put into this will fail (and will trigger the invalid parameter handler where the CRT aborts() the program by default on newer Windows CRTs). It does seem to me from the docs that g_creat() follows the rules that the system CRT ordains for creat()/_wcreat(). Some of my take on this, let me know if something otherwise is better is known though. With blessings, thank you!
Created attachment 268122 [details] [review] Fix the fileutils test (another take) Hi, Due to differences in the mode parameter that is taken by creat() (hence g_creat()) as mentioned in the previous comments, this is another take to define the mode parameter according to the platform where GLib is being built for, so that the fileutils test can run successfully on Windows with all the applicable tests. With blessings, thank you!
Created attachment 268772 [details] [review] Fix and enhance GTimeZone for Windows Hi, After investigating the GTimeZone items further, it seems that there needs to be some more fixes and enhancements to it. The enhancement is mainly to use RegLoadMUIStringW(), when possible, as this is only supported in Windows Vista+, as using RegLoadMUIStringW() on MUI_Std and MUI_Dlt will allow one to acquire strings timezone names and abbreviations that is not tied to the language version of Windows that programs are being run. The gdatetime test has been updated to support this kind of usage, when possible, with a note in the test_GDateTime_printf() test as the "Pacific Standard Time" test string actually varies, depending on which time zone the program is being run. There also needs to be a fix, as one cannot just directly do a strncpy of tzi->StandardName and tzi->DaylightName to rule->std_name and rule->dlt_name respectively, as tzi->StandardName and tzi->DaylightName are whcar_t (i.e. gunichar2) strings, so a g_utf16_to_utf8() call is needed. With blessings, thank you!
Created attachment 268886 [details] [review] Avoid possible overflow in glib/gasyncqueue.c Hi, In g_async_queue_timed_pop_unlocked() when the m_end_time is being evaluated, there is a part (end_time->tv_sec) * G_USEC_PER_SEC) + end_time->tv_usec during its calculation. as end_time->tv_sec and end_time->tv_usec are both glongs (i.e. longs), it is possible that (end_time->tv_sec) * G_USEC_PER_SEC becomes negative due to an overflow as long's are 4 bytes on x86 and x64 Windows, just like int's. This is revealed by the asyncqueue test as it passes in an GTimeVal to g_async_queue_timed_pop_unlocked () which is close to the value that is acquired by g_get_current_time(). This patch addresses the issue by casting end_time->tv_sec to be a gint64 and first calculating the end_time (a gint64) in usecs before deducing the (correct) value for m_end_time, which fixes the issue and allow the asyncqueue test to complete successfully. With blessings, thank you!
Created attachment 269100 [details] [review] Fix g_usleep() calls on Windows Hi, As there is a well-known limitation of Windows that it cannot do a sleep with an precision of less than 1 millisecond, so for example: g_usleep (x), where x < 1000 Since g_usleep(x) then calls Sleep (x/1000) on Windows, where x/1000 is a whole number (unsigned long)) means that we effectively call g_usleep (0) when x < 1000. This patch attempt to fix such calls on Windows, which are profound in glib/gthreadpool.c and in the timer test, so that it does indeed attempt to do a sleep on Windows before continuing. This allows the timer test to pass on Windows. With blessings, thank you!
Created attachment 269753 [details] [review] Fix and Enhance GTimeZone on Windows (take ii) Hi, This replaces the previous patch on GTimeZone for fixes and enhancements as some things can be simplified a bit, and I think it's better to split the patch into two parts. The enhancements are still for Windows Vista+ only, as MUI_Std/MUI_Dlt are not normally in a XP's registry, if at all. With blessings, thank you!
Hi, This updates the gdatetime test program so that it would utilize the enhancements in GDateTime (please see comment #25), where possible. This adds a note for the TEST_PRINTF (%Z, ...) test that the test string depends on which timezone of the Windows machine the test is being run on, so with a correct English string for that the test should pass on any language versions of Windows Vista or later. This means, as such functionality is not available on Windows XP, this test would not be able to succeed without changing the test strings for this test and the abbreviations test into the one indicated by the language of the OS, if running on an non-English version of XP. With blessings, thank you!
Created attachment 269754 [details] [review] Update the gdatetime test program for Windows Sorry, I forgot to attach the patch...
Review of attachment 268886 [details] [review]: Hi, For records, this bug was fixed by Ryan with this commit: 356fe2ce With blessings, thank you!
Created attachment 270448 [details] [review] Fix and Enhance GTimeZone on Windows (take iii) Hi, Apparently I didn't account for Windows XP during the fixing and enhancement of gtimezone.c. This makes all the RegOpenKeyEx()/RegQueryValueEx() calls use the wide (i.e. RegOpenKeyExW()/RegQueryValueExW()) versions of the APIs so that we can also acquire tzi->StandardName and tzi->DaylightName properly, even on Windows XP, for DBCS languages. So, for example, on my Chinese-Tradtional XP Mode system, where RegLoadMUIStringW() is not supported, I would get the correct "東南美洲標準時間", instead of a gibberish string, when I run the test_GDateTime_new_full() test in glib/tests/gdatetime.c during the g_date_time_get_timezone_abbreviation () portion. With blessings, thank you!
Created attachment 270964 [details] [review] Fix the GSubprocess test on Windows Hi, This further fixes the GSubprocess test so that it would be able to run and succeed on Windows, mainly due to differences in line endings ("\r\n" on Windows vs "\n" on *nix), and differences in the ways the OS handles things. Any feedback on the un-reviewed patches would be greatly appreciated. With blessings, thank you!
Review of attachment 270964 [details] [review]: ::: gio/tests/gsubprocess.c @@ +564,3 @@ +#ifdef G_OS_WIN32 + g_assert_cmpint (g_memory_output_stream_get_data_size ((GMemoryOutputStream*)membuf), ==, 28658); Maybe a comment here that this is about different line ending size... Or break out the calculation to look like 'x + y * strlen(LINEEND)' might be even better. @@ +933,3 @@ g_subprocess_launcher_setenv (launcher, "THREE", "1", FALSE); +#ifdef G_OS_WIN32 + g_subprocess_launcher_setenv (launcher, "PATH", g_getenv ("PATH"), TRUE); why is this one required? @@ +970,3 @@ + gchar *tmpdir, *tmpdir_plus_lineend; +#ifdef G_OS_WIN32 + tmpdir = g_strdup (g_getenv ("TEMP")); i wonder if we could just use g_get_tmp_dir() here?
Created attachment 271401 [details] [review] Fix the GSubprocess test on Windows (take ii) Hi, Here is my other go on fixing the GSubprocess test on Windows. > Or break out the calculation to look like 'x + y * strlen(LINEEND)' might be > even better. Agreed, so here it is... > + g_subprocess_launcher_setenv (launcher, "PATH", g_getenv ("PATH"), TRUE); > why is this one required? This is because g_subprocess_launcher_set_environ() is called on the launcher, which will wipe out all the user-defined environment variables and set the env vars passed into that function, including the PATH. It is quite common on Windows that third-party DLLs are not normally installed into the system's PATH to avoid conflicts in runtimes and so (i.e. the "DLL Hell"), and this is especially true on Windows builds of GLib, as the freshly-built GLib binaries on Windows are not in the system's default PATH in most cases. Hope this explains on the situation a bit. > i wonder if we could just use g_get_tmp_dir() here? Yup, so here it is. With blessings, thank you!
Created attachment 271693 [details] [review] Fix the test-printf test program on Windows by taking system-specific line endings of stdout into account. Hi, This is my attempt to fix the test-printf test program, which, like the gsubprocess test, checks for the output strings from stdout. As Windows's line endings from stdout is "\r\n", not "\n", that needs to be taken into account as well when we check against the string from stdout. With blessings, thank you!
Created attachment 272242 [details] [review] gio/gtestdbus.c: Fix write_config_file() for Windows Hi, In gio/gtestdbus.c's write_config_file(), we are calling g_file_set_contents(), which calls g_unlink() on the temp file that was created earlier in write_config_file(). The creation of the temp file, using g_file_open_tmp(), returns a file descriptor (fd), that needs to be closed before g_unlink() is called on that file, otherwise g_unlink () will fail with a permission defined error. This patch fixes this situation by moving the close() call earlier, before the g_file_set_contents() call, as the fd is not used otherwise. This fixes a number of tests in GTK+. With blessings, thank you!
Review of attachment 272242 [details] [review]: Hi, this patch is moved to bug 729981 for clarity's sake. With blessings, thank you!
Review of attachment 270448 [details] [review]: Hi, this patch is moved to bug 729858 for clarity's sake, as it involves fixing GLib itself. I am leaning towards making this bug a bug for fixing the GLib test programs on Windows.
Review of attachment 269754 [details] [review]: Hi, I have moved this patch to bug 729858, as this patch is closely tied to the GTimezone fixes and enhancements. With blessings, thank you!
Created attachment 276357 [details] [review] tests/child-test.c: Fix CreateProcess() call on Windows Hi, This fixes the CreateProcess() call that is used on Windows, as the current code actually requires invoking the program using child-test.exe (i.e. with the .exe extension), otherwise the program will fail to run due to a CreateProcess() error. As the command line argument to CreateProcess() already contains a call to child-test, we don't really need to pass in the application name, so update and clean up the code accordingly. With blessings, thank you!
Created attachment 276516 [details] [review] Fix the thumbnail-verification test Hi, GIO's thumbnail-verify() had its third argument updated from const GStatBuf to const GLocalFileStat, so in its test program, we should also pass in a GLocalFileStat instead of a GStatBuf, so that the test can be fixed on platforms that GLib supports. With blessings, thank you!
Review of attachment 276516 [details] [review]: Hi, This patch was pushed as part of bug 711547 (please see comments 27 and 28), for records. With blessings, thank you.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/796.