process library broken on Windows

Simon Marlow marlowsd at gmail.com
Mon Nov 2 14:44:41 UTC 2015


-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
>


More information about the ghc-devs mailing list