process library broken on Windows

Michael Snoyman michael at snoyman.com
Mon Nov 2 21:32:30 UTC 2015


I'd rather not change the public API (even though it's an internal module)
in a way we'll later undo to avoid a warning, when the code cleanup should
achieve the goal without making that modification. I've pushed my changes
to the less-cpp branch, if anyone wants to play with the changes thus far.
I've also modified the Travis build to use -Wall -Werror, and added an
AppVeyor Windows build that similarly uses -Werror.

On Mon, Nov 2, 2015 at 6:44 AM, Simon Marlow <marlowsd at gmail.com> wrote:

> -Wwarn shouldn't really be in source code.  Since it's an Internals
> module, I would just export it.  Maybe add a {-# WARNING #-} to discourage
> people from using it.
>
> Cheers
> Simon
>
> On 02/11/2015 06:14, Michael Snoyman wrote:
>
>>
>>
>> On Mon, Nov 2, 2015 at 5:57 AM, Simon Peyton Jones
>> <simonpj at microsoft.com <mailto:simonpj at microsoft.com>> wrote:
>>
>>     Aha.  It would be great to say all that in the source code!!  It’s
>>     very non-obvious that you not want people ever to construct a CGId
>>     on Windows.  After all, it has a newtype definition!____
>>
>>     __
>>
>>
>> Good call, I'll update with some comments (though see refactoring
>> comments below).
>>
>>     __
>>
>>     Could you declare it differently?____
>>
>>          data CGId   -- No constructors____
>>
>>     __
>>
>>
>> Certainly we could. Then the question would be "why does the Windows
>> code look so different?" There are lots of colors to this bikeshed, and
>> I don't have any particular affinity to any of them. If you'd prefer it
>> looked that way, I can make that change. I initially erred with making
>> the code as similar to the POSIX code as possible.
>>
>>     __
>>
>>     Also if so, why does the Windows-specific foreign import of
>>     c_runInteractiveProcess (lin 440-is) pas a Ptr CGId? You’d just told
>>     me that we can never create one.____
>>
>>     __
>>
>>
>>
>> The Windows-specific foreign import is on line ~533, and does not
>> include those arguments.
>>
>>     __
>>
>>     Also, ____
>>
>>     __·__It’d help to comment the #else on line 456 as being “else if
>>     not windows”____
>>
>>     __·__The #endifs on line 546 or thereabouts are mis-labelled.  at
>>     the moment the second says “GLASGOW_HASKELL” but actually it’s the
>>     first____
>>
>>     __
>>
>>
>>
>> Agreed that the code is fairly difficult to follow with the nested
>> ifdefs. However, instead of trying to salvage that, it's probably worth
>> a refactoring to put the Windows and POSIX code into separate modules
>> and then just import those conditionally.
>>
>>     __
>>
>>     I have no opinion about the best solution; I’d just like it to
>>     compile and preferably warning free since that is our default
>>     policy.  Or add a –Wwarn at the top.____
>>
>>     __
>>
>>
>> If you're looking for a short-term solution, I can add -Wwarn to the top
>> until some kind of refactoring takes place.
>>
>>     __
>>
>>     thanks____
>>
>>     __ __
>>
>>     Simon____
>>
>>     __ __
>>
>>     __ __
>>
>>     *From:*michael.snoyman at gmail.com <mailto:michael.snoyman at gmail.com>
>>     [mailto:michael.snoyman at gmail.com
>>     <mailto:michael.snoyman at gmail.com>] *On Behalf Of *Michael Snoyman
>>     *Sent:* 02 November 2015 13:42
>>
>>
>>     *To:* Simon Peyton Jones
>>     *Cc:* ghc-devs at haskell.org <mailto:ghc-devs at haskell.org>
>>     *Subject:* Re: process library broken on Windows____
>>
>>     __ __
>>
>>     That's the goal; it's a feature that does not work on Windows, only
>>     on non-Windows systems (setuid/setgid for a child process). For
>>     POSIX systems, CGid is exported from base, and can be used. On
>>     Windows, the data type is present to give the same signature, but
>>     the constructor itself is not exported to prevent using the feature.
>>     An argument could be made for other approaches:____
>>
>>     __ __
>>
>>     1. Expose the constructor, allowing users to set a value, and that
>>     value will be ignored____
>>
>>     2. Make the fields themselves conditional on the OS being used____
>>
>>     __ __
>>
>>     I don't think there's a strong argument in any direction for this.____
>>
>>     __ __
>>
>>     On Mon, Nov 2, 2015 at 5:37 AM, Simon Peyton Jones
>>     <simonpj at microsoft.com <mailto:simonpj at microsoft.com>> wrote:____
>>
>>         I’m puzzled.   Internals.hs defines a newtype____
>>
>>         ____
>>
>>         newtype CGid = CGid Word32____
>>
>>         ____
>>
>>         A value of this type is needed to fill in the child_group field
>>         of CreateProcess.  If you don’t export it, you could never
>>         initialise this field to anything other than Nothing, so why do
>>         you have it?____
>>
>>         ____
>>
>>         Looks to me as if the warning has nailed a real bug____
>>
>>         ____
>>
>>         Simon____
>>
>>         ____
>>
>>         *From:*michael.snoyman at gmail.com
>>         <mailto:michael.snoyman at gmail.com>
>>         [mailto:michael.snoyman at gmail.com
>>         <mailto:michael.snoyman at gmail.com>] *On Behalf Of *Michael
>> Snoyman
>>         *Sent:* 02 November 2015 13:34
>>         *To:* Simon Peyton Jones
>>         *Cc:* ghc-devs at haskell.org <mailto:ghc-devs at haskell.org>
>>         *Subject:* Re: process library broken on Windows____
>>
>>         ____
>>
>>         I didn't read closely enough: I see now that it's a warning, not
>>         an error. I initially didn't export that constructor since it's
>>         only present on Windows for API compatibility, but will never be
>>         used. Since this is just the internals module, I can export it,
>>         but my preference would in fact be to leave it as-is with the
>>         warning. Two alternatives:____
>>
>>         ____
>>
>>         1. Create a new hidden module that creates and exports the type
>>         constructor, just to hide the warning. I'm -1 on that, since
>>         that's extra compile time everyone has to endure just for
>>         warning avoidance.____
>>
>>         2. base could export CGid for Windows (currently, it does
>> not).____
>>
>>         ____
>>
>>         On Mon, Nov 2, 2015 at 5:17 AM, Michael Snoyman
>>         <michael at snoyman.com <mailto:michael at snoyman.com>> wrote:____
>>
>>             I'll look into this. I just made a new release of process,
>>             and was certain I tested on Windows, but perhaps something
>>             changed between that commit and release.____
>>
>>             ____
>>
>>             On Mon, Nov 2, 2015 at 5:15 AM, Simon Peyton Jones
>>             <simonpj at microsoft.com <mailto:simonpj at microsoft.com>>
>>             wrote:____
>>
>>                 I’m getting this on HEAD in te ‘____
>>
>>                 libraries\process\System\Process\Internals.hs:106:16:
>>                 warning:____
>>
>>                      Defined but not used: data constructor ‘CGid’____
>>
>>                 Indeed it looks as if CGId(..) should be exported, else
>>                 createProcess is unusuable.  This looks like the right
>>                 change.  Would someone like to check and make the
>> change____
>>
>>                 Simon____
>>
>>                 diff --git a/System/Process/Internals.hs
>>                 b/System/Process/Internals.hs____
>>
>>                 index 5575ac4..3e23ad5 100644____
>>
>>                 --- a/System/Process/Internals.hs____
>>
>>                 +++ b/System/Process/Internals.hs____
>>
>>                 @@ -37,6 +37,8 @@ module System.Process.Internals (____
>>
>>                 #if !defined(mingw32_HOST_OS) && !defined(__MINGW32__)____
>>
>>                       pPrPr_disableITimers, c_execvpe,____
>>
>>                       ignoreSignal, defaultSignal,____
>>
>>                 +#else____
>>
>>                 +    CGid(..), GroupID, UserID,____
>>
>>                 #endif____
>>
>>                       withFilePathException, withCEnvironment,____
>>
>>                       translate,____
>>
>>                 ____
>>
>>                 _______________________________________________
>>                 ghc-devs mailing list
>>                 ghc-devs at haskell.org <mailto:ghc-devs at haskell.org>
>>                 http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
>> <
>> https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fmail.haskell.org%2fcgi-bin%2fmailman%2flistinfo%2fghc-devs&data=01%7c01%7csimonpj%40064d.mgd.microsoft.com%7cdd25a0d693de489171bb08d2e38a505d%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=YdqpMC5wr2K6oUOw9WImRGpSl6EsV8zQyAK%2ba26oF9M%3d
>> >____
>>
>>             ____
>>
>>         ____
>>
>>     __ __
>>
>>
>>
>>
>> _______________________________________________
>> ghc-devs mailing list
>> ghc-devs at haskell.org
>> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.haskell.org/pipermail/ghc-devs/attachments/20151102/9eba3ff5/attachment-0001.html>


More information about the ghc-devs mailing list