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

Allowed customize SysProcAttr #53

Closed
wenerme opened this issue Oct 31, 2020 · 9 comments
Closed

Allowed customize SysProcAttr #53

wenerme opened this issue Oct 31, 2020 · 9 comments

Comments

@wenerme
Copy link
Contributor

wenerme commented Oct 31, 2020

e.g. windows need to set HideWindow

func hideWindow(cmd *exec.Cmd) {
	cmd.SysProcAttr.HideWindow = true
}

maybe cmd can accept option like this

type CmdOptions func(c *exec.Cmd)
@cgx027
Copy link

cgx027 commented Jun 9, 2021

It's a good idea to me. I do want to setup the cmd.SysProcAttr.Credential so that I can run commands under another user.

cmd.SysProcAttr.Credential = &syscall.Credential{Uid: uid, Gid: gid}

Refer to https://stackoverflow.com/a/21706757/14833782

@wenerme
Copy link
Contributor Author

wenerme commented Jun 9, 2021

currently using this fork #54

@daniel-nichter
Copy link
Member

Re #54: do you just wanted to set SysProcAttr, or do you want a more general-purpose callback like feature (like in that PR)?

@wenerme
Copy link
Contributor Author

wenerme commented Jun 9, 2021

I prefer the general-purpose callback, like use this if you know what you are doing.

@cgx027
Copy link

cgx027 commented Jun 10, 2021

Thanks @daniel-nichter for looking into this. For my user case, both will work fine. I would vote for the general-purpose callback although(as in #54 ) as it will give people a lot more flexibility on using this cool tool.

daniel-nichter added a commit that referenced this issue Oct 29, 2021
@daniel-nichter
Copy link
Member

@wenerme @Callisto13 et al. I merged PR #54 but reworked in PR #75. Here's why...

Cmd.Options is too close to the Options struct. And this latter is where I'm trying to move options so that Cmd doesn't become overloaded with optional fields. So Options.SetCmd is the better design, but it means you need to specify the callbacks like the test:

	p := cmd.NewCmdOptions(
		cmd.Options{
			SetCmd: []func(cmd *exec.Cmd){
				func(cmd *exec.Cmd) { handled = true },
			},
		},
		"/bin/ls",
	)

If that doesn't work, then I could add Cmd.SetCmd, too, which combines with any Options.SetCmd. Reason being: looking ahead, I'm thinking that v2 of this pkg move all things to Options, so Cmd.SetCmd would be a deprecated featured for the start.

@wenerme
Copy link
Contributor Author

wenerme commented Oct 29, 2021

lgtm, but SetCmd feels strange, more like BeforeExec or something, like hook

@daniel-nichter
Copy link
Member

Yeah, the same occurred to me and I wrote it as BeforeExec at one point. Although its original intent was to set/modify the underlying os/exec.Exec, its actual function is a generic before-exec callback/hook. Will think on it, and see if others have ideas, before the v1.4 release (where this will appear officially).

@daniel-nichter
Copy link
Member

Renamed to BeforeExec and released as v1.4.0.

This issue was closed.
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

No branches or pull requests

3 participants