Enhance default platform comparision for image shareablity #2810

[Shubhranshu153]

Partially Addresses: #2810

There are cases where --platform flag may be passed during nerdctl build. In such a situation if platform matches the runtime platform the image is shareable.

[pendo324]

Should we log this error?

Suggested change

	return false
	log.L.WithError(err).Errorf("failed to parse platform")
	return false

[Shubhranshu153]

i don't think its important to log here, as the Parse will be called elsewhere and be handled. Here it just important to figure out if the image is shareable, if any thing goes wrong it will attempt to create a non-shareable image.

[ktock]

Thanks. can we add a test for len(platform) == 1 case?

[Shubhranshu153]

Thanks. can we add a test for len(platform) == 1 case?

Will add a test case.

[Shubhranshu153]

@ktock
was thinking of adding a unit test file for build.go called build_test.go and add unit test for is_shreable() func. I dont see any other place where this can go. Let me know your thoughts.

[ktock]

Maybe we can add a test case here:

nerdctl/cmd/nerdctl/builder_build_test.go

Line 100 in 265d6b9

func TestBuildFromContainerd(t *testing.T) {

[Shubhranshu153]

Not sure about the test failures, some of the test seem flaky.

[ktock]

Test failed on CI: https://github.com/containerd/nerdctl/actions/runs/8026538828/job/21929197370?pr=2834#step:7:13

=== RUN   TestBuildIsShareableForCompatiblePlatform
    builder_build_test.go:86: assertion failed: 
        Command:  /usr/bin/docker build /tmp/nerdctl-build-test2104019243 -t nerdctl-testbuildisshareableforcompatibleplatform --platform linux/arm/v7 --progress plain
        ExitCode: 0
        Stdout:   
        Stderr:   #0 building with "default" instance using docker driver
        
        #1 [internal] load .dockerignore
        #1 transferring context: 2B done
        #1 DONE 0.0s
        
        #2 [internal] load build definition from Dockerfile
        #2 transferring dockerfile: 127B done
        #2 DONE 0.0s
        
        #3 [internal] load metadata for ghcr.io/stargz-containers/alpine:3.13-org
        #3 DONE 0.2s
        
        #4 [1/1] FROM ghcr.io/stargz-containers/alpine:3.13-org@sha256:ec14c7992a97fc11425907e908340c6c3d6ff602f5f13d899e6b7027c9b4133a
        #4 CACHED
        
        #5 exporting to image
        #5 exporting layers done
        #5 writing image sha256:6f5021027f826fc1f7353f081bd8ff6364f8db76b5ce718a9007e023faf033ab done
        #5 naming to docker.io/library/nerdctl-testbuildisshareableforcompatibleplatform done
        #5 DONE 0.0s
        
        
        Failures:
        Expected stderr to contain "tarball"
--- FAIL: TestBuildIsShareableForCompatiblePlatform (2.11s)

[Shubhranshu153]

Fixed the above docker failed test.

[ktock]

Thanks

[Shubhranshu153]

The lint error seems to be some sort of caching issue, is it possible to have no-cache option.
integ test not sure why it failed.

[AkihiroSuda]

  Running [/home/runner/golangci-lint-1.55.2-linux-amd64/golangci-lint run --out-format=github-actions --verbose] in [] ...
  Error: File is not `gofmt`-ed with `-s` (gofmt)

[AkihiroSuda]

Thanks

[apostasie]

Hey @Shubhranshu153

Looks like this is testing stdout (c.AssertOutWithFunc) and not stderr.
Can you clarify if the intention here was to test stderr?