On Mon, Nov 2, 2015 at 5:57 AM, Simon Peyton Jones <[email protected]> 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:* [email protected] [mailto:[email protected]] *On > Behalf Of *Michael Snoyman > *Sent:* 02 November 2015 13:42 > > *To:* Simon Peyton Jones > *Cc:* [email protected] > *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 <[email protected]> > 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:* [email protected] [mailto:[email protected]] *On > Behalf Of *Michael Snoyman > *Sent:* 02 November 2015 13:34 > *To:* Simon Peyton Jones > *Cc:* [email protected] > *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 <[email protected]> > 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 <[email protected]> > 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 > [email protected] > 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 [email protected] http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
