check that the number of parallel build is greater than 0 (fixes #12062)
ClosedPublic

Authored by petercommand on Jul 20 2016, 11:04 AM.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
petercommand edited the test plan for this revision. (Show Details)
petercommand updated the Trac tickets for this revision.
bgamari requested changes to this revision.Jul 20 2016, 11:11 AM
bgamari edited edge metadata.

Just two minor points. Thanks for fixing this!

compiler/main/DynFlags.hs
2348

Strictly speaking the do is unnecessary.

2355

Perhaps we should warn in this case?

This revision now requires changes to proceed.Jul 20 2016, 11:11 AM
petercommand edited edge metadata.

fix all.T

petercommand edited edge metadata.

remove unnecessary do & add warning for the Nothing case

bgamari accepted this revision.Jul 20 2016, 11:25 AM
bgamari edited edge metadata.

Looks good to me.

This revision is now accepted and ready to land.Jul 20 2016, 11:25 AM
bgamari added a subscriber: simonmar.

@simonmar, look reasonable to you?

thomie requested changes to this revision.Jul 20 2016, 12:58 PM
thomie added a reviewer: thomie.

What's with the Template Haskell in the test? Is that necessary?

compiler/main/DynFlags.hs
2355

No, the Int is optional for a reason, you're removing a feature here.

In compiler/main/GhcMake.hs:

n_jobs <- case parMakeCount dflags of
                Nothing -> liftIO getNumProcessors
                Just n  -> return n

Using the -j flag, you can compile modules in parallel.

This works the same way as +RTS -N.

This revision now requires changes to proceed.Jul 20 2016, 12:58 PM
bgamari added inline comments.Jul 20 2016, 1:29 PM
compiler/main/DynFlags.hs
2355

No, the Int is optional for a reason, you're removing a feature here.

Oh dear, thank you for pointing this out, @thomie. I had suspected that but the implementation showed no sign of this being implementation. Looks like I should have gone looking elsewhere.

Sorry for misleading you, @petercommand. Do you suppose that you could revert to the previous behavior and add a short comment making it clear what it happening here?

testsuite/tests/warnings/should_compile/T12062/T12062.hs
6 ↗(On Diff #8291)

Indeed this is a rather elaborate way of realizing a "no-op" program. I had assumed it was due to needing multiple modules to trigger the floating point exception but it would be good to clarify. @petercommand, could you elaborate?

bgamari requested changes to this revision.Jul 20 2016, 2:36 PM
bgamari edited edge metadata.
petercommand marked 2 inline comments as done.Jul 20 2016, 9:15 PM
petercommand added inline comments.
testsuite/tests/warnings/should_compile/T12062/T12062.hs
6 ↗(On Diff #8291)

It is because the trac issue uses this example to reproduce the problem, so I simply used it as the test case
Seems that the issue cannot be reproduced with only a single module

simonmar requested changes to this revision.Jul 21 2016, 4:45 AM
simonmar edited edge metadata.
simonmar added inline comments.
compiler/main/DynFlags.hs
2351–2355

Replace both of these with something like "Syntax: -j<n> where n > 0"

I dislike overly chatty interfaces.

testsuite/tests/warnings/should_compile/T12062/T12062.hs
6 ↗(On Diff #8291)

I'm not sure we really need a multi-module Template Haskell test case to fix a division by zero. A bit overkill perhaps?

I would put a test in tests/driver, using a single empty source file and just test that we get an error for -j0.

bgamari added inline comments.Jul 21 2016, 5:01 AM
compiler/main/DynFlags.hs
2351–2355

@thomie pointed out that -j without a count is a perfect valid argument. Otherwise your suggestion for the first branch sounds fine.

testsuite/tests/warnings/should_compile/T12062/T12062.hs
6 ↗(On Diff #8291)

I'm not sure we really need a multi-module Template Haskell test case to fix a division by zero. A bit overkill perhaps?

@simonmar, I have confirmed that there really doesn't seem to be a good way to trigger this without TH.

simonmar added inline comments.Jul 21 2016, 5:40 AM
testsuite/tests/warnings/should_compile/T12062/T12062.hs
6 ↗(On Diff #8291)

I am not at all surprised that using -j0 leads to a division by zero, I'm slightly surprised that it needs a bit of TH to trigger it but is it really all that important? Just validate the input, test that we're validating the input, and we're done.

Maybe I should shut up now, we're wasting far too many bytes on this division by zero :)

petercommand edited edge metadata.

remove the warning when the number of parallel builds is not specified

petercommand marked 2 inline comments as done.Jul 21 2016, 10:42 AM
petercommand added inline comments.
compiler/main/DynFlags.hs
2355

@bgamari The previous behavior is not correct either. It needs to be updated to Nothing, since the default value is (Just 1). I have updated the diff to include this change

thomie accepted this revision.Jul 21 2016, 10:49 AM
thomie edited edge metadata.

Thanks again for stepping up to fix this, @petercommand.

compiler/main/DynFlags.hs
2355

Ahh, indeed. Thanks @petercommand!

testsuite/tests/warnings/should_compile/T12062/T12062.hs
6 ↗(On Diff #8291)

I am not at all surprised that using -j0 leads to a division by zero, I'm slightly surprised that it needs a bit of TH to trigger it but is it really all that important?

Fair point; I suppose as long as it fails one way or another the test has fulfilled its purpose.

hmm...so are there any further changes needed?

bgamari accepted this revision.Jul 21 2016, 12:14 PM
bgamari edited edge metadata.

I think that is all. Thanks @petercommand!

petercommand added inline comments.Jul 24 2016, 8:37 AM
compiler/main/DynFlags.hs
2355

@simonmar perhaps we should mention that the n in -j<n> is optional?

simonmar added inline comments.Jul 25 2016, 8:47 AM
compiler/main/DynFlags.hs
2355

Indeed, could you update the docs please?

petercommand added inline comments.Jul 25 2016, 9:08 AM
compiler/main/DynFlags.hs
2355

@simonmar You mean update the warning message?

simonmar added inline comments.Jul 25 2016, 3:17 PM
compiler/main/DynFlags.hs
2350–2352

This should be an error, and I suggest "Syntax: -j[n] where n > 0"

2355

Yes (see other comment) and also the docs for -j in the User Guide.

petercommand edited edge metadata.

change the warning to error & update the error message

petercommand edited edge metadata.

update the test suite & change the error back to warning & update the comment in the Nothing case

petercommand added inline comments.Jul 25 2016, 8:39 PM
compiler/main/DynFlags.hs
2352

No, this not an error, this is the case when n is not specified. I've updated the comment in the code to make it more clear.

petercommand edited edge metadata.

update User's Guide

simonmar requested changes to this revision.Jul 26 2016, 8:57 AM
simonmar edited edge metadata.
simonmar added inline comments.
compiler/main/DynFlags.hs
2352

I was referring to the addWarn above (maybe my comment got moved by Phabricator). Instead of a warning, it should be an error to use zero or a negative argument to -j.

docs/users_guide/using.rst
421–422

"omitted" would be clearer than "not specified"

422

*defaults

448

This is the canonical documentation for -j, could you also document the syntax here please?

This revision now requires changes to proceed.Jul 26 2016, 8:57 AM
petercommand edited edge metadata.

update User's Guide & change warning to error

After changing the code to emit an error, where should I put the test folder T12062? It should definitely be moved out of the warnings folder.

After changing the code to emit an error, where should I put the test folder T12062?

tests/driver has tests for compiler command-line options.

petercommand edited edge metadata.

update testsuite

simonmar accepted this revision.Jul 27 2016, 11:22 AM
simonmar edited edge metadata.

lgtm

This revision is now accepted and ready to land.Jul 27 2016, 11:22 AM
This revision was automatically updated to reflect the committed changes.