process library broken on Windows

Mon Nov 2 14:14:52 UTC 2015

On Mon, Nov 2, 2015 at 5:57 AM, Simon Peyton Jones <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] *On
> Behalf Of *Michael Snoyman
> *Sent:* 02 November 2015 13:42
>
> *To:* Simon Peyton Jones
> *Cc:* 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>
> 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] *On
> Behalf Of *Michael Snoyman
> *Sent:* 02 November 2015 13:34
> *To:* Simon Peyton Jones
> *Cc:* 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>
> 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>
> 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
> 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>
>
>
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.haskell.org/pipermail/ghc-devs/attachments/20151102/b2aa474e/attachment.html>