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

proposal: os/exec: allow setting Context in Cmd #46699

Closed
MagicalTux opened this issue Jun 11, 2021 · 7 comments
Closed

proposal: os/exec: allow setting Context in Cmd #46699

MagicalTux opened this issue Jun 11, 2021 · 7 comments
Labels
Projects
Milestone

Comments

@MagicalTux
Copy link

@MagicalTux MagicalTux commented Jun 11, 2021

What version of Go are you using (go version)?

$ go version
go version go1.16 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/magicaltux/.cache/go-build"
GOENV="/home/magicaltux/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/magicaltux/go/pkg/mod"
GONOPROXY="git.atonline.com"
GONOSUMDB="git.atonline.com"
GOOS="linux"
GOPATH="/home/magicaltux/go"
GOPRIVATE="git.atonline.com"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16"
GCCGO="gccgo"
AR="ar"
CC="x86_64-pc-linux-gnu-gcc"
CXX="x86_64-pc-linux-gnu-g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2186180239=/tmp/go-build -gno-record-gcc-switches"

What did you do?

os/exec allows spawning and settings most options of exec.Cmd without using exec.Command(), however the ctx value can only be set when invoking using exec.CommandContext().

Because I instantiate it with a number of custom settings I typically not use exec.Command() but still need to use the context.

Hence, I am requesting if ctx can be instead renamed Context so it can be set cleanly. This change can be made so it doesn't affect any existing code, and is much cleaner.

What did you expect to see?

An easy way to set Cmd.ctx:

        p := &exec.Cmd{
                Context: ctx,
                Path: argv[0],
                Args: argv,
                Dir: usr.HomeDir,
                SysProcAttr: &syscall.SysProcAttr{
                        Credential: creds,
                },
        }

What did you see instead?

This is ugly but needed because exec.Command will try to use LookPath and set lookPathErr if it fails. In this specific case I do not use lookpath because argv[0] is special and requires SysProcAttr to appear valid, so instead I give it another binary that I know will exist. LookPath will return an error even when given a full path if it doesn't appear executable.

        // create p with a fake value but the context
        p := exec.CommandContext(ctx, "/bin/true")

        // setup
        p.Path = argv[0]
        p.Args = argv
        p.Dir = usr.HomeDir
        p.SysProcAttr = &syscall.SysProcAttr{
                Credential: creds,
        }
        ...
@cherrymui cherrymui changed the title os/exec: allow setting Context in Cmd proposal: os/exec: allow setting Context in Cmd Jun 11, 2021
@gopherbot gopherbot added this to the Proposal milestone Jun 11, 2021
@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Jun 11, 2021

Mark as proposal since this is requesting an API change.

cc @bradfitz @ianlancetaylor

Loading

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jun 11, 2021
@sfllaw
Copy link
Contributor

@sfllaw sfllaw commented Sep 29, 2021

What would it mean if exec.Cmd.Context was mutated after initialization?

Perhaps we should be inspired by net/http.Request.Context and WithContext?

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Nov 3, 2021

We still need to do #43724, which included replacing the unexported lookPathErr with an exported Err. If we do that, then this use case can be satisfied by just calling CommandContext and then editing Err away. It sounds like maybe we should do that?

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Nov 3, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

Loading

@rsc rsc moved this from Incoming to Active in Proposals Nov 3, 2021
@bcmills
Copy link
Member

@bcmills bcmills commented Nov 4, 2021

I think it's fine if this falls out naturally from #43724, but I would prefer we not go out of our way to allow setting Context because it usually sends an inappropriate signal anyway (see #31774 / #21135).

(I would rather see a more holistic redesign of the Context interaction with os/exec.)

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Nov 10, 2021

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

Loading

@rsc rsc moved this from Active to Likely Decline in Proposals Nov 10, 2021
@rsc rsc moved this from Likely Decline to Declined in Proposals Dec 1, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Dec 1, 2021

No change in consensus, so declined.
— rsc for the proposal review group

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants