<div dir="ltr">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.</div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Nov 2, 2015 at 6:44 AM, Simon Marlow <span dir="ltr"><<a href="mailto:marlowsd@gmail.com" target="_blank">marlowsd@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">-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.<br>
<br>
Cheers<br>
Simon<span class=""><br>
<br>
On 02/11/2015 06:14, Michael Snoyman wrote:<br>
</span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
<br>
<br>
On Mon, Nov 2, 2015 at 5:57 AM, Simon Peyton Jones<br></span><span class="">
<<a href="mailto:simonpj@microsoft.com" target="_blank">simonpj@microsoft.com</a> <mailto:<a href="mailto:simonpj@microsoft.com" target="_blank">simonpj@microsoft.com</a>>> wrote:<br>
<br>
Aha. It would be great to say all that in the source code!! It’s<br>
very non-obvious that you not want people ever to construct a CGId<br></span>
on Windows. After all, it has a newtype definition!____<br>
<br>
__<span class=""><br>
<br>
<br>
Good call, I'll update with some comments (though see refactoring<br>
comments below).<br>
<br></span>
__<br>
<br>
Could you declare it differently?____<br>
<br>
data CGId -- No constructors____<br>
<br>
__<span class=""><br>
<br>
<br>
Certainly we could. Then the question would be "why does the Windows<br>
code look so different?" There are lots of colors to this bikeshed, and<br>
I don't have any particular affinity to any of them. If you'd prefer it<br>
looked that way, I can make that change. I initially erred with making<br>
the code as similar to the POSIX code as possible.<br>
<br></span>
__<span class=""><br>
<br>
Also if so, why does the Windows-specific foreign import of<br>
c_runInteractiveProcess (lin 440-is) pas a Ptr CGId? You’d just told<br></span>
me that we can never create one.____<br>
<br>
__<span class=""><br>
<br>
<br>
<br>
The Windows-specific foreign import is on line ~533, and does not<br>
include those arguments.<br>
<br></span>
__<br>
<br>
Also, ____<br>
<br>
__·__It’d help to comment the #else on line 456 as being “else if<br>
not windows”____<br>
<br>
__·__The #endifs on line 546 or thereabouts are mis-labelled. at<span class=""><br>
the moment the second says “GLASGOW_HASKELL” but actually it’s the<br></span>
first____<br>
<br>
__<span class=""><br>
<br>
<br>
<br>
Agreed that the code is fairly difficult to follow with the nested<br>
ifdefs. However, instead of trying to salvage that, it's probably worth<br>
a refactoring to put the Windows and POSIX code into separate modules<br>
and then just import those conditionally.<br>
<br></span>
__<span class=""><br>
<br>
I have no opinion about the best solution; I’d just like it to<br>
compile and preferably warning free since that is our default<br></span>
policy. Or add a –Wwarn at the top.____<br>
<br>
__<span class=""><br>
<br>
<br>
If you're looking for a short-term solution, I can add -Wwarn to the top<br>
until some kind of refactoring takes place.<br>
<br></span>
__<br>
<br>
thanks____<br>
<br>
__ __<br>
<br>
Simon____<br>
<br>
__ __<br>
<br>
__ __<br>
<br>
*From:*<a href="mailto:michael.snoyman@gmail.com" target="_blank">michael.snoyman@gmail.com</a> <mailto:<a href="mailto:michael.snoyman@gmail.com" target="_blank">michael.snoyman@gmail.com</a>><br>
[mailto:<a href="mailto:michael.snoyman@gmail.com" target="_blank">michael.snoyman@gmail.com</a><br>
<mailto:<a href="mailto:michael.snoyman@gmail.com" target="_blank">michael.snoyman@gmail.com</a>>] *On Behalf Of *Michael Snoyman<br>
*Sent:* 02 November 2015 13:42<br>
<br>
<br>
*To:* Simon Peyton Jones<br>
*Cc:* <a href="mailto:ghc-devs@haskell.org" target="_blank">ghc-devs@haskell.org</a> <mailto:<a href="mailto:ghc-devs@haskell.org" target="_blank">ghc-devs@haskell.org</a>><br>
*Subject:* Re: process library broken on Windows____<br>
<br>
__ __<span class=""><br>
<br>
That's the goal; it's a feature that does not work on Windows, only<br>
on non-Windows systems (setuid/setgid for a child process). For<br>
POSIX systems, CGid is exported from base, and can be used. On<br>
Windows, the data type is present to give the same signature, but<br>
the constructor itself is not exported to prevent using the feature.<br></span>
An argument could be made for other approaches:____<br>
<br>
__ __<span class=""><br>
<br>
1. Expose the constructor, allowing users to set a value, and that<br></span>
value will be ignored____<br>
<br>
2. Make the fields themselves conditional on the OS being used____<br>
<br>
__ __<br>
<br>
I don't think there's a strong argument in any direction for this.____<br>
<br>
__ __<span class=""><br>
<br>
On Mon, Nov 2, 2015 at 5:37 AM, Simon Peyton Jones<br></span>
<<a href="mailto:simonpj@microsoft.com" target="_blank">simonpj@microsoft.com</a> <mailto:<a href="mailto:simonpj@microsoft.com" target="_blank">simonpj@microsoft.com</a>>> wrote:____<br>
<br>
I’m puzzled. Internals.hs defines a newtype____<br>
<br>
____<br>
<br>
newtype CGid = CGid Word32____<br>
<br>
____<span class=""><br>
<br>
A value of this type is needed to fill in the child_group field<br>
of CreateProcess. If you don’t export it, you could never<br>
initialise this field to anything other than Nothing, so why do<br></span>
you have it?____<br>
<br>
____<br>
<br>
Looks to me as if the warning has nailed a real bug____<br>
<br>
____<br>
<br>
Simon____<br>
<br>
____<br>
<br>
*From:*<a href="mailto:michael.snoyman@gmail.com" target="_blank">michael.snoyman@gmail.com</a><br>
<mailto:<a href="mailto:michael.snoyman@gmail.com" target="_blank">michael.snoyman@gmail.com</a>><br>
[mailto:<a href="mailto:michael.snoyman@gmail.com" target="_blank">michael.snoyman@gmail.com</a><br>
<mailto:<a href="mailto:michael.snoyman@gmail.com" target="_blank">michael.snoyman@gmail.com</a>>] *On Behalf Of *Michael Snoyman<br>
*Sent:* 02 November 2015 13:34<br>
*To:* Simon Peyton Jones<br>
*Cc:* <a href="mailto:ghc-devs@haskell.org" target="_blank">ghc-devs@haskell.org</a> <mailto:<a href="mailto:ghc-devs@haskell.org" target="_blank">ghc-devs@haskell.org</a>><br>
*Subject:* Re: process library broken on Windows____<br>
<br>
____<span class=""><br>
<br>
I didn't read closely enough: I see now that it's a warning, not<br>
an error. I initially didn't export that constructor since it's<br>
only present on Windows for API compatibility, but will never be<br>
used. Since this is just the internals module, I can export it,<br>
but my preference would in fact be to leave it as-is with the<br></span>
warning. Two alternatives:____<br>
<br>
____<span class=""><br>
<br>
1. Create a new hidden module that creates and exports the type<br>
constructor, just to hide the warning. I'm -1 on that, since<br>
that's extra compile time everyone has to endure just for<br></span>
warning avoidance.____<br>
<br>
2. base could export CGid for Windows (currently, it does not).____<br>
<br>
____<span class=""><br>
<br>
On Mon, Nov 2, 2015 at 5:17 AM, Michael Snoyman<br></span>
<<a href="mailto:michael@snoyman.com" target="_blank">michael@snoyman.com</a> <mailto:<a href="mailto:michael@snoyman.com" target="_blank">michael@snoyman.com</a>>> wrote:____<span class=""><br>
<br>
I'll look into this. I just made a new release of process,<br>
and was certain I tested on Windows, but perhaps something<br></span>
changed between that commit and release.____<br>
<br>
____<span class=""><br>
<br>
On Mon, Nov 2, 2015 at 5:15 AM, Simon Peyton Jones<br></span>
<<a href="mailto:simonpj@microsoft.com" target="_blank">simonpj@microsoft.com</a> <mailto:<a href="mailto:simonpj@microsoft.com" target="_blank">simonpj@microsoft.com</a>>><br>
wrote:____<br>
<br>
I’m getting this on HEAD in te ‘____<span class=""><br>
<br>
libraries\process\System\Process\Internals.hs:106:16:<br></span>
warning:____<br>
<br>
Defined but not used: data constructor ‘CGid’____<span class=""><br>
<br>
Indeed it looks as if CGId(..) should be exported, else<br>
createProcess is unusuable. This looks like the right<br></span>
change. Would someone like to check and make the change____<br>
<br>
Simon____<br>
<br>
diff --git a/System/Process/Internals.hs<br>
b/System/Process/Internals.hs____<br>
<br>
index 5575ac4..3e23ad5 100644____<br>
<br>
--- a/System/Process/Internals.hs____<br>
<br>
+++ b/System/Process/Internals.hs____<br>
<br>
@@ -37,6 +37,8 @@ module System.Process.Internals (____<br>
<br>
#if !defined(mingw32_HOST_OS) && !defined(__MINGW32__)____<br>
<br>
pPrPr_disableITimers, c_execvpe,____<br>
<br>
ignoreSignal, defaultSignal,____<br>
<br>
+#else____<br>
<br>
+ CGid(..), GroupID, UserID,____<br>
<br>
#endif____<br>
<br>
withFilePathException, withCEnvironment,____<br>
<br>
translate,____<br>
<br>
____<br>
<br>
_______________________________________________<br>
ghc-devs mailing list<br>
<a href="mailto:ghc-devs@haskell.org" target="_blank">ghc-devs@haskell.org</a> <mailto:<a href="mailto:ghc-devs@haskell.org" target="_blank">ghc-devs@haskell.org</a>><br>
<a href="http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs" rel="noreferrer" target="_blank">http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs</a> <<a href="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" rel="noreferrer" target="_blank">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</a>>____<br>
<br>
____<br>
<br>
____<br>
<br>
__ __<span class=""><br>
<br>
<br>
<br>
<br>
_______________________________________________<br>
ghc-devs mailing list<br>
<a href="mailto:ghc-devs@haskell.org" target="_blank">ghc-devs@haskell.org</a><br>
<a href="http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs" rel="noreferrer" target="_blank">http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs</a><br>
<br>
</span></blockquote>
</blockquote></div><br></div>