go-godo / godo

golang build tool in the spirt of rake, gulp
MIT License
536 stars 31 forks source link

feature to kill the previously started process #24

Open sathishvj opened 9 years ago

sathishvj commented 9 years ago

I'd like to suggest/request a feature/function to kill the previously 'Start'ed process. I know 'Start' will do that, but what if a build we did in a previous process failed and we don't do a Start?

My scenario:

  1. Detect file change
  2. Do go build Oops! Build failed Skip all remaining steps, including 'Start'

Now, the process 'Start'ed on the earlier set is still running and my app is still accessible which is not the expected result.

mgutz commented 9 years ago

Definitely a problem. I'm surprised I haven't run into that. I'll think of something and should have a fix in the soon.

sathishvj commented 9 years ago

One other issue I noticed for my team mate is that windows doesn't allow you to overwrite an exe file that is already running. So I'll have to first kill the running process and only then do a build. So yeah, this feature looks to be a necessity.

Let me see if I can make the killSpawned function public and see if it works. Will send a pull request if.

mgutz commented 9 years ago

This might take a couple of days. The solution is to add Start method to the context so the task can track the process and then add a Task.Kill method. Unfortunately, there's a little more to it. There are goroutines involved when watching so I have to do some refactoring to add quit channels. I'll work on it tonight and should have something by tomorrow.

mgutz commented 9 years ago

I need to understand your process a bit more. In most cases, if a dependency fails the parent shouldn't be restarted. For example

p.Task("server", D{"assets", "views"} ...)

If "assets" fails then there should be an error logged in red if the task returned an error. eg

p.Task("assets", func() error {
    return Run("browserify ...")
})

We found it annoying that we had to restart godo often when all we had to do was fix syntax in Javascript file. Once we fixed the error in our editor, it triggers a restart of the server. I admit though every now and then someone forgets to check their terminal for errors.

Also, you don't normally need to build any Go files before Start. Godo builds the main package for you including its dependencies if the argument to Start is a go file.

I can add a flag to always exit on errors if that would help. I'm not sure if it should be the default behavior.

sathishvj commented 9 years ago

I'm checking some of these things you asked and I'm seeing weird results. One, I actually am not able to run it via go run. I see this other stackoverflow qn about it too: http://stackoverflow.com/questions/26920969/why-does-the-go-run-command-fail-to-find-a-second-file-in-the-main-package

I also am not able to do a go run *.go because then it complains that it cannot run _test.go files. So I guess only the build and run executable option is currently available for me.

sathishvj commented 9 years ago

To your other question, here is what happens/happened to me ...

  1. I write code (v1.0), godo detects file change, it builds correctly, it runs, I use it via the browser.
  2. I change code (v1.1). godo detects file change. Code error - it doesn't build. But the old version is still running as it hasn't been killed/re'Start'ed. I still access the app via the browser and assume it's the latest when it's not.

Here is what would be preferable in my opinion ...

  1. I change code (v1.1). godo detects file change. It kills the previously spawned process. Code has error - it doesn't build. Now I can't access the app via the browser.
mgutz commented 9 years ago

Interesting, I created an example in godo. If I run go run main.go in cmd/example I get the error as described on SO. However, if I use godo server -w the server runs fine and restarts. Try updating godo

go get -u gopkg.in/godo.v1/cmd/godo

Then CD into the godo.v1 directory and run godo server -w which runs and watches cmd/example.

When you say change code, are you doing a git checkout to another branch whlie godo is watching? I will add a flag that will stop on any error.

sathishvj commented 9 years ago

Changing code as in fixing bugs and adding features locally. I miss a parentheses here or there, build breaks, but older executable is still running.

I was thinking that if you made your killSpawned -> KillSpawned, then we'd have a way to control it ourselves.

Also, as I mentioned, on my team mate's Windows ... assume the process is running from a previous build. So I've previously started executable.exe. When I do a go build again, windows gives me an error because it cannot overwrite the exe file of a running process. So godo always fails. Right now I've attempted to solve it with "taskkill /f /im executable.exe"

mgutz commented 9 years ago

Please post a gitst of your Godofile, the relevant parts that are causing this.

I made a fix on Windows. Not sure if it helps your teammate. I hardly use Windows but I did try the example and there was an issue with it. The example works while watching now. Please update godo and let me know.

sathishvj commented 9 years ago

I'm a little embarrassed to show you my current Godofile as it is not really following the dependencies mechanism. Instead I'm controlling each line of execution at the moment.

I liked Godo and I wanted to tell others in my team and also keep a note of it for later, so I immediately (maybe a little too early) wrote this medium post: https://medium.com/@sathishvj/watch-generate-build-execute-golang-projects-on-file-changes-50255a98ab7c

mgutz commented 9 years ago

The golang tools are finicky, to say the least. Using go run will not work as expected. It spawns a child process which cannot be killed programmatically. http://stackoverflow.com/questions/24982845/process-kill-on-child-processes. I use go install -a internally. If your app doesn't build and run cleanly with a simple go install then godo will not work.

sathishvj commented 9 years ago

Hmm, and there isn't a way to kill the process tree with go?

Anyways, I'm not running it with go run. I'm doing a ./executable. So that issue doesn't apply in this case.

I think, at the least, you could make the killSpawned function public, so that if somebody wants to call that specifically, we could. If not, no loss anyways.

mgutz commented 9 years ago

I can do that. I'll do it first thing in the morning. It's getting late. BTW, you can see which files are changing with the verbose flag godo server --verbose.

sathishvj commented 9 years ago

Oh ok. Thanks. I tried -v which is the idiomatic way I think of getting verbose output. But it gave me version.

mgutz commented 9 years ago

In the process of refactoring for godo version 2 (v2) and writing tests, I ran into the Debounce/Watch issues you were having. Watch file events were being swallowed in between Debounce intervals. For example, say index.go.html changed but the task handler was debounced. In this scenario the handler would never run from that change. v2 enqueues the file event to run after the Debounce interval.

The watch algorithm in v1 is crude and worked for my uses cases. The v2 is more intelligent and efficient. I hope to do a v2 pre-release in about a week.

sathishvj commented 9 years ago

awesome man! thank you for your work.