The Wayback Machine - https://web.archive.org/web/20240919233135/https://github.com/docker/cli/pull/5432
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

[27.x backport] command: check for wsl mount path on windows #5432

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

thaJeztah
Copy link
Member

- What I did

This checks for the equivalent WSL mount path on windows. WSL will mount the windows drives at /mnt/c (or whichever drive is being used).

- How I did it

The code will now check to see if a file exists in /mnt/<drive>. If there is no drive in the path, it'll default to the C drive. If that file exists, it will use it. The check now also supports URLs of the format c:/path/to/file (note the forward slashes rather than backslashes).

- How to verify it

Integrate the change with buildx and run a build in a WSL container on Windows.

- Description for the changelog

* Properly report metrics when run in WSL environment on Windows

This checks for the equivalent WSL mount path on windows. WSL will mount
the windows drives at `/mnt/c` (or whichever drive is being used).

This is done by parsing a UNC path with forward slashes from the unix
socket URL.

Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
(cherry picked from commit 38c3fef)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah added this to the 27.3.0 milestone Sep 12, 2024
@thaJeztah thaJeztah changed the title command: check for wsl mount path on windows [27.x backport] command: check for wsl mount path on windows Sep 12, 2024
@thaJeztah thaJeztah self-assigned this Sep 12, 2024
@codecov-commenter
Copy link

codecov-commenter commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 47.72727% with 23 lines in your changes missing coverage. Please review.

Project coverage is 59.79%. Comparing base (74bebe2) to head (b48720e).
Report is 3 commits behind head on 27.x.

Additional details and impacted files
@@            Coverage Diff             @@
##             27.x    #5432      +/-   ##
==========================================
- Coverage   59.81%   59.79%   -0.03%     
==========================================
  Files         345      345              
  Lines       23396    23439      +43     
==========================================
+ Hits        13994    14015      +21     
- Misses       8432     8450      +18     
- Partials      970      974       +4     

@thaJeztah thaJeztah requested a review from a team September 12, 2024 22:43
@tonistiigi
Copy link
Member

tonistiigi commented Sep 12, 2024

Because there are lots of changes in 27.x , can we get a 27.2.x (or just 27.2) branch? This PR would still go in as well.

@laurazard
Copy link
Member

For vendoring into BuildKit/Buildx?

@tonistiigi
Copy link
Member

@laurazard Yes, for vendoring into buildx.

Copy link
Member

@laurazard laurazard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@laurazard
Copy link
Member

@tonistiigi I'm a little worried about dependencies mismatch/unexpected consequences of not having all those lined up, especially thinking about OTEL dependencies and things of the sort, unless we take care to backport all of those as well.

Are there that many changes vs the last 27.2.x release?

We can definitely do it, just need to take some care/would prefer not to.

@laurazard
Copy link
Member

@thaJeztah thoughts on that?

@tonistiigi
Copy link
Member

@laurazard We only want this change in buildx v0.17.1 , no other cli changes or vendor updates. Rest of 27.x (27.3) can go to buildx master. v0.17.0 has v27.2.1 vendored atm.

@laurazard
Copy link
Member

Right but will buildx 0.17.1 go out with engine+CLI 27.3? I guess it's less scary if it's just for buildx/not BuildKit. I can't think of any change since 27.2.1 in the CLI code that would require a vendor in plugins. We could branch off of whatever last commit went into 27.2.1 and cherry-pick this PR if you're okay with vendoring a branch.

@tonistiigi
Copy link
Member

Right but will buildx 0.17.1 go out with engine+CLI 27.3?

No, that would probably be v0.18.

We could branch off of whatever last commit went into 27.2.1 and cherry-pick this PR if you're okay with vendoring a branch.

Yes, that is fine. We can vendor a branch commit. We don't need a 27.2.2 tag for this.

@thaJeztah
Copy link
Member Author

Because there are lots of changes in 27.x , can we get a 27.2.x (or just 27.2) branch? This PR would still go in as well.

@tonistiigi I'm confused; what are the "lots of changes"? Looking at the list of PRs for this release; https://github.com/docker/cli/pulls?q=is%3Apr+milestone%3A27.3.0

