Support more ways to pass -j/-k

In preparation to remove Make/makeparallel from soong_ui startup, we
need to preserve compatibility with the different ways that make
supports the -j option.

Nothing changes unless Make/makeparallel is removed from the startup.
Once that is removed, not specifying a -j value will be equivalent to
'-j' instead of '-j1', like Ninja. A value will also be supported when
specifying -k, like Ninja (though specifying it alone will be equivalent
to '-k 0').

Test: m -j blueprint_tools
Change-Id: I9d5d59bedd4f6e5cca76bdb4cd47e0b5b7d523f0

3 files changed, 140 insertions, 21 deletions

diff --git a/ui/build/Android.bp b/ui/build/Android.bp
index 489c06dc..23a98721 100644
--- a/ui/build/Android.bp
+++ b/ui/build/Android.bp
@@ -37,6 +37,7 @@ bootstrap_go_package {
         "util.go",
     ],
     testSrcs: [
+        "config_test.go",
         "environment_test.go",
         "util_test.go",
         "proc_sync_test.go",
diff --git a/ui/build/config.go b/ui/build/config.go
index 16826f24..925b1538 100644
--- a/ui/build/config.go
+++ b/ui/build/config.go
@@ -53,6 +53,12 @@ func NewConfig(ctx Context, args ...string) Config {
 		environ: OsEnvironment(),
 	}
 
+	// Sane default matching ninja
+	ret.parallel = runtime.NumCPU() + 2
+	ret.keepGoing = 1
+
+	ret.parseArgs(ctx, args)
+
 	// Make sure OUT_DIR is set appropriately
 	if outDir, ok := ret.environ.Get("OUT_DIR"); ok {
 		ret.environ.Set("OUT_DIR", filepath.Clean(outDir))
@@ -97,10 +103,6 @@ func NewConfig(ctx Context, args ...string) Config {
 	// Tell python not to spam the source tree with .pyc files.
 	ret.environ.Set("PYTHONDONTWRITEBYTECODE", "1")
 
-	// Sane default matching ninja
-	ret.parallel = runtime.NumCPU() + 2
-	ret.keepGoing = 1
-
 	// Precondition: the current directory is the top of the source tree
 	if _, err := os.Stat(srcDirFileCheck); err != nil {
 		if os.IsNotExist(err) {
@@ -135,38 +137,49 @@ func NewConfig(ctx Context, args ...string) Config {
 		log.Fatalln("Directory names containing spaces are not supported")
 	}
 
-	for _, arg := range args {
-		arg = strings.TrimSpace(arg)
+	return Config{ret}
+}
+
+func (c *configImpl) parseArgs(ctx Context, args []string) {
+	for i := 0; i < len(args); i++ {
+		arg := strings.TrimSpace(args[i])
 		if arg == "--make-mode" {
 			continue
 		} else if arg == "showcommands" {
-			ret.verbose = true
+			c.verbose = true
 			continue
 		} else if arg == "dist" {
-			ret.dist = true
+			c.dist = true
 		}
 		if arg[0] == '-' {
-			var err error
+			parseArgNum := func(def int) int {
+				if len(arg) > 2 {
+					p, err := strconv.ParseUint(arg[2:], 10, 31)
+					if err != nil {
+						ctx.Fatalf("Failed to parse %q: %v", arg, err)
+					}
+					return int(p)
+				} else if i+1 < len(args) {
+					p, err := strconv.ParseUint(args[i+1], 10, 31)
+					if err == nil {
+						i++
+						return int(p)
+					}
+				}
+				return def
+			}
+
 			if arg[1] == 'j' {
-				// TODO: handle space between j and number
-				// Unnecessary if used with makeparallel
-				ret.parallel, err = strconv.Atoi(arg[2:])
+				c.parallel = parseArgNum(c.parallel)
 			} else if arg[1] == 'k' {
-				// TODO: handle space between k and number
-				// Unnecessary if used with makeparallel
-				ret.keepGoing, err = strconv.Atoi(arg[2:])
+				c.keepGoing = parseArgNum(0)
 			} else {
 				ctx.Fatalln("Unknown option:", arg)
 			}
-			if err != nil {
-				ctx.Fatalln("Argument error:", err, arg)
-			}
 		} else {
-			ret.arguments = append(ret.arguments, arg)
+			c.arguments = append(c.arguments, arg)
 		}
 	}
-
-	return Config{ret}
 }
 
 // Lunch configures the environment for a specific product similarly to the
diff --git a/ui/build/config_test.go b/ui/build/config_test.go
new file mode 100644
index 00000000..b2a4dd81
--- /dev/null
+++ b/ui/build/config_test.go
@@ -0,0 +1,105 @@
+// Copyright 2017 Google Inc. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package build
+
+import (
+	"bytes"
+	"context"
+	"reflect"
+	"strings"
+	"testing"
+
+	"android/soong/ui/logger"
+)
+
+func testContext() Context {
+	return Context{&ContextImpl{
+		Context:        context.Background(),
+		Logger:         logger.New(&bytes.Buffer{}),
+		StdioInterface: NewCustomStdio(&bytes.Buffer{}, &bytes.Buffer{}, &bytes.Buffer{}),
+	}}
+}
+
+func TestConfigParseArgsJK(t *testing.T) {
+	ctx := testContext()
+
+	testCases := []struct {
+		args []string
+
+		parallel  int
+		keepGoing int
+		remaining []string
+	}{
+		{nil, -1, -1, nil},
+
+		{[]string{"-j"}, -1, -1, nil},
+		{[]string{"-j1"}, 1, -1, nil},
+		{[]string{"-j1234"}, 1234, -1, nil},
+
+		{[]string{"-j", "1"}, 1, -1, nil},
+		{[]string{"-j", "1234"}, 1234, -1, nil},
+		{[]string{"-j", "1234", "abc"}, 1234, -1, []string{"abc"}},
+		{[]string{"-j", "abc"}, -1, -1, []string{"abc"}},
+		{[]string{"-j", "1abc"}, -1, -1, []string{"1abc"}},
+
+		{[]string{"-k"}, -1, 0, nil},
+		{[]string{"-k0"}, -1, 0, nil},
+		{[]string{"-k1"}, -1, 1, nil},
+		{[]string{"-k1234"}, -1, 1234, nil},
+
+		{[]string{"-k", "0"}, -1, 0, nil},
+		{[]string{"-k", "1"}, -1, 1, nil},
+		{[]string{"-k", "1234"}, -1, 1234, nil},
+		{[]string{"-k", "1234", "abc"}, -1, 1234, []string{"abc"}},
+		{[]string{"-k", "abc"}, -1, 0, []string{"abc"}},
+		{[]string{"-k", "1abc"}, -1, 0, []string{"1abc"}},
+
+		// TODO: These are supported in Make, should we support them?
+		//{[]string{"-kj"}, -1, 0},
+		//{[]string{"-kj8"}, 8, 0},
+
+		// -jk is not valid in Make
+	}
+
+	for _, tc := range testCases {
+		t.Run(strings.Join(tc.args, " "), func(t *testing.T) {
+			defer logger.Recover(func(err error) {
+				t.Fatal(err)
+			})
+
+			c := &configImpl{
+				parallel:  -1,
+				keepGoing: -1,
+			}
+			c.parseArgs(ctx, tc.args)
+
+			if c.parallel != tc.parallel {
+				t.Errorf("for %q, parallel:\nwant: %d\n got: %d\n",
+					strings.Join(tc.args, " "),
+					tc.parallel, c.parallel)
+			}
+			if c.keepGoing != tc.keepGoing {
+				t.Errorf("for %q, keep going:\nwant: %d\n got: %d\n",
+					strings.Join(tc.args, " "),
+					tc.keepGoing, c.keepGoing)
+			}
+			if !reflect.DeepEqual(c.arguments, tc.remaining) {
+				t.Errorf("for %q, remaining arguments:\nwant: %q\n got: %q\n",
+					strings.Join(tc.args, " "),
+					tc.remaining, c.arguments)
+			}
+		})
+	}
+}