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 UserHomeDir #26463

Closed
kolyshkin opened this issue Jul 19, 2018 · 13 comments
Closed

proposal: os: add UserHomeDir #26463

kolyshkin opened this issue Jul 19, 2018 · 13 comments

Comments

@kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Jul 19, 2018

This is a proposal to add Home() method to os/user package to get the user's home directory.

There is a bad trend to use the data returned by user.Current() in order to get user's home directory. It's bad to use Current() to get the value of home dir for two reasons:

  1. $HOME should be used, if available, as user's home directory. Quoting http://man7.org/linux/man-pages/man3/getpwuid_r.3.html:

The pw_dir field contains the name of the initial working directory
of the user. Login programs use the value of this field to
initialize the HOME environment variable for the login shell. An
application that wants to determine its user's home directory should
inspect the value of HOME (rather than the value
getpwuid(getuid())->pw_dir) since this allows the user to modify
their notion of "the home directory" during a login session.

  1. user.Current() is not implemented for some platforms (notably, Linux when building statically with glibc).

In a sense, currently os/user provides a not-quite-right way to get user's home directory, and lots of software is using this way. Why it's out of scope of this project to fix broken third-party software, perhaps providing the right way would do some good.

Unix/Linux/Darwin implementation of Home() can look as simple as:

func Home() string {
        if v := os.Getenv("HOME"); v != "" {
                return v
        }
        // fallback
        if u, err := Current(); err == nil {
                return u.HomeDir
        }
        return ""
}

Windows version can be like:

func Home() string {
        return os.Getenv("USERPROFILE")
}

Example

Here is an example (of bad usage of user.Current()) from this very repo:

u, err := user.Current()
if err != nil {
if debugExecDarwinRoots {
println(fmt.Sprintf("crypto/x509: get current user: %v", err))
}
} else {
args = append(args,
filepath.Join(u.HomeDir, "/Library/Keychains/login.keychain"),
// Fresh installs of Sierra use a slightly different path for the login keychain
filepath.Join(u.HomeDir, "/Library/Keychains/login.keychain-db"),
)
}

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Jul 19, 2018

#22536 acts as a bit of a precedent. Also drawing from that proposal, I presume that the function should be called os.HomeDir.

@kolyshkin

This comment has been minimized.

Copy link
Contributor Author

@kolyshkin kolyshkin commented Jul 19, 2018

@mvdan thanks for the input. I tend to agree that it should be named os.HomeDir() (and maybe not include a call to Current() as $HOME is supposed to always be set).

@mvdan mvdan changed the title os/user: add Home() Proposal: os/user: add HomeDir() Jul 19, 2018
@gopherbot gopherbot added this to the Proposal milestone Jul 19, 2018
@gopherbot gopherbot added the Proposal label Jul 19, 2018
@mvdan mvdan changed the title Proposal: os/user: add HomeDir() Proposal: os: add UserHomeDir Jul 19, 2018
@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Jul 19, 2018

I've re-titled the issue to be a proposal, and to use the same package and naming scheme as the previous proposal for consistency. @kolyshkin feel free to change it if you disagree.

Another open question is whether or not the function should return an error. UserCacheDir initially didn't, but now it does. I think the reasoning is that without the error return, users may forget to check if the result is the empty string. If the function returns an error, it's obvious what the user should do to check if something went wrong.

@kolyshkin

This comment has been minimized.

Copy link
Contributor Author

@kolyshkin kolyshkin commented Jul 19, 2018

Yes, I believe it makes sense to return an error.

One other related thing is, it might make sense to add simple /etc/passwd parser to use as replacement for calling getpwuid_r (which is what user.Current() does), as it's not working for the linux static build case.

@bcmills bcmills changed the title Proposal: os: add UserHomeDir proposal: os: add UserHomeDir Jul 19, 2018
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 19, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 19, 2018

This differs from os.UserCacheDir in that the os package can not import the os/user package. Either this has to be in the os/user package, or it has to simply return $HOME (on Unix).

What should it look like on Plan 9? @0intro

@0intro

This comment has been minimized.

Copy link
Member

@0intro 0intro commented Jul 19, 2018

On Plan 9, I think it should look like Unix, except the environment variable is called home instead of HOME.

The Home function would look like:

func Home() string {
        if v := os.Getenv("home"); v != "" {
                return v
        }
        // fallback
        if u, err := Current(); err == nil {
                return u.HomeDir
        }
        return ""
}
@josharian

This comment has been minimized.

Copy link
Contributor

@josharian josharian commented Jul 19, 2018

This differs from os.UserCacheDir in that the os package can not import the os/user package.

FWIW, I still don’t understand why it is os.UserCacheDir instead of os/user.CacheDir.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 19, 2018

FWIW, I still don’t understand why it is os.UserCacheDir instead of os/user.CacheDir.

#22536 (comment)
and subsequent discussion.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jul 23, 2018

os.UserHomeDir is OK to add, alongside os.UserCacheDir. The "User" in UserCacheDir was there to distinguish the user-local cache directory from any system-wide cache directories. There is less ambiguity for HomeDir but maybe UserHomeDir fits next to UserCacheDir better.

euank added a commit to euank/jump that referenced this issue Sep 3, 2018
As discussed in golang/go#26463, Go's
'user.Current().HomeDir' does not do the right thing.
The 'HOME' environment variable is meant to take precedent over
whatever's in /etc/passwd.

This updates the config directory initialization code to use the correct
home directory.
euank added a commit to euank/jump that referenced this issue Sep 4, 2018
As discussed in golang/go#26463, Go's
'user.Current().HomeDir' does not do the right thing.
The 'HOME' environment variable is meant to take precedent over
whatever's in /etc/passwd.

This updates the config directory initialization code to use the correct
home directory.
@bradfitz bradfitz added the NeedsFix label Oct 3, 2018
@gopherbot gopherbot removed the NeedsDecision label Oct 3, 2018
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 3, 2018

Change https://golang.org/cl/139418 mentions this issue: os: add UserHomeDir

@gopherbot gopherbot closed this in fa1a49a Oct 3, 2018
gopherbot pushed a commit that referenced this issue Oct 4, 2018
As pointed out in #26463,
HOME (or equivalent) environment variable (rather than the
value obtained by parsing /etc/passwd or the like) should be
used to obtain user's home directory.

Since commit fa1a49a there's a method to obtain
user's home directory -- use it here.

Change-Id: I852fbb24249bcfe08f3874fae6e7b9d01d869190
Reviewed-on: https://go-review.googlesource.com/c/139426
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@stapelberg

This comment has been minimized.

Copy link
Contributor

@stapelberg stapelberg commented Nov 2, 2018

Is it intentional that UserHomeDir no longer matches UserCacheDir in how errors are handled?

UserHomeDir unconditionally returns Getenv("HOME"), whereas commit 50bd1c4 changed UserCacheDir to return an error instead of an empty string when HOME is unset.

Shouldn’t they be consistent?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 2, 2018

@stapelberg Thanks, opened #28562 for this.

JackOfMostTrades pushed a commit to JackOfMostTrades/go-crypto that referenced this issue Feb 3, 2019
As pointed out in golang/go#26463,
HOME (or equivalent) environment variable (rather than the
value obtained by parsing /etc/passwd or the like) should be
used to obtain user's home directory.

Since commit fa1a49aa556d8 there's a method to obtain
user's home directory -- use it here.

Change-Id: I852fbb24249bcfe08f3874fae6e7b9d01d869190
Reviewed-on: https://go-review.googlesource.com/c/139426
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Nov 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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