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