Feature/context for jobs #32

interlock · 2017-10-03T15:08:18Z

Allows for jobs that take and call with a context as described in #25

Todo:

Context caller for Deps as a separate func helper
Test a simple Timeout context
split dup code for acceptable func types out
wrap calling job func's to deal with error/context uniformly

closes #25

This commit adds helper functions that should be commonly needed by many magefiles. It includes command execution handlers as well as error handlers. This commit also removes the only two non-stdlib dependencies in the codebase. Since mage will likely be vendored into a repo, it would be nice to avoid cluttering the vendor directory with dependencies that are only for the build script.

Moved magefile template to its own file

natefinch · 2017-10-04T15:21:28Z

gah, sorry for the conflicts.

interlock · 2017-10-04T15:32:27Z

Ya, I saw it coming from that other PR. Still working out handling timeouts locally.

…ure/context

natefinch

Ran out of time to do a proper review, will do more tomorow

natefinch · 2017-10-10T02:49:03Z

example/magefile.go

+
+func Timeout(ctx context.Context) {
+	fmt.Printf("timeout start\n")
+	time.Sleep(1000000000000)


time.sleep takes a time.Duration, you could make this easier to read by just doing time.Sleep(time.Hour) or something :)

It does and "oops!" did not intend to commit that... I'll remove it. I use that for my manual test cases.

natefinch · 2017-10-10T02:50:01Z

mage/main.go

