-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
pkg/system: move some functions to a new home #44275
Conversation
Looks like the change in default |
c28bccc
to
28ea9a0
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about switching on runtime.GOOS
in the test instead of more platform files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this still use the canonical import path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
vendor
directory. - With our plans to (ultimately) move to Go modules, all of these comments will be removed (
go.mod
will 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be a call to oci.DefaultPathEnv
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
badImports=($(go list -e -f '{{ join .Deps "\n" }}' "$f" | sort -u \ | |
| grep -vE '^github.com/docker/docker/pkg/' \ | |
| grep -vE '^github.com/docker/docker/vendor' \ | |
| grep -vE '^github.com/docker/docker/internal' \ | |
| grep -E '^github.com/docker/docker' \ | |
|| true)) | |
unset IFS |
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 <github@gone.nl>
It's only used for an integration test, and has no external consumers. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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 <github@gone.nl>
b972bd1
to
fb77973
Compare
Thx! I'll bring this one in |
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.