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.

Sign up for GitHub

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

Jump to bottom

os,os/exec: should Environ record environment variables in the test log? #60057

Open
bcmills opened this issue May 8, 2023 · 2 comments
Open

os,os/exec: should Environ record environment variables in the test log? #60057

bcmills opened this issue May 8, 2023 · 2 comments
Labels
GoCommand cmd/go NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Backlog

Comments

@bcmills
Copy link
Contributor

bcmills commented May 8, 2023

Since CL 81895, os.Getenv and os.LookupEnv both record the keys in the test log:
https://cs.opensource.google/go/go/+/master:src/os/env.go;l=98-115;drc=b2faff18ce28edad98303d2c3134dec1331fd7b5

However, os.Environ does not:
https://cs.opensource.google/go/go/+/master:src/os/env.go;l=137-141;drc=b2faff18ce28edad98303d2c3134dec1331fd7b5

This means that, for example, a program that iterates over os.Environ() looking for keys has different caching behavior than one that calls LookupEnv on those exact same keys. This difference is especially pronounced in tests that execute subprocesses, because the variables accessed by the subprocess (which may be very significant, especially in the case of libraries like golang.org/x/tools/go/packages!) do not end up recorded in the cache key.

(For example, see the odd-looking Getenv loop in testenv.GoToolPath!)

It's not obvious to me whether the difference is an intentional decision or an oversight. Either way, I think we should consider recording all keys in the test log whenever os.Environ or execenv.Default is called.

(CC @ianlancetaylor)
@bcmills bcmills added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 8, 2023
@bcmills bcmills added this to the Backlog milestone May 8, 2023
@bcmills
Copy link
Contributor Author

bcmills commented May 8, 2023

This question is motivated by:

@bcmills bcmills mentioned this issue May 8, 2023
Closed
@ianlancetaylor
Copy link
Contributor

I note that that was actually an unresolved comment on https://go.dev/cl/81895: https://go-review.googlesource.com/c/go/+/81895/comment/486513ce_5f69647f/ .

The main problem with adding all of os.Environ to the test hash is that it means that when users run tests that call os.Environ, those tests will generally not be hashed. For example, my current environment has at least the following variables with unpredictable values: SESSION_MANAGER, SSH_AUTH_SOCK, GNOME_TERMINAL_SCREEN. Caching would apply if I ran a test twice in a row, but not if I run it, log out, log in again, and run it again.

@bcmills bcmills added the GoCommand cmd/go label Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
GoCommand cmd/go NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
2 participants
@ianlancetaylor @bcmills