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