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: add UserShell #34910

Closed
twpayne opened this issue Oct 15, 2019 · 9 comments
Closed

proposal: os: add UserShell #34910

twpayne opened this issue Oct 15, 2019 · 9 comments
Labels
Milestone

Comments

@twpayne
Copy link

@twpayne twpayne commented Oct 15, 2019

Go has added several functions for determining the user's configuration:

  • os.UserCacheDir in #22536 (Go 1.11).
  • os.UserHomeDir in #26463 (Go 1.12).
  • os.UserConfigDir in #29960 (Go 1.13).

Closely related to this is getting the user's shell, which is very OS-dependent, and surprisingly tricky to accomplish (especially if cgo is not available, for example when cross-compiling). Go's behavior of falling back to parsing local files (e.g. /etc/passwd) when cgo is not available has led to a number of bugs in applications (e.g. #24383, twpayne/chezmoi#65).

Being able to invoke the user's shell is useful in interactive applications (e.g. VSCode's interactive terminal), command line applications (e.g. chezmoi's cd command, or any application that launches a user-specified command on the user's behalf.

There are existing packages to do this, for example:

This proposal is concretely:

Add a os.UserShell function that returns the current user's shell.

Optionally, the os.UserShell function could return some indication of the level of confidence in the returned result (if the current user's shell could not be determined, maybe it makes sense to fall back to an OS-dependent default shell).

@gopherbot gopherbot added this to the Proposal milestone Oct 15, 2019
@gopherbot gopherbot added the Proposal label Oct 15, 2019
@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Oct 16, 2019

The os.User*Dir funcs are easy to use; you get a directory, so you can create more directories underneath. I'm not so sure how the API would be generally useful here.

For example, if the API returns /bin/sh or cmd.exe, how should one call that with any arguments in a portable way? Or is it only meant to be called with no arguments, to then be used interactively or with standard input?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 16, 2019

What is the use case here? When do you specifically want to run the user's shell?

The packages you point to seem to be looking in /etc/passwd for the shell, which seems to mean that if we added this functionality it should go in the os/user package, not the os package.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Oct 21, 2019

This seems too system specific.
Also, on Unix, you should use $SHELL, not /etc/passwd.

@twpayne

This comment has been minimized.

Copy link
Author

@twpayne twpayne commented Oct 21, 2019

Handling responses:

For example, if the API returns /bin/sh or cmd.exe, how should one call that with any arguments in a portable way? Or is it only meant to be called with no arguments, to then be used interactively or with standard input?

What is the use case here? When do you specifically want to run the user's shell?

The arguments can be supplied by the user, for example in any kind of hook ("when I do X, invoke my shell to do Y"). Interactive and/or standard input uses are also valid use cases, e.g. for any applications that provide an embedded shell.

The packages you point to seem to be looking in /etc/passwd for the shell, which seems to mean that if we added this functionality it should go in the os/user package, not the os package.

The suggestion for putting it in the os package is for consistency with the os.User*Dir functions. It would also fit in the os/user package.

The packages you point to seem to be looking in /etc/passwd for the shell...
This seems too system specific.
Also, on Unix, you should use $SHELL, not /etc/passwd.

The method used to determine the user's shell include:

  • Calling the getpwnam_r library function, if available.
  • Invoking a command that does the same (e.g. getent on Linux, dscl on macOS).
  • Reading /etc/passwd (on some systems, e.g. Termux, the current user doesn't have an entry).
  • Reading the $SHELL environment variable (assuming the user's environment is set up correctly).

In short, this is a superficially simple function that hides a lot of system-specific underlying complexity from the caller.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 22, 2019

Given that we need to call getpwnam_r, it must go in os/user, not os. I don't see any particular similarity to os.UserHomeDir, etc., as those are all directories.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 22, 2019

That said, it's not clear that this needs to go into the standard library. The argument that programs will use the user's shell to execute scripts seems very special purpose, as that can only be done for scripts provided by the user. Why not just have the user provide the shell at the same time?

Also, https://golang.org/doc/faq#x_in_std .

@twpayne

This comment has been minimized.

Copy link
Author

@twpayne twpayne commented Oct 22, 2019

Given that we need to call getpwnam_r, it must go in os/user, not os. I don't see any particular similarity to os.UserHomeDir, etc., as those are all directories.

OK for putting it in os/user.

Go's standard library provides nearly all the functionality to ready the /etc/passwd and /etc/group databases - with the exception of the user's shell. It could be argued that the user's shell is an even more fundamental attribute than say the user's config directory or cache directory.

That all said, I do realize that the use cases for needing to know the user's shell are limited and so am happy if this proposal is closed unaccepted. Thank you for considering it.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Oct 30, 2019

This seems too special purpose. It can be done outside the standard library.
It seems like a likely decline to me.

@twpayne

This comment has been minimized.

Copy link
Author

@twpayne twpayne commented Oct 30, 2019

Ack for decline. Thank you for considering it :)

Closing this issue.

@twpayne twpayne closed this Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.