GNOME Bugzilla – Bug 707787
Fix Build/Run of glocalfile.c on Windows/MSVC without dirent
Last modified: 2013-09-18 10:52:50 UTC
Created attachment 254506 [details] [review] Fix build of glocalfile.c on Windows/MSVC without dirent installed Hi, The recent commit 6ec2bb17 (GFile: add new g_file_measure_disk_usage() API) added items in glocalfile.c to measure the size of the file which made use of dirent functionality, which unfortunately is not universally available on Windows, as in the case of building the code on Visual C++, which caused the build to break there unless one builds and installs an implementation of dirent for Visual C++ beforehand. I have come up with a patch to make use of the (wide) dirent implementation that is currently shipped with GLib so that it will still build and work on Windows builds done with Visual C++ without an existing dirent implementation. The wide implementation is used due to the fact that many Windows-specific parts of GLib are using wide implementations of the Windows APIs. With blessings, thank you!
Created attachment 254507 [details] [review] gio/tests/gio-du.c: Avoid using an uninitialized variable Hi, On the way to test the use of the dirent implementation shipped with GLib, the gio-du.c test program was using an uninitialized variable that newer Windows CRTs don't like, causing the program to abort with --progressive is not specified on the command line. This simple patch remedies this situation. With blessings, thank you!
Sorry, "with --progressive" should read "when --progressive".
Created attachment 254761 [details] [review] GDir: Add g_dir_open_from_dirp Hi, This is my first attempt to do a *NIX function, as a result of the conversation with Ryan (desrt) on IRC earlier today. It is a simple function, but it's my first go at a *NIX function and documentation. With blessings, thank you!
Created attachment 254762 [details] [review] gio/glocalfile.c: Use GDir functionality Hi, This is my other attempt to fix glocalfile.c building on Windows/MSVC as well as running on Windows as a whole, as we are normally using wide character filenames on Windows. This makes use of the GDir APIs to do the stuff that was formerly done with dirent when we don't have AT_FDCWD, which also takes care of the wide character filename issue on Windows. I was not able to test this extensively on *NIX, but it seems that gio-du reports the same size for files that I run against on my Fedora 19 system before and after this patch set is applied. With blessings, thank you!
Review of attachment 254761 [details] [review]: Looks good except for docs. Please merge with a fix that says the function never fails. ::: glib/gdir.c @@ +189,3 @@ + * created using opendir() or fdopendir(). + * + * Return value: a newly allocated #GDir on success, %NULL on failure. the "NULL on failure" language is a bit odd. This function really can't possibly fail... We usually don't consider a failure to satisfy the precondition as a 'failure' of the function.
Review of attachment 254762 [details] [review]: I don't care for this one as much. It's too complicated. It should be possible to clean up the loop a little as well (removing the "." and ".." check, for example, since GDir already does them). I'll refactor this one.
Review of attachment 254507 [details] [review]: Oops.
Created attachment 254801 [details] [review] GDir: Add g_dir_new_from_dirp Add a simple UNIX-only API that is used to create a GDir object from a DIR* that is aquired using opendir() or fdopendir(). This makes it possible to use GDir with openat(), which in turn will allow use of GDir in the existing GLocalFile implementation of g_file_measure_disk_usage(), avoiding the current MSVC compatibility problems there. I renamed _open_ to _new_ to emphasise that this is just a constructor and it will never fail. "Open" sounds like something that might fail.
Created attachment 254802 [details] [review] GLocalFile: use GDir for g_file_measure_disk_usage It turns out that although dirent is available on mingw32 (where the code was originally tested), it is not usable from MSVC. Avoid portability problems by just using GDir.
Created attachment 254803 [details] [review] GLocalFile.measure_disk_usage: utf8ify filenames Make sure that filenames in error messages are printed in UTF-8. Also, drop the file:/// that we were prefixing to the filenames. These filenames are not proper URIs (since they are not escaped) and adding the escaping would only serve to reduce legibility. Bonus patch.
Ugh. My glocalfile patch is obviously incorrect. Please don't test it. Will have a new one soon.
Created attachment 254806 [details] [review] GDir: add errno variant of g_dir_open() Add a version of g_dir_open() that returns an error via errno instead of GError. This will allow for custom formatting of error messages.
Created attachment 254807 [details] [review] GLocalFile: use GDir for g_file_measure_disk_usage It turns out that although dirent is available on mingw32 (where the code was originally tested), it is not usable from MSVC. Avoid portability problems by just using GDir.
This is rapidly spinning out of control, but the crux of the issue is more or less this: the error message from g_dir_open() is not the one we want. I'd much prefer to format it myself, but in order to do so I need errno. That's also convenient, since errno is how we would handle all of the other cases (like failing g_stat, etc), so we can use the same error reporting function. So... time to add two new APIs in the 11th hour?
Review of attachment 254507 [details] [review]: Hi, I have pushed this patch as 7a91a6c9 to master. With blessings, thank you!
Hi guys, I'm confused about what's in master now... Yesterday I applied Fan's patch (locally) and I reported via the mailing list that it seemed to work, except for a query about whether or not errors were being correctly handled. Today (thinking that something's now been pushed to master) I reverted Fan's patch and pulled from master. But I'm now back to having the original problem that I reported in the first place (unresolved externals). Due to the imminent release of a new stable version, it seems as if this problem has been fixed with patches, rather than pushing a fix to master. Firstly, this means that the upcoming 'stable' release won't even build with MSVC!! Secondly, I'm confused about whose patches to apply. Is there only one fix or do I need to apply two sets (one from Ryan and one from Fan)?
Review of attachment 254807 [details] [review]: Hi Ryan, Thanks for the patches and review on my take, but a few things I would like to bring up: -dirent.h is still included unconditionally, so I think we need to guard it with #ifdef AT_FDCWD/#endif. -As the filenames will be UTF-8 on Windows, I think we also need to change, otherwise file/directory names with non-ascii characters will not be properly processed: #if defined (AT_FDCWD) if (fstatat (parent_fd, name->data, &buf, AT_SYMLINK_NOFOLLOW) != 0) #elif defined (HAVE_LSTAT) if (lstat (name->data, &buf) != 0) #else if (stat (name->data, &buf) != 0) #endif -to- #if defined (AT_FDCWD) if (fstatat (parent_fd, name->data, &buf, AT_SYMLINK_NOFOLLOW) != 0) #else if (g_lstat (name->data, &buf) != 0) #endif Just a few of my thoughts. With blessings, thank you!
Review of attachment 254806 [details] [review]: Another thing: g_return_val_if_fail (wpath != NULL, NULL); (at around line 100) will cause an assert message to pop up if the directory that is being measured has no files in it (i.e. it only has . and .. in it), so I think we might need to have: if (wpath == NULL) return NULL; This seems to be the behavior before we add g_dir_open_with_errno(). With blessings, thank you!
Created attachment 254845 [details] [review] Improve the gio-du test program on Windows Hi Ryan, I have gone a bit further to improve the gio-du test program on Windows by using __wgetmainargs(), which is used to convert the argv strings passed into the program into wchar_t (gunichar2) strings. This is necessary as it makes it possible on Windows to test for files/directories that are input as files to test as non-ASCII characters (such as CJK), as we can use g_utf16_to_utf8() to properly convert the input file/directory names into UTF-8 strings so that they can be found and processed correctly. Since __wgetmainargs() is possibly not provided in the MinGW headers, I have declared it as an extern on non-MSVC (please see gspawn-win32-helper.c, for example) so that it should build fine there, but please let me know if it should be a problem there (as I don't use MinGW to build GLib normally :) ) With blessings, thank you!
(In reply to comment #16) > Hi guys, I'm confused about what's in master now... > Hi John, Hang on a bit... as there are some issues that needs to be dealt with with the fixes (as I spoke to Ryan on IRC about 1,5 days ago). Unfortunately there are some more things that needs to be checked and considered before things are deemed resolved. Can you try to apply all the patches in this bug that are not made obsolete (with my comments in comments 17 and 18 put in, if it's ok with you) and see how the things go with you, and let us know about how things work for you here. Thanks, with blessings.
(In reply to comment #18) > g_return_val_if_fail (wpath != NULL, NULL); (at around line 100) will cause an > assert message to pop up if the directory that is being measured has no files > in it (i.e. it only has . and .. in it), so I think we might need to have: How is that possible? My understanding is that it's only possible to get NULL here in the case that the conversion of utf8 to utf16 failed (ie: you gave a non-utf8 string as input).
John: we have a bit of time before the release still and I want to ensure that we get this right. We won't release a version that doesn't build on MSVC -- don't worry :) You were mentioning something about some hacked up MSVC dirent support that should be working but isn't? Is it possible that we could maybe get that fixed as an alternative to growing the public API of GDir?
(In reply to comment #22) > John: we have a bit of time before the release still and I want to ensure that > we get this right. We won't release a version that doesn't build on MSVC -- > don't worry :) > Okay, I won't panic just yet... ;-) > > You were mentioning something about some hacked up MSVC dirent support that > should be working but isn't? Is it possible that we could maybe get that fixed > as an alternative to growing the public API of GDir? > I mentioned that for the other sources that used opendir() / readdir() etc, a different approach was taken for MSVC ('glib/gspawn.c' became 'glib/gspawn-win32.c'. 'gio/gcontenttype.c' became 'gio/gcontenttype-win32.c'). Do you mean that something similar should get done for 'gio/glocalfile.c'? That would definitely be more consistent although I haven't looked in any depth at what actually got done... (In reply to comment #20) > (In reply to comment #16) > > Can you try to apply all the patches in this bug that are not made obsolete > (with my comments in comments 17 and 18 put in, if it's ok with you) and see > how the things go with you, and let us know about how things work for you here. > Okay Fan. Leave that with me and I'll apply them this afternoon. Am I right in thinking that your patch 254507 is now redundant (since it's now in master)? If so, please confirm that I'd need to apply 5 x patches in total, namely:- 254801 254803 254806 (along with your change from Comment #18) 254807 (along with your changes from Comment #17) 254845 Or (in the light of Comment #22) should we wait to see if a completely fresh approach is needed? Best regards, John
I was referring to the build/win32/dirent/dirent.c stuff...
Ah, I see. Yes, there's one particular file in glib ('glib/gdir.c') which gains access to opendir() etc by effectively #including dirent.c. At the time, I hadn't spotted the relevant #include so I was intrigued about how it was working. There's nothing there that needs fixing AFAIK. I was just baffled about how it was all managing to work!
(In reply to comment #21) > > How is that possible? My understanding is that it's only possible to get NULL > here in the case that the conversion of utf8 to utf16 failed (ie: you gave a > non-utf8 string as input). Hello Ryan, My fault, sorry! :) It was because I used the gio-du program on that case before I did my update to gio-du, and neglected to check it again. gio-du was not passing in the correct utf8 items to the associated functions before I updated as in my patches in comment 19. With blessings, thank you! > 254801 > 254803 > 254806 > 254807 (along with your changes from Comment #17) > 254845 Hello John, Yes please, please apply and try these patches (and please disregard my comments in comment 18, I messed up a bit as mentioned above:) ). With blessings, thank you!
Hi Fan, I just tried to catch Ryan on IRC but it might be the wrong time of day for him. Anyway... I wanted to report that I applied everything you just mentioned above (except for patch 254845 as I'm not yet building any tests with my current setup). The result is that everything is now building fine again with MSVC. It's quite late in my working day here so I'm a bit too tired to look through the code for possible anomalies but the build itself is working fine.
Review of attachment 254806 [details] [review]: Hi, >dir.dirp = opendir (path); >if (dir.dirp == NULL) > return dir; >#endif Just noticed another part (sorry I didn't notice as this is when !G_OS_WIN32): Probably when dir.dirp == NULL we should also return NULL (otherwise the build in Linux breaks), like the Windows case. Just to bring this up. With blessings, thank you!
Hi, Another thing I noticed in attachment 254806 [details] [review]: >dir.dirp = opendir (path); >if (dir.dirp == NULL) > return dir; >#endif Just noticed another part (sorry I didn't notice as this is when !G_OS_WIN32): Probably when dir.dirp == NULL we should also return NULL (otherwise the build in Linux breaks), like the Windows case. Just to bring this up. With blessings, thank you!
Review of attachment 254807 [details] [review]: hi Fan, Thanks for these comments. Both of them were mistakes on my part that you had handled correctly in your original patch, so sorry for that. I'll be attaching a new version soon.
Review of attachment 254806 [details] [review]: Fixed the return NULL issue. Will upload a new patch soon. Can you tell I'm testing for Windows now and not the unix path? :)
Created attachment 255042 [details] [review] GDir: add some glib-private APIs Since it's so late in the game, let's do these as glib-private calls instead of adding new public API.
Created attachment 255043 [details] [review] GLocalFile: use GDir for g_file_measure_disk_usage It turns out that although dirent is available on mingw32 (where the code was originally tested), it is not usable from MSVC. Avoid portability problems by just using GDir. Also, be careful about ensuring that we utf8-format filenames in our error messages, and leave out the "file://" component since the strings we're displaying are not URIs (and we don't want to make them URIs since the extra escaping would reduce legibility). Thanks to Chun-wei Fan <fanchunwei@src.gnome.org> for portions of this patch and for reviews.
I'm pushing these for now because we need a release today. I hope they're working on MSVC, but I have no way to check. If someone else coudl do that, it would be awesome. Attachment 255042 [details] pushed as 725125a - GDir: add some glib-private APIs Attachment 255043 [details] pushed as 084e5b0 - GLocalFile: use GDir for g_file_measure_disk_usage
Hi Ryan, Sorry I was not able to respond in time for the 2.37.92 release, due to time differences in another part of the planet ;) Thanks very much for the patches and putting up with my ignorance in the process-it does seem to me that these patches work fine on Windows/MSVC, including file/directories that use non-ascii/wide characters like CJK characters, at least from the 2.37.92 release. There is however (although this is another issue) a problem running gio-du with the --async option (the program crashes-I guess you were able to run with --async in wine, so I am suspecting it to be another case where the newer Windows CRT caught something that it deems to be unsafe), so I will look into it ASAP. This is however not related to any of the patches here so far, AFAICT, so I think those patches are good. (In reply to comment #32) > Can you tell I'm testing for Windows now and not the unix path? :) Yup :), just happened to run into that when I was packaging tarballs in my Linux system to test GLib bleeding edge on Windows/MSVC :), I think I need to update Shixin's Python scripts to simplify the MSVC testing process as it might have become outdated, but that's another thing. :) With blessings!
(In reply to comment #36) > There is however (although this is another issue) a problem running gio-du with > the --async option (the program crashes...). Hi, It seems that this is caused when --progress is not specified on the command line of gio-du when --async is specified, as my previous patch made sure progress is initially NULL to avoid using an uninitialized variable, resulting progress to be NULL and causing an access violation (segfault) as the async functions attempt to use the progress variable as the callback function it uses. It seems to me that checking whether progress != NULL with option_use_async (gio-du.c line 150) will avoid this crash, but I'm not sure whether I am missing something here, but few things that I observed when testing this (these may need separate bug reports): -It seems that the callback parameter is (allow-none) in its doc annotations, so probably it's not so in this case, as AFAIK allow-none means the parameter can be NULL without problems. -The number of files and directories seem to be random bizarre numbers when we use --async and --progress (but not --progress alone) in Windows (and Linux, I tried it out of the blue), so probably something else is going on? With blessings, thank you!
For some reason, Attachment 255042 [details] failed when I tried to apply it. I could probably investigate further but as an alternative, are these updates in master now? i.e. should I just update from master or is it still necessary to apply the changes as patches?
Review of attachment 254845 [details] [review]: Hi, For records, the patch was committed by Ryan as 2684dec4. Hello John, Yes, those patches were committed to master already. Can you download 2.37.92 from the GNOME FTP and try to build it, and if possible, build and run gio/tests/gio-du.c without using --async (please see my previous comment about this)? With blessings, thank you!
Created attachment 255107 [details] [review] measure_disk_usage: skip progress on NULL callback In the real_..._async wrapper for GFile.measure_disk_usage, skip the wrapping of the progress callback in the case that the user gave a NULL callback to the async function. This is a performance improvement because the sync version won't have to do continuous sampling of the clock to issue a call to the wrapper which will then do nothing. Unfortunately, I made this simplifying assumption when writing the wrapper, but forgot to actually implement it when making the sync call. As a result, the wrapper is still called, and invokes the NULL callback, causing a segfault. Make sure we pass NULL if the user's callback was NULL.
Attachment 255107 [details] pushed as dbf95a5 - measure_disk_usage: skip progress on NULL callback Thanks for the checking on Windows and for catching this one remaining bug. I think this is everything, then. Closing.
Only one minor issue... When building glib with MSVC the compiler issues a warning at line 224 of 'glib/gdir.c' "warning C4716 : 'g_dir_new_from_dirp' must return a value" It's caused because g_assert_not_reached() doesn't return anything (though in practice, it should never get reached!) Someone should probably test if MinGW will compile it though....
I tested it with mingw (cross build on Fedora) and all is well there. It's pretty annoying that MSVC is unable to tell that g_assert_not_reached() never returns. Maybe we could have a separate bug for that. We fixed it in the past for g_error() but adding a for(;;); after it...
(In reply to comment #42) > Only one minor issue... > > When building glib with MSVC the compiler issues a warning at line 224 of > 'glib/gdir.c' (warning C4716) > Hello John, I presume you are using Visual Studio 2005? I didn't get this warning when I built it with both Visual Studio 2008 and 2010, under both Win32 and x64 configs, and I guess it's likewise for 2012 (didn't check that). Just to let you know my take on this-I guess probably it would be best to leave this part alone. Hello Ryan, Thanks a whole lot for all the time. The async parts are doing better but running "gio-du --async gio-du.exe gio-du.c services" (in gio/tests) can have: -filenames that aren't correct (with other file names or garbled text) -wrong files/directories being checked. I will check on this later to find out why (most probably a threading issue), possibly also under Linux too (sorry to bother you with loads of stuff here). BTW the gio/task test program passes in my MSVC builds (x86 and x64 builds), just to let you know. With blessings.
hi Chun-wei Fan, I think the problem is that on Windows, your patch to get the unicode argument names does this: + g_file_measure_disk_usage_async (file, flags, G_PRIORITY_DEFAULT, NULL, + progress, argv[1], async_ready_func, argv_utf8); and then this: +#ifdef G_OS_WIN32 + g_free (argv_utf8); +#endif ie: you give the argv_utf8 as the user_data for the async result function, but then you free it immediately. By the time the result is reported, it's already gone. This is a pretty trivial issue imho since it's only in a test/demo program that will probably be erased once we create a proper 'gfile' tool, but if you want me to include a fix, I'm happy to do so if you write and test it. Cheers
Created attachment 255158 [details] [review] gio-du: Avoid freeing the UTF-8-fied arguments prematurely on Windows Hello Ryan, I messed up a bit, as you said-I did g_free'ed the argv_utf8 too early :) So, here's my fix for this... With blessings, thank you!