Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a --parallel flag #1142

Closed
tibbe opened this issue Dec 10, 2012 · 8 comments
Closed

Add a --parallel flag #1142

tibbe opened this issue Dec 10, 2012 · 8 comments
Assignees

Comments

@tibbe
Copy link
Member

tibbe commented Dec 10, 2012

We should add a --parallel flag that turns on the parallel build infrastructure. The purpose of having this flag in addition to --jobs is so users can enable it in ~/.cabal/config and still get the smart default number of jobs selection that's already implemented.

@ghost ghost assigned 23Skidoo Dec 10, 2012
@23Skidoo
Copy link
Member

Not that hard to add, but it might be confusing to have two flags for enabling the same functionality.

@tibbe
Copy link
Member Author

tibbe commented Dec 11, 2012

@23Skidoo This idea comes from the (very long) discussion we had about how to allow users to enable parallel builds by default in ~/.cabal/config without actually specifying the number of jobs in that file.

@23Skidoo
Copy link
Member

the (very long) discussion we had

That is, #1053. I have a patch that makes it possible to write jobs: $DEFAULT in the config file (#1124). This also works for all OptArg options.

The discussion in #1053 was about extending this syntax to other kinds of options (ReqArg, BoolOpt, ...), which is not easy to do.

@tibbe
Copy link
Member Author

tibbe commented Dec 13, 2012

I could swear we had a discussion where we decided to punt on the default value issue and settle for --parallel for now to solve the immediate problem, but I cannot find it on the bug tracker or in my email archive.

@23Skidoo
Copy link
Member

I don't feel strongly about the jobs: $DEFAULT vs. parallel: True issue. Maybe @dcoutts has an opinion?

$DEFAULT could be useful if it was extended to other option types, but implementing this is not easy.

@dcoutts
Copy link
Contributor

dcoutts commented Dec 13, 2012

I think the solution is not to try to pretend that there is a global $DEFAULT value, there isn't. But what is missing is that there is no value you can specify in --jobs=... that makes it behave the same as --jobs. The patch below adds such a value.

From e4afcecfaf521abee25aa84a4b5d1fb24adf70c3 Mon Sep 17 00:00:00 2001
From: Duncan Coutts <[email protected]>
Date: Thu, 13 Dec 2012 19:03:50 +0000
Subject: [PATCH] Add a default value for the --jobs flag

This will allow it to be set in config files too.
---
 cabal-install/Distribution/Client/Setup.hs |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/cabal-install/Distribution/Client/Setup.hs b/cabal-install/Distribution/Client/Setup.hs
index 37f66e8..e5e654a 100644
--- a/cabal-install/Distribution/Client/Setup.hs
+++ b/cabal-install/Distribution/Client/Setup.hs
@@ -846,13 +846,11 @@ installOptions showOrParseArgs =
           (yesNoOpt showOrParseArgs)

       , option "j" ["jobs"]
-        "Run NUM jobs simultaneously."
+        "Run NUM jobs simultaneously (or '$ncpus' if no NUM is given)."
         installNumJobs (\v flags -> flags { installNumJobs = v })
-        (optArg "NUM" (readP_to_E (\_ -> "jobs should be a number")
-                                  (fmap (toFlag . Just)
-                                        (Parse.readS_to_P reads)))
+        (optArg "NUM" (fmap Flag flagToJobs)
                       (Flag Nothing)
-                      (map (fmap show) . flagToList))
+                      (map (Just . maybe "$ncpus" show) . flagToList))
       ] ++ case showOrParseArgs of      -- TODO: remove when "cabal install" avoids
           ParseArgs ->
             [ option [] ["only"]
@@ -860,6 +858,18 @@ installOptions showOrParseArgs =
               installOnly (\v flags -> flags { installOnly = v })
               trueArg ]
           _ -> []
+  where
+    flagToJobs :: ReadE (Maybe Int)
+    flagToJobs = ReadE $ \s ->
+      case s of
+        "$ncpus" -> Right Nothing
+        _        -> case read s of
+          [(n, "")]
+            | n < 1     -> Left "The number of jobs should be 1 or more."
+            | n > 64    -> Left "You probably don't want that many jobs."
+            | otherwise -> Right (Just n)
+          _             -> Left "The jobs value should be a number or '$ncpus'"
+

 instance Monoid InstallFlags where
   mempty = InstallFlags {
-- 
1.7.7.6

@tibbe
Copy link
Member Author

tibbe commented Dec 13, 2012

We're doing this: 9926424

@tibbe tibbe closed this as completed Dec 13, 2012
@23Skidoo
Copy link
Member

@dcoutts

I think the solution is not to try to pretend that there is a global $DEFAULT value, there isn't.

In case of OptArg options the default value is a part of the option definition. For other types of options, we could use the values from defaultFooFlags.

The $DEFAULT feature (if extended to other types of options) could be useful for explicitly setting options in sandbox.config files to their default values (even if they are set to something else in ~/.cabal/config). The problem is that I haven't yet come up with a good design for how to make $DEFAULT work for all types of options (I sketched a solution here, but I don't really like it).

But what is missing is that there is no value you can specify in --jobs=... that makes it behave the same as --jobs. The patch below adds such a value.

Great, thanks. I like this better than a separate --parallel flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants