Repository: magefile/mage
Pull request #84 (2017-10-24 20:23:08)

Fixed backward compatibility
This PR removes the use of mg.Context from the generated mainfile,
instead generating the code to create the context in the mainfile
itself. This prevents backwards compatibility problems with older
versions of mage that may be vendored, which don't have mg.Context.

Also adds a test to ensure that we don't have the generated mainfile
depend on anything outside the stdlib.


Patch:
diff --git a/mage/main.go b/mage/main.go
index 9525b4b..b082ec0 100644
--- a/mage/main.go
+++ b/mage/main.go
@@ -40,8 +40,11 @@ var initOutput = template.Must(template.New("").Parse(mageTpl))
 const mainfile = "mage_output_file.go"
 const initFile = "magefile.go"
 
+// set by ldflags when you "mage build"
 var (
-	timestamp, commitHash, gitTag string // set by ldflags when you "mage build"
+	commitHash string
+	timestamp  string
+	gitTag     = "v2"
 )
 
 // Main is the entrypoint for running mage.  It exists external to mage's main
diff --git a/mage/template.go b/mage/template.go
index e382733..cf88873 100644
--- a/mage/template.go
+++ b/mage/template.go
@@ -13,8 +13,7 @@ import (
 	"os"
 	"strings"
 	"text/tabwriter"
-
-	"github.com/magefile/mage/mg"
+	"time"
 )
 
 func main() {
@@ -131,7 +130,7 @@ func handleError(logger *log.Logger, err interface{}) {
 
 func runTarget(fn func(context.Context) error) interface{} {
 	var err interface{}
-	ctx, cancel := mg.Context()
+	ctx, cancel := getContext()
 	d := make(chan interface{})
 	go func() {
 		defer func() {
@@ -152,4 +151,30 @@ func runTarget(fn func(context.Context) error) interface{} {
 		return err
 	}
 }
+
+var ctx context.Context
+var ctxCancel func()
+
+func getContext() (context.Context, func()) {
+	if ctx != nil {
+		return ctx, ctxCancel
+	}
+
+	if os.Getenv("MAGEFILE_TIMEOUT") != "" {
+		timeout, err := time.ParseDuration(os.Getenv("MAGEFILE_TIMEOUT"))
+		if err != nil {
+			fmt.Printf("timeout error: %v\n", err)
+			os.Exit(1)
+		}
+
+		ctx, ctxCancel = context.WithTimeout(context.Background(), timeout)
+	} else {
+		ctx = context.Background()
+		ctxCancel = func() {}
+	}
+	return ctx, ctxCancel
+}
+
+
+
 `
diff --git a/mg/deps.go b/mg/deps.go
index 64f069d..3492192 100644
--- a/mg/deps.go
+++ b/mg/deps.go
@@ -36,7 +36,7 @@ var onces = &onceMap{
 // shouldn't be run at the same time.
 func SerialDeps(fns ...interface{}) {
 	checkFns(fns)
-	ctx, _ := Context()
+	ctx := context.Background()
 	for _, f := range fns {
 		runDeps(ctx, f)
 	}
@@ -115,8 +115,7 @@ func checkFns(fns []interface{}) {
 
 // Deps runs the given functions with the default runtime context
 func Deps(fns ...interface{}) {
-	ctx, _ := Context()
-	CtxDeps(ctx, fns...)
+	CtxDeps(context.Background(), fns...)
 }
 
 func changeExit(old, new int) int {
diff --git a/mg/runtime.go b/mg/runtime.go
index f0acc14..8b99613 100644
--- a/mg/runtime.go
+++ b/mg/runtime.go
@@ -1,12 +1,9 @@
 package mg
 
 import (
-	"context"
-	"fmt"
 	"os"
 	"path/filepath"
 	"runtime"
-	"time"
 )
 
 // CacheEnv is the environment variable that users may set to change the
@@ -37,26 +34,3 @@ func CacheDir() string {
 		return filepath.Join(os.Getenv("HOME"), ".magefile")
 	}
 }
-
-var ctx context.Context
-var ctxCancel func()
-
-func Context() (context.Context, func()) {
-	if ctx != nil {
-		return ctx, ctxCancel
-	}
-
-	if os.Getenv("MAGEFILE_TIMEOUT") != "" {
-		timeout, err := time.ParseDuration(os.Getenv("MAGEFILE_TIMEOUT"))
-		if err != nil {
-			fmt.Printf("timeout error: %v\n", err)
-			os.Exit(1)
-		}
-
-		ctx, ctxCancel = context.WithTimeout(context.Background(), timeout)
-	} else {
-		ctx = context.Background()
-		ctxCancel = func() {}
-	}
-	return ctx, ctxCancel
-}
diff --git a/site/content/magefiles/_index.en.md b/site/content/magefiles/_index.en.md
index 1aa8f03..71a82af 100644
--- a/site/content/magefiles/_index.en.md
+++ b/site/content/magefiles/_index.en.md
@@ -61,7 +61,7 @@ var Default = Install
 
 // Targets may have a context argument, in which case a default context is passed
 // to the target, which will be cancelled after a timeout if the -t flag is used.
-func Build(ctx context.Context) error {
+func Build(ctx context.Context) {
     mg.CtxDeps(ctx, Target)
 }
 
diff --git a/site/content/targets/_index.en.md b/site/content/targets/_index.en.md
index 80c339c..859fa61 100644
--- a/site/content/targets/_index.en.md
+++ b/site/content/targets/_index.en.md
@@ -38,5 +38,13 @@ targets.  If any target panics or returns an error, no later targets will be run
 ## Contexts and Cancellation
 
 A default context is passed into any target with a context argument.  This
-context will have a timeout if mage was run with -t, and thus will cancel
-the running targets and dependencies at that time.
\ No newline at end of file
+context will have a timeout if mage was run with -t, and thus will cancel the
+running targets and dependencies at that time.  To pass this context to
+dependencies, use mg.CtxDeps(ctx, ...) to pass the context from the target to
+its dependencies (and pass the context to sub-dependencies).  Dependencies run
+with mg.Deps will not get the starting context, and thus will not be cancelled
+when the timeout set with -t expires.
+
+mg.CtxDeps will pass along whatever context you give it, so if you want to
+modify the original context, or pass in your own, that will work like you expect
+it to.
\ No newline at end of file


Test patch:
diff --git a/mage/main_test.go b/mage/main_test.go
index 4487334..a6e4ac2 100644
--- a/mage/main_test.go
+++ b/mage/main_test.go
@@ -4,6 +4,8 @@ import (
 	"bytes"
 	"flag"
 	"fmt"
+	"go/parser"
+	"go/token"
 	"io/ioutil"
 	"log"
 	"os"
@@ -13,6 +15,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/magefile/mage/build"
 	"github.com/magefile/mage/mg"
 )
 
@@ -264,12 +267,51 @@ func TestKeepFlag(t *testing.T) {
 	buildFile := fmt.Sprintf("./testdata/keep_flag/%s", mainfile)
 	os.Remove(buildFile)
 	defer os.Remove(buildFile)
+	w := tLogWriter{t}
+
 	inv := Invocation{
 		Dir:    "./testdata/keep_flag",
-		Stdout: ioutil.Discard,
-		Stderr: ioutil.Discard,
-		Args:   []string{"noop"},
+		Stdout: w,
+		Stderr: w,
+		List:   true,
 		Keep:   true,
+		Force:  true, // need force so we always regenerate
+	}
+	code := Invoke(inv)
+	if code != 0 {
+		t.Fatalf("expected code 0, but got %v", code)
+	}
+
+	if _, err := os.Stat(buildFile); err != nil {
+		t.Fatalf("expected file %q to exist but got err, %v", buildFile, err)
+	}
+}
+
+type tLogWriter struct {
+	*testing.T
+}
+
+func (t tLogWriter) Write(b []byte) (n int, err error) {
+	t.Log(string(b))
+	return len(b), nil
+}
+
+// Test if generated mainfile references anything other than the stdlib
+func TestOnlyStdLib(t *testing.T) {
+	buildFile := fmt.Sprintf("./testdata/onlyStdLib/%s", mainfile)
+	os.Remove(buildFile)
+	defer os.Remove(buildFile)
+
+	w := tLogWriter{t}
+
+	inv := Invocation{
+		Dir:     "./testdata/onlyStdLib",
+		Stdout:  w,
+		Stderr:  w,
+		List:    true,
+		Keep:    true,
+		Force:   true, // need force so we always regenerate
+		Verbose: true,
 	}
 	code := Invoke(inv)
 	if code != 0 {
@@ -279,6 +321,27 @@ func TestKeepFlag(t *testing.T) {
 	if _, err := os.Stat(buildFile); err != nil {
 		t.Fatalf("expected file %q to exist but got err, %v", buildFile, err)
 	}
+
+	fset := &token.FileSet{}
+	// Parse src but stop after processing the imports.
+	f, err := parser.ParseFile(fset, buildFile, nil, parser.ImportsOnly)
+	if err != nil {
+		fmt.Println(err)
+		return
+	}
+
+	// Print the imports from the file's AST.
+	for _, s := range f.Imports {
+		// the path value comes in as a quoted string, i.e. literally \"context\"
+		path := strings.Trim(s.Path.Value, "\"")
+		pkg, err := build.Default.Import(path, "./testdata/keep_flag", build.FindOnly)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if !filepath.HasPrefix(pkg.Dir, build.Default.GOROOT) {
+			t.Errorf("import of non-stdlib package: %s", s.Path.Value)
+		}
+	}
 }
 
 func TestMultipleTargets(t *testing.T) {
diff --git a/mage/testdata/onlyStdLib/command.go b/mage/testdata/onlyStdLib/command.go
new file mode 100644
index 0000000..3822a80
--- /dev/null
+++ b/mage/testdata/onlyStdLib/command.go
@@ -0,0 +1,29 @@
+// +build mage
+
+package main
+
+import (
+	"fmt"
+	"log"
+
+	"github.com/magefile/mage/mg"
+)
+
+var Default = SomePig
+
+// this should not be a target because it returns a string
+func ReturnsString() string {
+	fmt.Println("more stuff")
+	return ""
+}
+
+func TestVerbose() {
+	log.Println("hi!")
+}
+
+// This is the synopsis for SomePig.  There's more data that won't show up.
+func SomePig() {
+	mg.Deps(f)
+}
+
+func f() {}