GNOME Bugzilla – Bug 701707
Allow Broadway to compile & run on Win32
Last modified: 2013-07-01 12:53:32 UTC
Created attachment 246140 [details] [review] gtk382-broadway-win32.patch Please consider this patch, which allows Broadway to compile and run under Win32. It enables both the GDK Backend and the "broadwayd" server. Here is a screenshot : http://www.tarnyko.net/repo/gtk382-broadway-win32.png Some tweaks were necessary. Here is a summary of all important modifications : 1) Many UNIX-specific functions (mmap, shm_open...) converted to Windows equivalents ; 2) Original code calls "gio_unix_socket*" functions to use UNIX sockets, which are special files acting for inter-communication. Windows does not not have such ; sockets can only be TCP/IP ones. As we do not want to do RPC (not supported by GLib, and specific) we do the following : - open a socket on local loopback (127.0.0.1), inaccessible from the outside ; - use a port starting from 9090. To avoid eating up a port which the user could need, I created a "BROADWAY_LOCALPORT" environment variable able to redefine it. Usage : set BROADWAY_LOCALPORT=<myport> 3) Original code uses "/dev/shm/bwd-XXXXX" objects for data persistence. They obviously don't exist on Windows, so we create temporary files in "$ROOT/.bwd" instead. PS : I tried to use user temp dir ("\Users\foobar\Local\Temp\bwd-XXXXX" e.g.), or even current dir, but doesn't seem to work with long names/paths. Sidenotes : a) New code uses lots of #ifdefs. If we want to be cleaner, we could dispatch platform-specific functions in different files, such as : - broadway_unix.c - broadway_win32.c However I need some feedback on that matter. b) broadwayd server needs to be run manually before we can use the backend at all. Is it able to start automatically on Linux ? If it is, and we want that, it's trivial to implement.
Review of attachment 246140 [details] [review]: ::: gtk+-3.8.2/gdk/broadway/broadwayd.c @@ +1,3 @@ #include "config.h" #include <string.h> +#ifndef _WIN32 Add a configure check for the header instead and use #ifdef HAVE_SYS_MMAN_H @@ +11,3 @@ #include <glib.h> #include <gio/gio.h> +#ifndef _WIN32 Check for G_OS_UNIX instead here. @@ +496,3 @@ + if (local_port <= 0) + local_port = 1; +#endif I don't think this should be win32 specific. Instead make the tcp code work on unix too and extend the support for the display string to be able to define both tcp and unix sockets, plus the actual port used in tcp. Something like BROADWAY_DISPLAY=tcp:9090 or BROWADWAY_DISPLAY=unix:0 Then you can wrap the unix code in #ifdef G_OS_UNIX @@ +1880,3 @@ + if (bwdpath == NULL) + bwdpath = g_strdup ("c:"); + strncat (bwdpath, G_DIR_SEPARATOR_S, PATH_MAX); This looks like its overwriting bwpath which points into the environment. @@ +1900,3 @@ + strncat (bwdpath, G_DIR_SEPARATOR_S, PATH_MAX); + strncat (bwdpath, name, PATH_MAX); + fd = open(bwdpath, O_RDONLY, 0600); Can you put this all into a helper function. @@ +1925,3 @@ + ptr = MapViewOfFile (fm, FILE_MAP_READ, 0, 0, (size_t)size); + CloseHandle (fm); + } Helper function here too @@ +57,3 @@ + if (bwdpath == NULL) + bwdpath = g_strdup ("c:"); + strncat (bwdpath, G_DIR_SEPARATOR_S, PATH_MAX); Seems to overwrite the environment here too. @@ +698,3 @@ + } + pos_t += w; + } Is this needed on win32? The fallocate was mainly to get correct error reporting wrt shared memory. It was not counted as "used" until it was allocated. @@ +382,3 @@ GDK_BACKENDS="$GDK_BACKENDS broadway" cairo_backends="$cairo_backends cairo" + have_gio_unix=no This should not *unset* have_gio_unix ever. But we should only set it if OS_UNIX
(In reply to comment #1) > @@ +496,3 @@ > + if (local_port <= 0) > + local_port = 1; > +#endif > > I don't think this should be win32 specific. Instead make the tcp code work on > unix too and extend the support for the display string to be able to define > both tcp and unix sockets, plus the actual port used in tcp. > > Something like BROADWAY_DISPLAY=tcp:9090 or BROWADWAY_DISPLAY=unix:0 > > Then you can wrap the unix code in #ifdef G_OS_UNIX Nice idea. Seems to be a lot cleaner, agreed. > @@ +1880,3 @@ > + if (bwdpath == NULL) > + bwdpath = g_strdup ("c:"); > + strncat (bwdpath, G_DIR_SEPARATOR_S, PATH_MAX); > > This looks like its overwriting bwpath which points into the environment. bwdpath is g_getenv ("HOMEDRIVE"). Maybe I can come up with a cleaner function, will look at that. What bothers me is that it crashes with a path containing spaces or special characters, like the one returned by "g_get_temp_dir ()". > @@ +698,3 @@ > + } > + pos_t += w; > + } > > Is this needed on win32? The fallocate was mainly to get correct error > reporting wrt shared memory. It was not counted as "used" until it was > allocated. Good point, I don't really know. True that we are using a standard file here. I will remove it and make tests. > @@ +382,3 @@ > GDK_BACKENDS="$GDK_BACKENDS broadway" > cairo_backends="$cairo_backends cairo" > + have_gio_unix=no > > This should not *unset* have_gio_unix ever. But we should only set it if > OS_UNIX *facepalm* Big error on my part. Totally ok with everything else. I will come back with a better patch soon.
> bwdpath is g_getenv ("HOMEDRIVE"). Yes, which is not a pointer you own, but rather a pointer into the environment block. Then this: strncat (bwdpath, G_DIR_SEPARATOR_S, PATH_MAX); Will *write* to whatever happens to be after the path in what it points to. Not sure how win32 works, but on unix this would be other environment variables. Of course you als do: + if (bwdpath == NULL) + bwdpath = g_strdup ("c:"); and at this point it points to an allocation you own, but it is only 3 bytes long. You can't write stuff after that...
(In reply to comment #3) > Yes, which is not a pointer you own, but rather a pointer into the environment > block. Ouch. > it points to an allocation you own, but it is only 3 bytes > long. You can't write stuff after that... Re-ouch. I think I get it. I should declare "char bwdpath[PATH_MAX]" and concatenate strings into it, instead of directly using functions that will make it point to random memory blocks. Thanks for being so helpful. It boosts things up a lot, I should be able to repost a corrected version quickly.
Created attachment 246597 [details] [review] gtk382-broadway-win32.patch
(In reply to comment #3) Hi, Please find attached a revised patch, modified regarding your suggestions, and which I hope meets good quality standards. All repetitive cross-platform #ifdefs have been put in helper functions ("mmap_cross", "shm_open_read_cross", ...). String manipulation problems have been corrected. Temp files are now put in "g_get_tmp_dir" (thanks !). Broadway clients and servers now recognize the following syntax : set BROADWAY_LOCALPORT=unix set BROADWAY_LOCALPORT=tcp:<port> (you can put a number after "unix", "unix:1000" e.g., it won't change the behavior. We don't really need to customize the UNIX socket number, as the OS takes care of this, don't we ?) Win32 always uses "tcp:<port>", even if we force "unix", falling back to defaults.
Created attachment 246616 [details] [review] gtk382-broadway-win32.patch
(In reply to comment #6) My bad, I forgot the HAVE_SYS_MMAN_H code in configure.ac. Fixed. BTW, you were right about posix_fallocate(). It works very well without replacement code, so I removed it totally. I only modify "gdkdisplay-broadway.c" and "gdkwindow-broadway.c" because there are unix-specific socket headers therein ("netinet/in.h"). Are they really needed ? I read the code without finding any network code in it. If we can remove the headers, we can remove the win32 #ifdefs too.
I commited a series of patches based on this to master. Not tested on win32 though. can you verify it works?
Created attachment 247515 [details] [review] master-broadway-win32.patch
(In reply to comment #9) > I commited a series of patches based on this to master. Not tested on win32 > though. can you verify it works? Hi Alex, tested on yesterday's Git pull, fully works with following fixes : - missing semicolon at the end of one line ; - in Makefile.am, another reference to librt which I didn't see ; - unimportant error in one comment :-). I've attached a patch, if you'd like to review it.
Created attachment 247612 [details] [review] Allow Broadway to be built in Visual Studio Hi, I have managed to build and run Broadway on my Windows 7, which I have updated a little bit (since Alex's commit of Tarnyko's changes to master) so that it will also compile with Visual Studio, in both x86 and x64 flavours. In all to say, kudos to Tarnyko for the work as it mainly ran quite alright on Windows in my brief tests. Here's my patch for allowing Broadway to enable it to build under Visual Studio as well. I will update the Visual Studio Project files when my patches for this is ok for inclusion. p.s. This also covers Tarnyko's later patches to make up for the missed ';' in two places. With blessings, thank you!
Created attachment 247616 [details] [review] Include a crypt(3) implementation for MSVC builds Hi, As Visual Studio does not support crypt(3) out of the box, I thought it might be a good idea to include sources of suitable implementation that is available in the public domain and is not bound by licensing concerns in build/win32 so that Visual Studio builds can make use of it, so that password authentication can still be done in broadwayd. Here's the patch for inclusion of it in build/win32. Hopefully it is okay for inclusion legally and technically, as that is the impression I have when I looked at the code :) With blessings, thank you!
all patches pushed to master