os/exec: improved API and default behavior for the PWD env variable #50599
Comments
|
This proposal has been added to the active column of the proposals project |
|
Neato! One more missed |
|
Change https://golang.org/cl/380503 mentions this issue: |
… Dir Updates golang/go#50599. Change-Id: I5702de0d6e23fa2ae1a03b0f47464170d4b691c0 Reviewed-on: https://go-review.googlesource.com/c/dl/+/380503 Trust: Bryan Mills <bcmills@google.com> Run-TryBot: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
|
This seems reasonable. It's unfortunate that sometimes we will have the wrong PWD inherited from constructs like but that's indistinguishable from really wanting to preserve PWD. So be it. Those will just have to update to |
|
Does anyone object to making this change? |
|
Based on the discussion above, this proposal seems like a likely accept. |
|
No change in consensus, so accepted. 🎉 |
rsc
changed the title
|
Change https://go.dev/cl/391804 mentions this issue: |
Since these commands already include an explicit Env field, they will not be fixed automatically by proposal #50599. Change-Id: Ia8157a71cf0cfe208bdc0da9aef54be3d26c795f Reviewed-on: https://go-review.googlesource.com/c/go/+/391804 Trust: Bryan Mills <bcmills@google.com> Run-TryBot: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Daniel Martí <mvdan@mvdan.cc> Trust: Daniel Martí <mvdan@mvdan.cc>
|
Should linters warn if an variable |
Maybe! Really the condition we want to avoid is “ |
|
Change https://go.dev/cl/401339 mentions this issue: |
|
Change https://go.dev/cl/401340 mentions this issue: |
Once #50599 is implemented, the entries will be observable via the Environ method. I find it confusing for later entries in the list to jump arbitrarily far forward based on entries for the same key that no longer exist. This also fixes the deduplication logic for the degenerate Windows keys observed in #49886, which were previously deduplicated as empty keys. (It does not do anything about the even-more-degenerate keys observed in #52436.) For #50599. Change-Id: Ia7cd2200ec34ccc4b9d18631cb513194dc420c25 Reviewed-on: https://go-review.googlesource.com/c/go/+/401339 Reviewed-by: Ian Lance Taylor <iant@google.com> Run-TryBot: Bryan Mills <bcmills@google.com> Auto-Submit: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
|
Change https://go.dev/cl/401535 mentions this issue: |
|
Change https://go.dev/cl/401534 mentions this issue: |
With #50599 implemented, base.AppendPWD is redundant if cmd.Env would otherwise be nil, and calls to os.Environ followed by base.AppendPWD can be replaced by a simpler call to cmd.Environ. Updates #50599. Change-Id: I94a22e2a4cc8e83c815ac41702ea0b1ee5034ecc Reviewed-on: https://go-review.googlesource.com/c/go/+/401534 Run-TryBot: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@google.com>
This is a code simplification using the new API added in #50599. Change-Id: Icb9628bcd0daa3dc2d653e9654b38099730137d1 Reviewed-on: https://go-review.googlesource.com/c/go/+/401535 Run-TryBot: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@google.com>
|
Change https://go.dev/cl/401934 mentions this issue: |
The previous implementation of helperCommand relied on running a well-known Test function which implemented all known commands. That not only added Skip noise in the test's output, but also (and more importantly) meant that the commands could not write directly to stdout in the usual way, since the testing package hijacks os.Stdout for its own use. The new implementation addresses the above issues, and also ensures that all registered commands are actually used, reducing the risk of an unused command sticking around after refactoring. It also sets the subprocess environment variable directly in the test process, instead of on each individual helper command's Env field, allowing helper commands to be used without an explicit Env. Updates #50599. (Also for #50436.) Change-Id: I189c7bed9a07cfe47a084b657b88575b1ee370b9 Reviewed-on: https://go-review.googlesource.com/c/go/+/401934 Run-TryBot: Bryan Mills <bcmills@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com>
|
Change https://go.dev/cl/484295 mentions this issue: |
On macOS, TMPDIR is typically a symlink, and the GOROOT for the buildlet is in TMPDIR as well. PWD must be preserved in order for os.Getwd (and functions based on it) to report paths that remain relative to GOROOT, and paths relative to GOROOT are necessary in order for filepath.Rel to report subdirectories as subdirectories (rather than paths with long "../../…" prefixes). Fortunately, the (*Cmd).Environ method added for #50599 makes preserving PWD somewhat easier. This fixes 'go test cmd/internal/moddeps' on the new darwin-amd64-longtest builder. For #35678. Change-Id: Ibaa458bc9a94b44ba455519bb8da445af07fe0d2 Reviewed-on: https://go-review.googlesource.com/c/go/+/484295 Auto-Submit: Bryan Mills <bcmills@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org> Run-TryBot: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Michael Pratt <mpratt@google.com>
Background
On POSIX platforms, the
PWDenvironment variable represents (emphasis mine) “an absolute pathname of the current working directory.”The specific value of
PWDis important if the current working directory can be reached through multiple absolute paths, as is the case for subdirectories of/tmpon macOS. It affects the behavior ofos.Getwd(see #8400), which can in turn affect both the error and non-error output of programs, including behaviors like paths in debug metadata (which can invalidate build caches if skewed) and parent-directory lookup (important for, among other things, identifying the current Go workspace and module). APWDin/tmparises naturally in any Go test that executes a subprocesses within a directory obtained by calling(*testing.T).TempDir, which is (or ought to be!) common for tests of command-line binaries.Failing to set
PWDappropriately when setting theDirhas led to a number of subtle bugs within the Go project, such as:go:generate)Moreover, setting
PWDcorrectly is not trivial:PWDis required to be absolute, but theDirfield may be relative, leading to bugs like #46832 when the two do not match.Proposal
I propose two ergonomic improvements to the
exec.CmdAPI:If the
Envfield of anexec.Cmdis nil and itsDirfield is non-empty on a platform that usesPWD, the subprocess'sPWDvariable should be implicitly set to an appropriate absolute path (computed fromos.Getwd()ifDiris relative) when the command is started.Dirfield is empty, thePWDvariable (or lack thereof) would continue to be inherited from the Go process, as it is today.Envfield is non-nil, itsPWDvariable (or lack thereof) would continue to be used, as it is today.A new method,
(*Cmd).Environ, analogous toos.Environ(), should return a copy of the environment that would be used by the command in its current configuration: either a copy of the current value of theEnvfield (if non-nil), oros.Environ()augmented with thePWDand/orSYSTEMROOTvariables that would otherwise be used.Part (1) alone would set a more appropriate
PWDfor existing commands that setDirand do not explicitly modify the environment — that is, commands for which the author almost certainly hasn't considered thePWDinteraction at all.Part (2) would make it easier to set up a command that runs in a particular directory with a small number of overridden environment variables — the common case when environment variables other than
PWDare changed at all.Compatibility
This proposal would set an updated
PWDvariable for some commands that today receive a stale one (or none at all) inherited from the parent Go process.It would have no effect on commands that have an empty
Dirfield or a non-nilEnvfield.It may change the observable behavior of commands that have a nil
Envbut are sensitive to the value ofPWD, including commands that enumerate all variables in the environment. For commands that are sensitive to the value of thePWDvariable, it should nearly always be a strict improvement: the semantics of thePWDvariable are explicitly described in the POSIX standard, and this change would bring the behavior in line with that standard on POSIX-like platforms.The only case I can conceive of in which a user explicitly wants to inherit a missing or mismatched
PWDis when testing the behavior of a command-line binary in a non-POSIX-compliant environment. However, for that rare case it would be straightforward for the user to explicitly set theEnvfield to a slice with the desiredPWDvariable, including no such variable at all.Limitations
This proposal would not fix the
PWDvariable for commands that explicitly setEnvwith an incorrect or missingPWDtoday (such as from an explicit call toos.Environ()). That is a deliberate choice to bias toward maintaining compatibility, but may limit the scope of improvements.Alternatives considered
PWDvariable more or less aggressively:a. We could omit the
PWDfrom the command's environment if the Go process itself has noPWDvariable, even if the platform normally usesPWD.b. We could set
PWD(on platforms that use it) wheneverDiris non-empty andEnvdoes not include an explicit entry for it, analogous to the conditions under which we setSYSTEMROOTon Windows today. That would fix commands that set explicit variables but accidentally omitPWD, but would remove the ability to deliberately leavePWDcompletely unset when using theDirfield. (At best, it would be set to the empty string.)c. We could set
PWD(on platforms that use it) wheneverDiris non-empty andEnvincludes aPWDthat is not an absolute path that resolves toDir. That would enforce compliance with the POSIX standard for the variable, but may be expensive to detect, and would remove the ability to deliberately use a noncompliantPWD.In my opinion, options (a) or (b) would be reasonable but (c) would be too invasive.
Environmethod, we could add aSetDirmethod that both sets theDirfield and appends an appropriate value to theEnvfield, populating theEnvfield usingos.Environ()if it was previously nil.I worry that appending to the existing
Envfield might be a bit too subtle: it wouldn't be intuitively obvious to me whetherSetDirappends toEnv(potentially overwriting entries in aliased slices), or toEnv[:len(Env):len(Env)](producing a possibly-unexpected new allocation). That could be addressed with documentation, but I think theEnvironmethod is easier to describe, especially by analogy toos.Environ.The text was updated successfully, but these errors were encountered: