From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23442 invoked by alias); 7 Nov 2011 18:10:12 -0000 Received: (qmail 23433 invoked by uid 22791); 7 Nov 2011 18:10:09 -0000 X-SWARE-Spam-Status: No, hits=-2.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,TW_NX X-Spam-Check-By: sourceware.org Received: from smtpout.karoo.kcom.com (HELO smtpout.karoo.kcom.com) (212.50.160.34) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 07 Nov 2011 18:09:54 +0000 Received: from 213-152-38-55.dsl.eclipse.net.uk (HELO [192.168.0.7]) ([213.152.38.55]) by smtpout.karoo.kcom.com with ESMTP; 07 Nov 2011 18:09:49 +0000 Message-ID: <4EB81EF8.5060107@dronecode.org.uk> Date: Mon, 07 Nov 2011 18:10:00 -0000 From: Jon TURNEY Reply-To: cygwin-xfree User-Agent: Mozilla/5.0 (Windows NT 5.1; rv:8.0) Gecko/20111103 Thunderbird/8.0 MIME-Version: 1.0 To: cygwin-xfree@cygwin.com CC: rpavlik@iastate.edu Subject: Re: Built XWin on mingw - with patches References: <4EB2E8FE.6020305@dronecode.org.uk> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Mailing-List: contact cygwin-xfree-help@cygwin.com; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: cygwin-xfree-owner@cygwin.com Reply-To: cygwin-xfree@cygwin.com Mail-Followup-To: cygwin-xfree@cygwin.com X-SW-Source: 2011-11/txt/msg00014.txt.bz2 On 04/11/2011 23:39, Ryan Pavlik wrote: > On Thu, Nov 3, 2011 at 2:18 PM, Jon TURNEY wrote: >> On 01/11/2011 20:39, Ryan Pavlik wrote: >>> specific and probably more appropriate than the overall xorg lists. >>> I wanted to build a native windows X server (essentially an >>> open-source Xming). I had to patch a number of the packages along the >>> way, but did eventually arrive at a build that worked. I looked at >>> and used a few of the Xming patches, but only generally went to those >>> when the naive approach didn't work. >> >> This may be a problem. >> >> The material on the Xming website is licensed under a Creative Commons license, which is not compatible with the X11 license. So, patches from the Xming website are not acceptable unless the author has agreed to re-license them appropriately. > > I emailed him and he indicated that he already has a process worked > out for re-licensing a patch at a time to submit to you. > > As such, I have re-worked my patch queue to remove all patches derived > from the Xming web site. It doesn't add everything that the previous > branch did, but it does build and start to work, and all these patches > are licensed like the X11 source itself as they are my work. I also > re-worked the commit messages and patches a bit, and added some > additional patches. Okay, thank you. > https://github.com/rpavlik/Xserver/tree/cleanpatches-1.11-branch > >> I've quickly looked over these patches and in general they look good, and I'd be happy to help get them merged upstream. >> >> A couple of general points I'd make though: >> >> It helps a great deal in reviewing if the comments state why the change is a good idea (e.g. what problem it fixes), rather than just describing the change which is made. > > I've revised the commit messages for xorg-server accordingly - > hopefully these are better. > >> >> I also noticed that a bit more care might be needed with the define you are using to enable platform specific code: WIN32 and __MINGW__ should not be used interchangeably. WIN32 will also be defined when building VcXsrv, and neither is defined on Cygwin. >> > > Ah, true. I have gone through and improved this in my cleanpatches > branch, and have also added some convenience defines to make this > easier to get right - this is the last half of the commits. I see what you are trying to do here, but I'm not sure it actually adds any clarity. I think I'd just prefer to assume the knowledge that WIN32 and CYGWIN are mutually exclusive, so '#if defined(WIN32) && !defined(__CYGWIN__)' can just be written '#ifdef WIN32' >> So, can you post your patches here, preferably in git-send-email format so we can review them in detail? > > I wasn't sure if you wanted one email per patch, and 43 emails seemed > like an awful lot, so I've attached them all (git format-patch) to > this one. If you prefer, I can send them individually to this (or > another) list with git send-email. These are just the xserver > patches, not the ones for other packages that I mentioned in the > mingw-cross-env repository. Sorry, I should have mentioned this before, but please can you add a 'Signed-off-by' line to these (git commit --amend --signoff will add one for you) A few comments, I'll take a deeper look later: 0001-os-osinit.c-Exclude-new-signal-sigaction-code-on-non.patch Shouldn't this be X_NOT_POSIX rather than X_NO_POSIX? 0006-hw-xwin-Makefile.am-Include-manifest-in-the-dist-tar.patch Good catch! :-) 0009-os-utils.c-Use-winxp-or-better-for-Winsock-API.patch I am a bit unclear why this is needed, surely the winsock API predates XP? It might be better to add this define to CFLAGS rather than to start sprinkling it around source files as needed? 0013-hw-xwin-InitOutput.c-Remove-duplicated-code-for-sett.patch Comment should probably say 'Consolidate duplicate code' rather than 'Remove' It seems this changes more than that, though, as it now looks for the files in both PROJECTROOT and basedir? 0017-dix-registry.c-non-cygwin-find-protocol.txt-in-reloc.patch I think the answer to the question 'Should this actually be checking RELOCATE_PROJECTROOT ?' is yes I think it would probably be neater to do something like arrange for FILENAME to start with the platform-appropriate path separator, rather than to define FILENAME_ONLY as the same name without an initial path separator? 0027-dix-registry.c-Free-old-memory-upon-realloc-failure.patch Interesting. It would probably be useful to quote the language from the appropriate standard which describes the behavior of realloc() in this error case in the comment. I don't think this change is fully correct however. If the realloc'ed size is 0, realloc() may return NULL, but the previously allocated memory has been freed. Perhaps you need to check if errno has been set by realloc to distinguish these two cases? Did you notice this by inspection or actually have a problem caused by this code? Have you audited the rest of the xserver code for this class of error? 0041-configure.ac-mingw-doesn-t-have-setuid-either.patch Use whitespace consistently with the context, please -- Jon TURNEY Volunteer Cygwin/X X Server maintainer -- Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple Problem reports: http://cygwin.com/problems.html Documentation: http://x.cygwin.com/docs/ FAQ: http://x.cygwin.com/docs/faq/