pkg/system: move some functions to a new home#44275
Conversation
|
Looks like the change in default |
thaJeztah
force-pushed
the
move_pkg_system_funcs
branch
3 times, most recently
from
c28bccc to
28ea9a0
Compare
thaJeztah
force-pushed
the
move_pkg_system_funcs
branch
2 times, most recently
from
41848ad to
b972bd1
Compare
| // getLongPathName converts Windows short pathnames to full pathnames. | ||
| // For example C:\Users\ADMIN~1 --> C:\Users\Administrator. | ||
| // It is a no-op on non-Windows platforms | ||
| func getLongPathName(path string) (string, error) { |
There was a problem hiding this comment.
What do you think about switching on runtime.GOOS in the test instead of more platform files?
There was a problem hiding this comment.
Unfortunately that won't work, as this uses "golang.org/x/sys/windows", which only contains files buildable on OS=windows, so no wrappers / stubs available
| // +build windows | ||
|
|
||
| package system // import "github.com/docker/docker/pkg/system" | ||
| package archive |
There was a problem hiding this comment.
Should this still use the canonical import path?
There was a problem hiding this comment.
The canonical import path comments act at the "package" level, so technically we only need a single file within the package to have such a comment. We did add them to all (most) files, mostly to be explicit, but strictly, it's not needed.
That said; reasons I didn't add the comment here;
- Go nowadays completely ignore them if Go modules are used (in any form), even when using a
vendordirectory. - With our plans to (ultimately) move to Go modules, all of these comments will be removed (
go.modwill be leading) - ^^ because of that, I gradually remove them in places they'e not needed, so that we have less code-churn once we do the move to go modules.
| // | ||
| // Deprecated: use oci.DefaultPathEnv | ||
| func DefaultPathEnv(os string) string { | ||
| if os == "windows" { |
There was a problem hiding this comment.
Can this be a call to oci.DefaultPathEnv?
There was a problem hiding this comment.
Doing so would add oci (including containerd as indirect) as dependency for a pkg/, which wasn't desirable, and would fail in CI:
moby/hack/validate/pkg-imports
Lines 14 to 20 in 3b84630
I couldn't find external consumers of this function, so perhaps we could've skipped the "deprecated" altogether, but we should be able to remove this file after it's been in a release.
This patch: - Deprecates pkg/system.DefaultPathEnv - Moves the implementation inside oci - Adds TODOs to align the default in the Builder with the one used elsewhere Signed-off-by: Sebastiaan van Stijn <[email protected]>
It's only used for an integration test, and has no external consumers. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
This one is a "bit" fuzzy, as it may not be _directly_ related to `archive`, but it's always used _in combination_ with the archive package, so moving it there. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
move_pkg_system_funcs
branch
from
b972bd1 to
fb77973
Compare
|
Thx! I'll bring this one in |
2 participants
pkg/system: move DefaultPathEnv to oci, use BuildKit for builder
This patch:
pkg/system: move GetLongPathName to integration-cli
It's only used for an integration test, and has no external consumers.
pkg/system: move CheckSystemDriveAndRemoveDriveLetter to pkg/archive
This one is a "bit" fuzzy, as it may not be directly related to
archive,but it's always used in combination with the archive package, so moving it
there.