Outside of this PR, the only changes are a bugfix only affecting the docker CLI itself (#5426), a change in the output for the docker CLI itself (#5423), and some documentation updates. All vendor updates are because of buildkit those updates

For the daemon https://github.com/moby/moby/pulls?q=is%3Apr+milestone%3A27.3.0

A bugfix for NVIDIA GPU support (moby/moby#48483), and containerd handling of pruning (moby/moby#48488), which don't affect BuildKit vendoring.

Some linting fixes (moby/moby#48481) and a bugfix for seccomp on riscv64 (moby/moby#48463), which would affect BuildKit (but only riscv64)

All vendoring updates were because of the BuildKit update; moby/moby#48469

@thaJeztah
Copy link
Member Author

I had a look at the changes in buildkit;

The first commit shows the changes in docker and docker/cli code. Second commit contains dependency updates, which are not "enforced" because we don't have a go.mod.

  • Some golang.org/x/ versions are indeed ahead of BuildKit, because they were updated for rootlesskit, but not sure if the golang.org/x ones are problematic
  • We aldo updated containerd to get bugfixes, although the vendor dependency has no changes
  • ☝️ because we are not a module yet, go modules doesn't enforce those updates, so BuildKit could choose to skip those updates, or only pick the ones it wants

Also had a look at the buildx changes;

And this is the diff (I excluded the swagger-file and go.sum changes);

git diff -- .  ':(exclude)go.sum' ':(exclude)vendor/modules.txt' ':(exclude)vendor/github.com/docker/docker/api/swagger.yaml'
diff --git a/go.mod b/go.mod
index 24d06871..b0e0acf8 100644
--- a/go.mod
+++ b/go.mod
@@ -16,9 +16,9 @@ require (
 	github.com/containerd/typeurl/v2 v2.2.0
 	github.com/creack/pty v1.1.21
 	github.com/distribution/reference v0.6.0
-	github.com/docker/cli v27.2.1+incompatible
+	github.com/docker/cli v27.2.2-0.20240912223434-c85c3df6b5b7+incompatible // 27.x branch / v27.3.0-dev
 	github.com/docker/cli-docs-tool v0.8.0
-	github.com/docker/docker v27.2.1+incompatible
+	github.com/docker/docker v27.2.2-0.20240912213415-bf60e5cced83+incompatible // 27.x branch / v27.3.0-dev
 	github.com/docker/go-units v0.5.0
 	github.com/gofrs/flock v0.12.1
 	github.com/gogo/protobuf v1.3.2
diff --git a/vendor/github.com/docker/docker/api/types/filters/parse.go b/vendor/github.com/docker/docker/api/types/filters/parse.go
index 0c39ab5f..0914b2a4 100644
--- a/vendor/github.com/docker/docker/api/types/filters/parse.go
+++ b/vendor/github.com/docker/docker/api/types/filters/parse.go
@@ -196,7 +196,7 @@ func (args Args) Match(field, source string) bool {
 }
 
 // GetBoolOrDefault returns a boolean value of the key if the key is present
-// and is intepretable as a boolean value. Otherwise the default value is returned.
+// and is interpretable as a boolean value. Otherwise the default value is returned.
 // Error is not nil only if the filter values are not valid boolean or are conflicting.
 func (args Args) GetBoolOrDefault(key string, defaultValue bool) (bool, error) {
 	fieldValues, ok := args.fields[key]
diff --git a/vendor/github.com/docker/docker/api/types/image/summary.go b/vendor/github.com/docker/docker/api/types/image/summary.go
index c7168fe6..e87e216a 100644
--- a/vendor/github.com/docker/docker/api/types/image/summary.go
+++ b/vendor/github.com/docker/docker/api/types/image/summary.go
@@ -12,7 +12,7 @@ type Summary struct {
 	Containers int64 `json:"Containers"`
 
 	// Date and time at which the image was created as a Unix timestamp
-	// (number of seconds sinds EPOCH).
+	// (number of seconds since EPOCH).
 	//
 	// Required: true
 	Created int64 `json:"Created"`
diff --git a/vendor/github.com/docker/docker/api/types/swarm/swarm.go b/vendor/github.com/docker/docker/api/types/swarm/swarm.go
index 3eae4b9b..1b4be6ff 100644
--- a/vendor/github.com/docker/docker/api/types/swarm/swarm.go
+++ b/vendor/github.com/docker/docker/api/types/swarm/swarm.go
@@ -122,7 +122,7 @@ type CAConfig struct {
 	SigningCAKey  string `json:",omitempty"`
 
 	// If this value changes, and there is no specified signing cert and key,
-	// then the swarm is forced to generate a new root certificate ane key.
+	// then the swarm is forced to generate a new root certificate and key.
 	ForceRotate uint64 `json:",omitempty"`
 }
 
diff --git a/vendor/github.com/docker/docker/api/types/volume/cluster_volume.go b/vendor/github.com/docker/docker/api/types/volume/cluster_volume.go
index bbd9ff0b..618a4816 100644
--- a/vendor/github.com/docker/docker/api/types/volume/cluster_volume.go
+++ b/vendor/github.com/docker/docker/api/types/volume/cluster_volume.go
@@ -414,7 +414,7 @@ type Info struct {
 	// the Volume has not been successfully created yet.
 	VolumeID string `json:",omitempty"`
 
-	// AccessibleTopolgoy is the topology this volume is actually accessible
+	// AccessibleTopology is the topology this volume is actually accessible
 	// from.
 	AccessibleTopology []Topology `json:",omitempty"`
 }
diff --git a/vendor/github.com/docker/docker/pkg/jsonmessage/jsonmessage.go b/vendor/github.com/docker/docker/pkg/jsonmessage/jsonmessage.go
index 035160c8..8d2c8857 100644
--- a/vendor/github.com/docker/docker/pkg/jsonmessage/jsonmessage.go
+++ b/vendor/github.com/docker/docker/pkg/jsonmessage/jsonmessage.go
@@ -290,7 +290,7 @@ func DisplayJSONMessagesStream(in io.Reader, out io.Writer, terminalFd uintptr,
 }
 
 // Stream is an io.Writer for output with utilities to get the output's file
-// descriptor and to detect wether it's a terminal.
+// descriptor and to detect whether it's a terminal.
 //
 // it is subset of the streams.Out type in
 // https://pkg.go.dev/github.com/docker/cli@v20.10.17+incompatible/cli/streams#Out
diff --git a/vendor/github.com/docker/docker/pkg/pools/pools.go b/vendor/github.com/docker/docker/pkg/pools/pools.go
index 3792c67a..3ea3012b 100644
--- a/vendor/github.com/docker/docker/pkg/pools/pools.go
+++ b/vendor/github.com/docker/docker/pkg/pools/pools.go
@@ -124,7 +124,7 @@ func (bufPool *BufioWriterPool) Put(b *bufio.Writer) {
 }
 
 // NewWriteCloserWrapper returns a wrapper which puts the bufio.Writer back
-// into the pool and closes the writer if it's an io.Writecloser.
+// into the pool and closes the writer if it's an io.WriteCloser.
 func (bufPool *BufioWriterPool) NewWriteCloserWrapper(buf *bufio.Writer, w io.Writer) io.WriteCloser {
 	return ioutils.NewWriteCloserWrapper(w, func() error {
 		buf.Flush()
diff --git a/vendor/github.com/docker/docker/pkg/system/xattrs_linux.go b/vendor/github.com/docker/docker/pkg/system/xattrs_linux.go
index facfbb31..b877ecc5 100644
--- a/vendor/github.com/docker/docker/pkg/system/xattrs_linux.go
+++ b/vendor/github.com/docker/docker/pkg/system/xattrs_linux.go
@@ -6,7 +6,7 @@ import (
 
 // Lgetxattr retrieves the value of the extended attribute identified by attr
 // and associated with the given path in the file system.
-// It will returns a nil slice and nil error if the xattr is not set.
+// It returns a nil slice and nil error if the xattr is not set.
 func Lgetxattr(path string, attr string) ([]byte, error) {
 	sysErr := func(err error) ([]byte, error) {
 		return nil, &XattrError{Op: "lgetxattr", Attr: attr, Path: path, Err: err}
diff --git a/vendor/github.com/docker/docker/registry/config.go b/vendor/github.com/docker/docker/registry/config.go
index 84b0a63a..e1b0a0ca 100644
--- a/vendor/github.com/docker/docker/registry/config.go
+++ b/vendor/github.com/docker/docker/registry/config.go
@@ -359,7 +359,7 @@ func hasScheme(reposName string) bool {
 }
 
 func validateHostPort(s string) error {
-	// Split host and port, and in case s can not be splitted, assume host only
+	// Split host and port, and in case s can not be split, assume host only
 	host, port, err := net.SplitHostPort(s)
 	if err != nil {
 		host = s
---

@thaJeztah
Copy link
Member Author

TL;DR; there are no code changes except for typo-fixes in code-comments. The dependencies can be updated, but (because we're not a go module yet) could be skipped if you decide to.

@thaJeztah
Copy link
Member Author

Let me bring this one in

@thaJeztah thaJeztah merged commit 48a2cdf into docker:27.x Sep 13, 2024
103 of 108 checks passed
@thaJeztah thaJeztah deleted the 27.x_backport_wsl-socket-path branch September 13, 2024 08:54
@tonistiigi
Copy link
Member

I'm confused; what are the "lots of changes"?

Ok, seems that we can do without it. There are thousands of lines of changes, but because cli does not use go modules, nothing under vendor bumps automatically, and we can skip all these updates.

@thaJeztah
Copy link
Member Author

Right, but most of those 1000s lines of changes was because buildkit updates those, so we had to update as well😂

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

Successfully merging this pull request may close these issues.

6 participants