homedir: remove idtools and libcontainer's user package dependencies#39975
homedir: remove idtools and libcontainer's user package dependencies#39975thaJeztah merged 1 commit intomoby:masterfrom
Conversation
1fffa3b to
0f84737
Compare
|
This looks reasonable to me. Are there any other options for statically compiling? ping @jonjohnsonjr |
0f84737 to
ca4a5c7
Compare
There was a problem hiding this comment.
There is no longer homedir.GetStatic(), this comment needs to be updated.
There was a problem hiding this comment.
@kolyshkin Should I move ensureHomeIfIAmStatic to gcplogging.go ?
There was a problem hiding this comment.
Nevermind i'm just removing GetStatic from the comment.
kolyshkin
left a comment
There was a problem hiding this comment.
LGTM except for nitpick about a leftover comment
About github.com/opencontainers/runc/libcontainer/user: According to opencontainers/runc@195d8d5 this package has two functions: - Have a static implementation of user lookup, which is now supported in the os/user stdlib package with the osusergo build tag, but wasn't at the time. - Have extra functions that os/user doesn't have, but none of those are used in homedir. Since moby#11287, homedir depended directly on libcontainer's user package for CurrentUser(). This is being replaced with os/user.Current(), because all of our static binaries are compiled with the osusergo tag, and for dynamic libraries it is more correct to use libc's implementation than parsing /etc/passwd. About github.com/docker/docker/pkg/idtools: Only dependency was from GetStatic() which uses idtools.LookupUID(uid). The implementation of idtools.LookupUID just calls to github.com/opencontainers/runc/libcontainer/user.LookupUid or fallbacks to exec-ing to getent (since moby#27599). This patch replaces calls to homedir.GetStatic by homedir.Get(), opting out of supporting nss lookups in static binaries via exec-ing to getent for the homedir package. If homedir package users need to support nss lookups, they are advised to compile dynamically instead. Signed-off-by: Tibor Vass <tibor@docker.com>
ca4a5c7 to
a8608b5
Compare
|
Actually, this kludge of setting |
|
@kolyshkin i'm pretty sure it's not needed anymore, it would just prevent reading from /etc/passwd again. Let me know if you want me to remove it entirely, though I'd love @AkihiroSuda's input too. |
|
OK, in case |
|
Hmm. This is all chicken-and-egg, we use user.Current() to get homedir which gets it from os.Getenv("HOME"), and then do os.Setenv("HOME=xxx"). This no longer makes sense. I think we can now remove the whole |
|
That is incorrect. It looks up /etc/passwd first via |
kolyshkin
left a comment
There was a problem hiding this comment.
LGTM
Removing ensureHomeIfIAmStatic() can be done separately
|
i noticed flaky test |
About github.com/opencontainers/runc/libcontainer/user:
According to opencontainers/runc@195d8d5
this package has two functions:
os/user stdlib package with the osusergo build tag, but wasn't at the time.
in homedir.
Since #11287, homedir depended directly on
libcontainer's user package for CurrentUser().
This is being replaced with os/user.Current(), because all of our static
binaries are compiled with the osusergo tag, and for dynamic libraries it
is more correct to use libc's implementation than parsing /etc/passwd.
About github.com/docker/docker/pkg/idtools:
Only dependency was from GetStatic() which uses idtools.LookupUID(uid).
The implementation of idtools.LookupUID just calls to
github.com/opencontainers/runc/libcontainer/user.LookupUid or fallbacks
to exec-ing to getent (since #27599).
This patch replaces calls to homedir.GetStatic by homedir.Get(), opting out
of supporting nss lookups in static binaries via exec-ing to getent for
the homedir package.
If homedir package users need to support nss lookups, they are advised
to compile dynamically instead.
Signed-off-by: Tibor Vass tibor@docker.com