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

Feature Request: automatic insertion of a job-scoped context #25

Closed
Xe opened this issue Oct 1, 2017 · 3 comments · Fixed by #32
Closed

Feature Request: automatic insertion of a job-scoped context #25

Xe opened this issue Oct 1, 2017 · 3 comments · Fixed by #32
Assignees

Comments

@Xe
Copy link

Xe commented Oct 1, 2017

This would allow for users to write functions like:

func Build(ctx context.Context) error {
  cmd := exec.CommandContext(ctx, "/usr/local/go/bin/go", "build", "./cmd/...")
  cmd.Stdout = os.Stdout
  cmd.Stderr = os.Stderr
  err := cmd.Run()
}

And then if the user hits control-c to terminate the mage process, the context will instantly cancel and cause the process started by the Build function to be killed.

Mage itself would need to start and manage these contexts.

It would be nice to also have a command line flag -t that would set a timeout on these contexts.

@natefinch
Copy link
Member

great idea! I had been contemplating allowing context, but hadn't thought about CLI specified timeouts. very cool.

@interlock
Copy link
Contributor

We should figure out how to handle some common cases for this?

With a time for overall job complete, if a exec.ComandContext exceeds this time how do we signal to that job? What should we signal?

We may out grow a method signature that takes context.Context? Should we great a Mage struct that contains this and other future features? I am thinking about #24 right now.

-t applies to whole job or each job called? If we have many deps/build steps/network fetches it may make sense to limit total run time and/or job run time? Perhaps this is already solved with Context.WithDeadline and Context.WithTimeout?

@natefinch Thoughts on using context.WithValue ? Community seems split on it's use now, either: use only for values needed to cancel/deadline/manage context; or, use it for any sort value to this session/call/job/etc.

@natefinch
Copy link
Member

I'd like to keep parameters as minimal as possible so that magefiles are easy to read and write. An auto-populated context is fine, especially if it's opt-in. A ton of other values gets too heavyweight for the writer and reader.

I really dislike context.WithValue. That being said, the context should be used so people can put whatever they want in the context, because at the end of the day, it's just go code.

For example, this should be totally legal code:

func Build(ctx *context.Context) {
     ctx2 := ctx.WithValue(foo, bar)
     mg.Deps(ctx2, f, g)
}

Now as to whether we add official support for supplying values from the CLI or something, I don't know. I don't think I would do it preemptively, for sure.

As for timeouts... I think to start, a simple timeout for the whole thing to run is sufficient. If people start needing more complicated timeouts, we can think about it, but since I don't think make or rake support that kind of timeout, having anything will still put us ahead.

@interlock interlock mentioned this issue Oct 3, 2017
4 tasks
natefinch pushed a commit that referenced this issue Oct 18, 2017
Allows for jobs that take and call with a context.  Closes #25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants