Skip to content

Conversation

hfrappier
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 98.201% when pulling 0d6d1f1 on hfrappier:master into 36217f0 on go-cmd:master.

@daniel-nichter
Copy link
Member

Very nice, thank you. This kind of minimal change gives me more confidence that existing functionality won't break.

I'm not familiar with Windows, but #22 handles process termination like this:

	p, err := os.FindProcess(-c.status.PID)
	if err != nil {
		return err
	}
 	return p.Signal(syscall.SIGTERM)

That seems more native Go than kill := exec.Command("TASKKILL", "/T", "/F", "/PID", fmt.Sprintf("%d", pid)). Does the #22 approach work instead?

@scbizu
Copy link

scbizu commented Nov 24, 2018

Hi there, thanks to review my PR at #22 , I think it's better to add the PLATFORM CI such as goreleaser and gox to check if cmd will work as expected in other platform.

@hfrappier
Copy link
Contributor Author

I can test this codepath when I get my hands on a windows machine.

Otherwise, as far as taskkill's doc goes,
Taskkill is the equivalent of SIGTERM
Taskkill /T handles child processes,
Taskkill /F is the equivalent of SIGKILL

@nyanshak
Copy link

What else needs to happen to get this merged? Anything I can do?

@daniel-nichter
Copy link
Member

Sticking point is kill := exec.Command("TASKKILL", "/T", "/F", "/PID", fmt.Sprintf("%d", pid)). I don't think it's a good idea to exec another program like that. I'm not a Windows expert, but if it were Linux my concern would be that TASKKILL isn't in $PATH or even installed. I suspect the same would hold true for Windows, making that approach too fragile.

Instead, a native Go solution like #22 is preferable. I presume that's same as TASKKILL? If so, just replace the Windows implementation in this PR with #22 's implementation.

@nengc
Copy link

nengc commented Oct 17, 2019

"TASKKILL" is the most credible method to kill the process after trying any possible methods.

@hfrappier
Copy link
Contributor Author

gopsutil
uses:
process := os.Process{Pid: int(p.Pid)} return process.Kill()

@daniel-nichter
Copy link
Member

Thanks @hfrappier, I'll merge this and switch the Windows implementation to use os.Process. I think it's best to rely on Go's built-in process management rather than shell out to another process (TASKKILL).

@daniel-nichter daniel-nichter merged commit d5ef410 into go-cmd:master Nov 24, 2019
@scbizu
Copy link

scbizu commented Nov 25, 2019

May I ask that why not use os.FindProcess directly ?

( As Go doc mentioned , Windows will return an error if we have some unexpected case ? )

The err which returned by os.FindProcess will be ignored in current implementation ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants