bump docker and dependencies

[thaJeztah]

Updates docker/docker to b711437bbd8596312c962d4189e9ad4d2108c2dc (from moby master)

ping @tonistiigi PTAL for the platform changes

[tonistiigi]

@thaJeztah platforms.Parse does not accept empty string as an option. It needs to be detected before and nil used as a value.

[thaJeztah]

oh, dang thought I tested it locally, lol. my bad

[thaJeztah]

Ok, looks like I'm still getting a failure; looking at "why";

=== RUN   TestBuildFromContextDirectoryWithTag
--- FAIL: TestBuildFromContextDirectoryWithTag (0.19s)
	build_test.go:32: assertion failed: 
		Command:  docker build -t myimage .
		ExitCode: 1
		Error:    exit status 1
		Stdout:   Sending build context to Docker daemon  4.608kB
		
		Stderr:   Error response from daemon: invalid platform: invalid platform os "unknown"
		
		
		Failures:
		ExitCode was 1 expected 0
		Expected stderr to contain "[NOTHING]"
		Expected no error
=== RUN   TestTrustedBuild
--- FAIL: TestTrustedBuild (2.88s)
	build_test.go:71: assertion failed: 
		Command:  docker build -t myimage .
		ExitCode: 1
		Error:    exit status 1
		Stdout:   Sending build context to Docker daemon  2.048kB
		
		Stderr:   Error response from daemon: invalid platform: invalid platform os "unknown"
		
		
		Failures:
		ExitCode was 1 expected 0
		Expected stdout to contain "FROM registry:5000/trust-build1@sha"
		Expected stderr to contain "Tagging registry:5000/trust-build1@sha"
		Expected no error

Note that that error message (invalid platform os "unknown") is no longer in Moby master, but used to be in LCOW: https://github.com/moby/moby/blob/35193c0e7dc301e1d2f6ea96e0ce34ffd2d4b88d/pkg/system/lcow.go#L23-L27

	if platform.OS != "" {
		if !(platform.OS == runtime.GOOS || (LCOWSupported() && platform.OS == "linux")) {
			return fmt.Errorf("invalid platform os %q", platform.OS)
		}
	}

t.b.h., no idea how we can hit that error if the daemon is running on Linux?

The "unknown" looks to come from https://github.com/containerd/containerd/blob/db3c5afc6d61dc3bd1ac48a55156e58bf4557a45/platforms/platforms.go#L233-L240;

// Format returns a string specifier from the provided platform specification.
func Format(platform specs.Platform) string {
	if platform.OS == "" {
		return "unknown"
	}

	return joinNotEmpty(platform.OS, platform.Architecture, platform.Variant)
}

I wonder if returning "unknown" is the correct approach there; should it just return an empty string?

The reason the "unknown" is used is by one of these; https://github.com/moby/moby/blob/a2a3b8fe9cb8a3c9afc11ce27eadebcdb8b40ee5/client/image_build.go#L33-L38 and https://github.com/moby/moby/blob/a2a3b8fe9cb8a3c9afc11ce27eadebcdb8b40ee5/client/image_build.go#L133-L135

I'd have to check but platform empty but not nil is likely my fault, still, I found the "unknown" as default a bit curious, and definitely not sure how we can hit the lcow code path 🤔

[thaJeztah]

Right, so need to change this, and don't set Platform if options.platform is empty

[tonistiigi]

@thaJeztah I think there are still issues with parsing the os/arch only platforms. containerd's platforms.Parse is specific to current os and that is not correct in the case of Docker CLI. Eg. building on a mac with platform=amd64 would set the platform as darwin/amd64. So I think we should revert the switch to *specs.Platform in ImageBuildOptions and keep it as string. Or we could update the Parse method, like the last item in containerd/containerd#2409 describes but that would take even longer.

[thaJeztah]

containerd's platforms.Parse is specific to current os and that is not correct in the case of Docker CLI

Hm, good point; was focussing on the other changes, but you're right 😞

We should also discuss the "unknown" return value; wondering if platform must be non-empty according to the spec; if not, an empty string would make more sense than a magic "unknown" string.

[thaJeztah]

@johnstep any idea how I was able to hit the lcow path on a Linux daemon?

[thaJeztah]

Ah, never mind the LCOW thing I see it; its if !(......); so that could be triggered if ! platform.OS ("unknown") == runtime.GOOS

[silvin-lubecki]

func runBuild(dockerCli command.Cli, options buildOptions) error really needs to be cut into pieces (290 LoC) 😅

[thaJeztah]

func runBuild(dockerCli command.Cli, options buildOptions) error really needs to be cut into pieces (290 LoC)

Yup. There was an attempt once, but it didn't make it a lot better (just splitting up the parts in that attempt only hid away complexity, and you still had to go to all the extracted functions to understand what happened)

[tiborvass]

Created moby/moby#37381

[tiborvass]

@thaJeztah updated this to match moby master. I suggest we open a similar one for 18.06 branch once docker-archive/engine#7 is merged WDYT?

[thaJeztah]

LGTM

[vdemeester]

SGTM 🐯

[silvin-lubecki]

LGTM 👍