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

Extract $HOME usages into utils.GetHomeDir() (reopen #10358) #10602

Merged
merged 2 commits into from Feb 6, 2015

Conversation

Projects
None yet
8 participants
@ahmetb
Contributor

ahmetb commented Feb 5, 2015

Refactored getHomeDir in docker/docker to GetHomeDir in utils Currently covers all use cases on the client-side.

@ahmetb ahmetb referenced this pull request Feb 5, 2015

Merged

Pretty the help text #10547

Extract $HOME usages into utils.GetHomeDir()
Refactored getHomeDir in docker/docker to GetHomeDir in utils
pkg. Currently covers all use cases on the client-side.

Signed-off-by: Ahmet Alp Balkan <ahmetb@microsoft.com>
@LK4D4

This comment has been minimized.

Contributor

LK4D4 commented Feb 6, 2015

LGTM

@tiborvass

This comment has been minimized.

Collaborator

tiborvass commented Feb 6, 2015

~~Can we create a new package for this? not sure how to call it... osutil? ~~

EDIT: +1 to mitchellh/go-homedir or pkg/home.

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 6, 2015

I'll see if I can vendor that package but it will just add one more trivial dependency. Are we sure we want to do that? It seems pretty harmless to have this in "util" since the pkg already exists.

@tianon

This comment has been minimized.

Member

tianon commented Feb 6, 2015

@tiborvass

This comment has been minimized.

Collaborator

tiborvass commented Feb 6, 2015

My position is that it's not because we have a utils package that we want to continue adding to it :)

@phemmer

This comment has been minimized.

Contributor

phemmer commented Feb 6, 2015

Also, $HOME can't be relied upon. There are many cases where the HOME environment variable is unset.
It's safest to use the os/user package.

package main
import (
    "fmt"
    "os"
    "os/user"
)

func main() {
    fmt.Printf("HOME: %#v\n", os.Getenv("HOME"))
    u, _ := user.Current()
    fmt.Printf("user.HomeDir: %#v\n", u.HomeDir)
}
# env -i go run test.go  
HOME: ""
user.HomeDir: "/home/phemmer"
@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 6, 2015

@phemmer that's actually a cleaner solution. Is there a reason why this hasn't been used in the first place? e.g. mitchellh/go-homedir mentions:

Why not just use os/user? The built-in os/user package requires cgo on Darwin systems. This means that any Go code that uses that package cannot cross compile. But 99% of the time the use for os/user is just to retrieve the home directory, which we can do for the current user without cgo. This library does that, enabling cross-compilation.

@tianon

This comment has been minimized.

Member

tianon commented Feb 6, 2015

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 6, 2015

@tianon yeah makes sense stick with this change then.

@tiborvass

This comment has been minimized.

Collaborator

tiborvass commented Feb 6, 2015

@ahmetalpbalkan @tianon I'm still voting for pkg/home or pkg/homedir. I don't want to add more stuff to utils, only remove.

@tianon

This comment has been minimized.

Member

tianon commented Feb 6, 2015

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 6, 2015

I'll work on creating pkg/homedir.

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 6, 2015

@tiborvass @tianon so there's a possibility that @phemmer brought up, where environment variables are not set. I'm making my changes by ignoring that fact because all the code currently depends on that.

@tiborvass

This comment has been minimized.

Collaborator

tiborvass commented Feb 6, 2015

@ahmetalpbalkan fair enough!

@tianon

This comment has been minimized.

Member

tianon commented Feb 6, 2015

@duglin

This comment has been minimized.

Contributor

duglin commented Feb 6, 2015

Is there a process or set of tests someplace in our build that we could modify to grep all of our code to make sure people don't use $HOME ? Just looking for a way to make sure something doesn't slip by our reviews.

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 6, 2015

I just added another commit to export this to pkg/homedir. I'll squash into one before submitting. PTAL.

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 6, 2015

@duglin I believe we don't have any test in place. Certain test cases and daemon-referenced packages are using os.Getenv("HOME") or $PATH. I guess a validation step can be added using grep by whitelisting this file in the makefiles.

Export $HOME lookup to pkg/homedir
Signed-off-by: Ahmet Alp Balkan <ahmetb@microsoft.com>
@tiborvass

This comment has been minimized.

Collaborator

tiborvass commented Feb 6, 2015

LGTM

1 similar comment
@jessfraz

This comment has been minimized.

Contributor

jessfraz commented Feb 6, 2015

LGTM

tiborvass added a commit that referenced this pull request Feb 6, 2015

Merge pull request #10602 from ahmetalpbalkan/dev/ahmetb-homedir-fix
Extract $HOME usages into utils.GetHomeDir() (reopen #10358)

@tiborvass tiborvass merged commit 9491cec into moby:master Feb 6, 2015

1 check passed

janky Jenkins build 'Docker-PRs' #333 has succeeded
Details

@ahmetb ahmetb deleted the ahmetb:dev/ahmetb-homedir-fix branch Feb 6, 2015

@mattmoor

This comment has been minimized.

Contributor

mattmoor commented on f9ae2d4 Feb 10, 2015

@ahmetalpbalkan Why didn't you use the standard Go library for this?

http://stackoverflow.com/questions/7922270/obtain-users-home-directory

You should also fixup the python client library to read this from the correct location:
https://github.com/docker/docker-py/blob/40770878ad5b6fccc1b5f5866fa1167bb990f3d3/docker/auth/auth.py#L128

thanks.

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 10, 2015

@mattmoor discussion about this is at #10602. #10602 (comment) it prevents us from cross-compiling binaries since it requires cgo to compile os/user on darwin.

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 10, 2015

You should also fixup the python client library to read this from the correct location:
https://github.com/docker/docker-py/blob/40770878ad5b6fccc1b5f5866fa1167bb990f3d3/docker/auth/auth.py#L128

Umm, feel free to change it. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment