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

x/tools/go/packages: document role of PWD in DriverRequest protocol #67757

Closed
adonovan opened this issue May 31, 2024 · 7 comments
Closed

x/tools/go/packages: document role of PWD in DriverRequest protocol #67757

adonovan opened this issue May 31, 2024 · 7 comments
Assignees
Labels
Documentation Issues describing a change to documentation. Proposal Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@adonovan
Copy link
Member

adonovan commented May 31, 2024

Original title: x/tools/go/packages: add DriverRequest.Dir string
(Proposal withdrawn; this is now a doc issue.)

The go/packages driver protocol runs an external command in place of go list to retrieve Go package metadata from the build system, whatever it is. It has hitherto always implicitly assumed that the name of the directory in which the command is run will be used to absolutize any relative file names generated by the command. However, creating a process loses information about the directory name: the string is lost, only the inode is retained by the process. (Confusing matters, there is a special-case heuristic when $PWD exactly matches--not prefix-matches--the directory... though happily it can be used as the basis of a work-around.) Consequently, the string returned by Go's os.Getwd or C's getcwd(3) within the child process may differ from the original string, and this difference is crucial, since the returned filenames will not satisfy the original query.

We propose to add an explicit Dir string field to the DriverRequest struct to be used for path absolutization.

type DriverRequest struct {
      ...

+    // Dir is the preferred name of the driver command's working directory.
+    // It must be an absolute path that denotes the same directory as os.Getwd,
+    // though it may be an alias. If empty, os.Getwd is used.
+    // This can be used to ensure that symbolic links in directory names
+    // (notably /tmp on macOS) are not inadvertently expanded.
+    Dir string 
}

(Context: https://go.dev/cl/588767)

@findleyr

@rsc
Copy link
Contributor

rsc commented Jun 12, 2024

Per discussion, setting PWD correctly when using os/exec is likely the right solution here rather than adding new API.

@ianlancetaylor
Copy link
Member

And the way to set PWD correctly is to set cmd.Dir and to either not set cmd.Env or to set it by appending things to the result of cmd.Environ().

@adonovan
Copy link
Member Author

adonovan commented Jun 13, 2024

FWIW, getcwd(3), the usual way for a C program to find the name of the current directory, does not do the $PWD shortcut trick, nor is it allowed to by POSIX.1: "The pathname copied to the array shall contain no components that are symbolic links."

xtools$ cat cwd.c
#include <stdio.h>
#include <unistd.h>

int main() {
	char buf[1000];
	puts(getcwd(buf, sizeof buf));
}

xtools$ gcc cwd.c
xtools$ exe=$(pwd)/a.out
xtools$ $exe
/Users/adonovan/w/xtools
xtools$ mkdir /tmp/real
xtools$ ln -sf /tmp/real /tmp/link 
xtools$ ( cd /tmp/link && $exe )
/private/tmp/real
xtools$ ( cd /tmp/link && PWD=/tmp/link $exe )
/private/tmp/real

I realize that the odds of the driver program being in a language other than Go are low, but I don't think this interface should assume anything about the implementation language of the child process or require it to know that it should use os.Getenv("PWD") in preference to getcwd when constructing the filenames that it prints.

@ianlancetaylor
Copy link
Member

The C library function that looks at PWD is get_current_dir_name.

Checking PWD is a pretty wide-spread convention even if getcwd and getwd don't do it.

If we set PWD, is this causing actual problems?

@adonovan
Copy link
Member Author

adonovan commented Jun 14, 2024

The C library function that looks at PWD is get_current_dir_name.
Checking PWD is a pretty wide-spread convention even if getcwd and getwd don't do it.

FWIW, that's a GNUism rather than a standard feature of C, and I don't think I ever used it (or was much aware of it, or the motivating subtlety) when I was working in C/C++. But perhaps you're right that there is a convention.

If we set PWD, is this causing actual problems?

Not at present. Perhaps this can be downgraded to a documentation issue. I think there are two aspects:

  1. os/exec.Cmd should document its treatment of Dir and PWD, explain that os.Getwd checks PWD, and recommend this approach for all subprocesses regardless of implementation language. I've filed os/exec: document subtleties around Cmd.Dir, PWD, os.Getwd and C's getcwd #68000.
  2. packages.DriverRequest should explain that it sets PWD and expects that the child process will honor it, redirecting the reader to comment number 1 for more detail.

@adonovan
Copy link
Member Author

Retracting the proposed change; leaving the issue open until item 2 in the previous note is addressed.

@adonovan adonovan changed the title proposal: x/tools/go/packages: add DriverRequest.Dir string x/tools/go/packages: document role of PWD in DriverRequest protocol Jun 20, 2024
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. Documentation Issues describing a change to documentation. labels Jun 20, 2024
@adonovan adonovan self-assigned this Sep 26, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/616057 mentions this issue: go/packages: document the role of PWD

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues describing a change to documentation. Proposal Tools This label describes issues relating to any tools in the x/tools repository.
Projects
Status: Incoming
Development

No branches or pull requests

4 participants