Builder: Review feedback

Signed-off-by: John Howard <jhoward@microsoft.com>

builder/dockerfile/dispatchers.go

@@ -152,7 +152,10 @@ func (d *dispatchRequest) getImageMount(imageRefOrID string) (*imageMount, error
 //
 func initializeStage(d dispatchRequest, cmd *instructions.Stage) error {
 	d.builder.imageProber.Reset()
-	image, err := d.getFromImage(d.shlex, cmd.BaseName, cmd.OperatingSystem)
+	if err := system.ValidatePlatform(&cmd.Platform); err != nil {
+		return err
+	}
+	image, err := d.getFromImage(d.shlex, cmd.BaseName, cmd.Platform.OS)
 	if err != nil {
 		return err
 	}
@@ -215,32 +218,29 @@ func (d *dispatchRequest) getExpandedImageName(shlex *shell.Lex, name string) (s
 // stagePlatform contains the value supplied by optional `--platform=` on
 // a current FROM statement. b.builder.options.Platform contains the operating
 // system part of the optional flag passed in the API call (or CLI flag
-// through `docker build --platform=...`).
+// through `docker build --platform=...`). Precedence is for an explicit
-func (d *dispatchRequest) getOsFromFlagsAndStage(stagePlatform string) string {
+// platform indication in the FROM statement.
-	osForPull := ""
+func (d *dispatchRequest) getOsFromFlagsAndStage(stageOS string) string {
-	// First, take the API platform if nothing provided on FROM
+	switch {
-	if stagePlatform == "" && d.builder.options.Platform != "" {
+	case stageOS != "":
-		osForPull = d.builder.options.Platform
+		return stageOS
+	case d.builder.options.Platform != "":
+		// Note this is API "platform", but by this point, as the daemon is not
+		// multi-arch aware yet, it is guaranteed to only hold the OS part here.
+		return d.builder.options.Platform
+	default:
+		return runtime.GOOS
 	}
-	// Next, use the FROM flag if that was provided
-	if osForPull == "" && stagePlatform != "" {
-		osForPull = stagePlatform
-	}
-	// Finally, assume the host OS
-	if osForPull == "" {
-		osForPull = runtime.GOOS
-	}
-	return osForPull
 }
 
-func (d *dispatchRequest) getImageOrStage(name string, stagePlatform string) (builder.Image, error) {
+func (d *dispatchRequest) getImageOrStage(name string, stageOS string) (builder.Image, error) {
 	var localOnly bool
 	if im, ok := d.stages.getByName(name); ok {
 		name = im.Image
 		localOnly = true
 	}
 
-	os := d.getOsFromFlagsAndStage(stagePlatform)
+	os := d.getOsFromFlagsAndStage(stageOS)
 
 	// Windows cannot support a container with no base image unless it is LCOW.
 	if name == api.NoBaseImageSpecifier {
@@ -267,12 +267,12 @@ func (d *dispatchRequest) getImageOrStage(name string, stagePlatform string) (bu
 	}
 	return imageMount.Image(), nil
 }
-func (d *dispatchRequest) getFromImage(shlex *shell.Lex, name string, stagePlatform string) (builder.Image, error) {
+func (d *dispatchRequest) getFromImage(shlex *shell.Lex, name string, stageOS string) (builder.Image, error) {
 	name, err := d.getExpandedImageName(shlex, name)
 	if err != nil {
 		return nil, err
 	}
-	return d.getImageOrStage(name, stagePlatform)
+	return d.getImageOrStage(name, stageOS)
 }
 
 func dispatchOnbuild(d dispatchRequest, c *instructions.OnbuildCommand) error {

builder/dockerfile/instructions/commands.go

@@ -7,6 +7,7 @@ import (
 
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/api/types/strslice"
+	specs "github.com/opencontainers/image-spec/specs-go/v1"
 )
 
 // KeyValuePair represent an arbitrary named value (useful in slice instead of map[string] string to preserve ordering)
@@ -357,11 +358,11 @@ type ShellCommand struct {
 
 // Stage represents a single stage in a multi-stage build
 type Stage struct {
-	Name            string
+	Name       string
-	Commands        []Command
+	Commands   []Command
-	BaseName        string
+	BaseName   string
-	SourceCode      string
+	SourceCode string
-	OperatingSystem string
+	Platform   specs.Platform
 }
 
 // AddCommand to the stage

builder/dockerfile/instructions/parse.go

@@ -276,23 +276,13 @@ func parseFrom(req parseRequest) (*Stage, error) {
 	if err := req.flags.Parse(); err != nil {
 		return nil, err
 	}
-	specPlatform := system.ParsePlatform(flPlatform.Value)
-	if err := system.ValidatePlatform(specPlatform); err != nil {
-		return nil, fmt.Errorf("invalid platform %q on FROM", flPlatform.Value)
-	}
-	if specPlatform.OS != "" && !system.IsOSSupported(specPlatform.OS) {
-		return nil, fmt.Errorf("unsupported platform %q on FROM", flPlatform.Value)
-	}
-	if err != nil {
-		return nil, err
-	}
 	code := strings.TrimSpace(req.original)
 	return &Stage{
-		BaseName:        req.args[0],
+		BaseName:   req.args[0],
-		Name:            stageName,
+		Name:       stageName,
-		SourceCode:      code,
+		SourceCode: code,
-		Commands:        []Command{},
+		Commands:   []Command{},
-		OperatingSystem: specPlatform.OS,
+		Platform:   *system.ParsePlatform(flPlatform.Value),
 	}, nil
 
 }

builder/dockerfile/instructions/parse_test.go

@@ -1,8 +1,6 @@
 package instructions // import "github.com/docker/docker/builder/dockerfile/instructions"
 
 import (
-	"fmt"
-	"runtime"
 	"strings"
 	"testing"
 
@@ -186,16 +184,6 @@ func TestErrorCases(t *testing.T) {
 			dockerfile:    `foo bar`,
 			expectedError: "unknown instruction: FOO",
 		},
-		{
-			name:          "Invalid platform",
-			dockerfile:    `FROM --platform=invalid busybox`,
-			expectedError: `invalid platform "invalid"`,
-		},
-		{
-			name:          "Only OS",
-			dockerfile:    fmt.Sprintf(`FROM --platform=%s/%s busybox`, runtime.GOOS, runtime.GOARCH),
-			expectedError: `invalid platform`,
-		},
 	}
 	for _, c := range cases {
 		r := strings.NewReader(c.dockerfile)