build: ensure build flags are inserted before arguments (#223)

* Append command flags before arguments

* add test

Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>

* refactor `newGoBuildCommand`

Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>

---------

Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
Co-authored-by: Mohammed Al Sahaf <msaa1990@gmail.com>

builder.go

@@ -97,7 +97,10 @@ func (b Builder) Build(ctx context.Context, outputFile string) error {
 	// generating windows resources for embedding
 	if b.OS == "windows" {
 		// get version string, we need to parse the output to get the exact version instead tag, branch or commit
-		cmd := buildEnv.newGoBuildCommand(ctx, "list", "-m", buildEnv.caddyModulePath)
+		cmd, err := buildEnv.newGoBuildCommand(ctx, "list", "-m", buildEnv.caddyModulePath)
+		if err != nil {
+			return err
+		}
 		var buffer bytes.Buffer
 		cmd.Stdout = &buffer
 		err = buildEnv.runCommand(ctx, cmd)
@@ -147,9 +150,12 @@ func (b Builder) Build(ctx context.Context, outputFile string) error {
 	}
 
 	// compile
-	cmd := buildEnv.newGoBuildCommand(ctx, "build",
+	cmd, err := buildEnv.newGoBuildCommand(ctx, "build",
 		"-o", absOutputFile,
 	)
+	if err != nil {
+		return err
+	}
 	if b.Debug {
 		// support dlv
 		cmd.Args = append(cmd.Args, "-gcflags", "all=-N -l")

environment.go

@@ -246,9 +246,16 @@ func (env environment) newCommand(ctx context.Context, command string, args ...s
 
 // newGoBuildCommand creates a new *exec.Cmd which assumes the first element in `args` is one of: build, clean, get, install, list, run, or test. The
 // created command will also have the value of `XCADDY_GO_BUILD_FLAGS` appended to its arguments, if set.
-func (env environment) newGoBuildCommand(ctx context.Context, args ...string) *exec.Cmd {
+func (env environment) newGoBuildCommand(ctx context.Context, goCommand string, args ...string) (*exec.Cmd, error) {
-	cmd := env.newCommand(ctx, utils.GetGo(), args...)
+	switch goCommand {
-	return parseAndAppendFlags(cmd, env.buildFlags)
+	case "build", "clean", "get", "install", "list", "run", "test":
+	default:
+		return nil, fmt.Errorf("unsupported command of 'go': %s", goCommand)
+	}
+	cmd := env.newCommand(ctx, utils.GetGo(), goCommand)
+	cmd = parseAndAppendFlags(cmd, env.buildFlags)
+	cmd.Args = append(cmd.Args, args...)
+	return cmd, nil
 }
 
 // newGoModCommand creates a new *exec.Cmd which assumes `args` are the args for `go mod` command. The
@@ -336,7 +343,10 @@ func (env environment) execGoGet(ctx context.Context, modulePath, moduleVersion,
 		caddy += "@" + caddyVersion
 	}
 
-	cmd := env.newGoBuildCommand(ctx, "get", "-v")
+	cmd, err := env.newGoBuildCommand(ctx, "get", "-v")
+	if err != nil {
+		return err
+	}
 	// using an empty string as an additional argument to "go get"
 	// breaks the command since it treats the empty string as a
 	// distinct argument, so we're using an if statement to avoid it.

environment_test.go

@@ -0,0 +1,114 @@
+// Copyright 2020 Matthew Holt
+//
+// 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 xcaddy
+
+import (
+	"context"
+	"reflect"
+	"testing"
+
+	"github.com/caddyserver/xcaddy/internal/utils"
+)
+
+func Test_environment_newGoBuildCommand(t *testing.T) {
+	type fields struct {
+		buildFlags string
+	}
+	type args struct {
+		goCommand string
+		args      []string
+	}
+	tests := []struct {
+		name     string
+		fields   fields
+		args     args
+		wantArgs []string
+		wantErr  bool
+	}{
+		{
+			name:    "no flags + no args",
+			fields:  fields{},
+			args:    args{},
+			wantErr: true,
+		},
+
+		{
+			name:     "no flags + single arg",
+			fields:   fields{},
+			args:     args{"build", []string{}},
+			wantArgs: []string{utils.GetGo(), "build"},
+		},
+
+		{
+			name:     "no flags + multi arg",
+			fields:   fields{},
+			args:     args{"build", []string{"main.go"}},
+			wantArgs: []string{utils.GetGo(), "build", "main.go"},
+		},
+
+		{
+			name:     "single flag + no arg",
+			fields:   fields{"-trimpath"},
+			args:     args{"", []string{}},
+			wantArgs: []string{utils.GetGo(), "-trimpath"},
+			wantErr:  true,
+		},
+
+		{
+			name: "multi flag + no arg",
+			fields: fields{
+				"-ldflags '-w -s -extldflags=-static'",
+			},
+			args:    args{},
+			wantErr: true,
+		},
+
+		{
+			name: "multi flag + one arg",
+			fields: fields{
+				"-ldflags '-w -s -extldflags=-static'",
+			},
+			args:     args{"build", []string{}},
+			wantArgs: []string{utils.GetGo(), "build", "-ldflags", "-w -s -extldflags=-static"},
+		},
+
+		{
+			name: "multi flags + multi args",
+			fields: fields{
+				buildFlags: "-ldflags '-w -s -extldflags=-static'",
+			},
+			args:     args{"build", []string{"main.go"}},
+			wantArgs: []string{utils.GetGo(), "build", "-ldflags", "-w -s -extldflags=-static", "main.go"},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			env := environment{
+				buildFlags: tt.fields.buildFlags,
+			}
+			got, err := env.newGoBuildCommand(context.TODO(), tt.args.goCommand, tt.args.args...)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("environment.newGoBuildCommand() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if (err != nil) && tt.wantErr {
+				return // expected error, continue
+			}
+			if !reflect.DeepEqual(got.Args, tt.wantArgs) {
+				t.Errorf("environment.newGoBuildCommand() = %#v, want %#v", got.Args, tt.wantArgs)
+			}
+		})
+	}
+}