@@ -35,8 +35,8 @@ const initFile = "magefile.go"

 var (
 	force, verbose, list, help, mageInit, keep, showVersion bool
-
-	timestamp, commitHash, gitTag string
+	timeout                                                 int


you can put spaces between the lines os the types aren't so crazily right justified :)

Removed template defered recover in preference to the runTarget func that does the same. Fixed many, but not all, gofmt errors in the template

interlock · 2017-10-14T15:14:41Z

This goes as far as I can take it right now.

Parse updated to spit out some scaffold around running targets wrapped with context
template has a new function, runTarget that handles the defer recover and actual ctx channel handling for errors. Brings the extraction of a StatusCode error to one spot as well.
There is a single test for expiring a context on timeout. We could have more complicated tests, but I figure we can make those when someone does something complicated.
I lightly updated the site docs to indicate there is now a -t <seconds> flag to set a timeout
mg/runtime has a Context function which returns the same Context everytime based on if we have a -t or not.

natefinch

Looks great, a few minor nitpicks

natefinch · 2017-10-10T13:10:31Z

example/magefile.go

+// Manage your deps, or running package managers.
+func InstallDeps(ctx context.Context) error {
+	fmt.Println("Installing Deps...")
+	cmd := exec.Command("go", "get", "github.com/stretchr/piglatin")


there's a version of exec.Command that takes a context, that would be a perfect example for using this new feature: https://golang.org/pkg/os/exec/#CommandContext

natefinch · 2017-10-10T13:13:00Z

mage/main.go

@@ -46,6 +46,7 @@ func init() {
 	flag.BoolVar(&help, "h", false, "show this help")
 	flag.BoolVar(&mageInit, "init", false, "create a starting template if no mage files exist")
 	flag.BoolVar(&keep, "keep", false, "keep intermediate mage files around after running")
+	flag.IntVar(&timeout, "t", 0, "timeout in seconds, Default (0) no timeout")


if you make this a flag.DurationVar, it'll convert a string right to a duration, and then you can do -t 1m5s for example

natefinch · 2017-10-18T01:12:30Z

.vscode/settings.json

@@ -0,0 +1,3 @@
+// Place your settings in this file to overwrite default and user settings.
+{


this should probably be .gitignored

natefinch · 2017-10-18T01:13:50Z

mage/main.go

@@ -40,6 +40,8 @@ const mainfile = "mage_output_file.go"
 const initFile = "magefile.go"

 var (
+	force, verbose, list, help, mageInit, keep, showVersion bool


this can get removed.... with switch to a standalone parse function, we don't have global flag variables anymore.

natefinch · 2017-10-18T01:33:43Z

mage/main_test.go

@@ -31,7 +31,11 @@ func testmain(m *testing.M) int {
 		log.Fatal(err)
 	}
 	if err := os.Mkdir(dir, 0700); err != nil {
-		log.Fatal(err)
+		if os.IsExist(err) {
+			os.RemoveAll(fmt.Sprintf("%s/*", dir))


RemoveAll removes a directory. globbing (*) doesn't generally work in go functions anyway. if you want to just delete everything inside the directory, you'll either have to loop through, or better, just delete the whole thing and recreate it.

natefinch · 2017-10-18T01:55:05Z

mg/deps.go

 		fn = f
+	default:
+		panic("attempted to add a dep that did not match required type")


can you slap a
// should be impossible, since we already checked this
above this?

natefinch · 2017-10-18T01:56:35Z

mg/runtime.go

+var ctx context.Context
+var ctxCancel func()
+
+func GetContext() (context.Context, func()) {


Can you call this just Context()? GetFoo is sorta non-idiomatic.

natefinch · 2017-10-18T01:57:28Z

mg/runtime.go

+	if os.Getenv("MAGEFILE_TIMEOUT") != "" {
+		timeout, err := strconv.Atoi(os.Getenv("MAGEFILE_TIMEOUT"))
+		if err != nil {
+			fmt.Println("timeout must be a number >= 0")


speaking of which, check for negative numbers here

natefinch · 2017-10-18T01:58:25Z

parse/parse.go

+	Comment   string
+}
+
+func (f Function) TemplateString() string {


doc comment here please

natefinch · 2017-10-18T02:02:15Z

parse/parse_test.go

+	}
+
+	// DefaultName
+	if strings.Compare(info.DefaultName, "ReturnsError") != 0 {


if info.DefaultName != "ReturnsError" {

natefinch

two tiny tweaks and we're home free!

natefinch · 2017-10-18T03:41:29Z

mage/main.go

@@ -368,7 +367,7 @@ func RunCompiled(inv Invocation, exePath string) int {
 		c.Env = append(c.Env, "MAGEFILE_HELP=1")
 	}
 	if inv.Timeout > 0 {
-		c.Env = append(c.Env, fmt.Sprintf("MAGEFILE_TIMEOUT=%d", inv.Timeout))
+		c.Env = append(c.Env, fmt.Sprintf("MAGEFILE_TIMEOUT=%dns", inv.Timeout))


you can probably just do inv.Timeout.String()

natefinch · 2017-10-18T03:42:18Z

mage/main_test.go

@@ -32,7 +33,7 @@ func testmain(m *testing.M) int {
 	}
 	if err := os.Mkdir(dir, 0700); err != nil {
 		if os.IsExist(err) {
-			os.RemoveAll(fmt.Sprintf("%s/*", dir))
+			os.RemoveAll(fmt.Sprintf("%s", dir))


can remove the sprintf now :)

…removed a pointless fmt

natefinch and others added 9 commits October 1, 2017 22:24

fix a bug a add a test for it

dfbdf98

Merge branch 'master' into sh

36d9acf

fixed test on parse

d11e222

prelimit blank Context

65752c9

Merge remote-tracking branch 'origin/master' into feature/context

0370733

types package created and parse package updated to use it.

440233e

Moved magefile template to its own file

Updated template to use TemplateString func from parse.Function struct

e168a4b

fixed Default in template

038005c

interlock added 12 commits October 4, 2017 08:49

Merge branch 'master' into feature/context

0471aa6

Context-ified deps, added some context jobs for the mage test

e236cc4

un-lazy the parse func test

f11723c

gofmt and undeep the test for parse

df87051

back to inline types

1e3fd1b

added -t for timeout and started the template code to handle that

3c0b0b8

one layer of context timeout working

89e13bf

test on timeout

a1cdd7e

filled in context/chan running for all Func types

9dea55b

timeout flag added to readme doc

9acc410

timeout flag added to readme doc

3ce1b8c

Merge branch 'feature/context' of github.com:interlock/mage into feat…

33a3a1f

…ure/context

natefinch reviewed Oct 10, 2017

View reviewed changes

interlock added 2 commits October 10, 2017 08:13

updated spacing for main vars, and removed example

71f1848

missed a merge

8ca6529

interlock force-pushed the feature/context branch from bde64d9 to 8ca6529 Compare October 10, 2017 15:29

interlock added 3 commits October 10, 2017 09:01

reorg main test after merge

1c2f55e

worked towards tests working

6cdb0a6

Tests updated for "Error: " outputs

0f0faf7

Removed template defered recover in preference to the runTarget func that does the same. Fixed many, but not all, gofmt errors in the template

interlock added 2 commits October 14, 2017 08:21

Merge branch 'master' into feature/context

4159552

broke Mage list test into an array of contains tests

7459834

interlock changed the title ~~[WIP] Feature/context for jobs~~ Feature/context for jobs Oct 14, 2017

test on panics from Dep targets

a3b30f0

natefinch requested changes Oct 18, 2017

View reviewed changes

interlock added 2 commits October 17, 2017 19:45

updated to parse a duration string instead of straight int seconds

1ef64a7

revisions based on CR

39efa8c

natefinch requested changes Oct 18, 2017

View reviewed changes

interlock and others added 2 commits October 17, 2017 20:50

Turns out Duration does the right thing when converted to string and …

0b698d3

…removed a pointless fmt

Merge branch 'master' into feature/context

08fee70

natefinch approved these changes Oct 18, 2017

View reviewed changes

natefinch merged commit df36722 into magefile:master Oct 18, 2017

interlock deleted the feature/context branch October 18, 2017 03:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/context for jobs #32

Feature/context for jobs #32

interlock commented Oct 3, 2017 •

edited

Loading

natefinch commented Oct 4, 2017

interlock commented Oct 4, 2017

natefinch left a comment

natefinch Oct 10, 2017

interlock Oct 10, 2017 •

edited

Loading

natefinch Oct 10, 2017

interlock commented Oct 14, 2017

natefinch left a comment

natefinch Oct 10, 2017

natefinch Oct 10, 2017

natefinch Oct 18, 2017

natefinch Oct 18, 2017

natefinch Oct 18, 2017

natefinch Oct 18, 2017

natefinch Oct 18, 2017

natefinch Oct 18, 2017

natefinch Oct 18, 2017

natefinch Oct 18, 2017

natefinch left a comment

natefinch Oct 18, 2017

natefinch Oct 18, 2017

		@@ -0,0 +1,3 @@
		// Place your settings in this file to overwrite default and user settings.
		{

Feature/context for jobs #32

Feature/context for jobs #32

Conversation

interlock commented Oct 3, 2017 • edited Loading

natefinch commented Oct 4, 2017

interlock commented Oct 4, 2017

natefinch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

interlock Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

interlock commented Oct 14, 2017

natefinch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natefinch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

interlock commented Oct 3, 2017 •

edited

Loading

interlock Oct 10, 2017 •

edited

Loading