Repository: uber/makisu
Pull request #331 (2020-05-02 21:03:28)

Add `--target` option to build command
Maybe this Closes https://github.com/uber/makisu/issues/262

But I saw https://github.com/uber/makisu/issues/280 and don't really know why makisu should implement stages as tree ?

yiranwang52: Tree is better for calculating cache IDs.
Currently stages inherit cache ID from last step of previous stage - which is not necessary if no files are copied between them.

Patch:
diff --git a/bin/makisu/cmd/build.go b/bin/makisu/cmd/build.go
index 9870449..7ff0473 100644
--- a/bin/makisu/cmd/build.go
+++ b/bin/makisu/cmd/build.go
@@ -46,6 +46,7 @@ type buildCmd struct {
 	registryConfig string
 	destination    string
 
+	target        string
 	buildArgs     []string
 	allowModifyFS bool
 	commit        string
@@ -103,6 +104,7 @@ func getBuildCmd() *buildCmd {
 	buildCmd.PersistentFlags().StringVar(&buildCmd.registryConfig, "registry-config", "", "Set build-time variables")
 	buildCmd.PersistentFlags().StringVar(&buildCmd.destination, "dest", "", "Destination of the image tar")
 
+	buildCmd.PersistentFlags().StringVar(&buildCmd.target, "target", "", "Set the target build stage to build.")
 	buildCmd.PersistentFlags().StringArrayVar(&buildCmd.buildArgs, "build-arg", nil, "Argument to the dockerfile as per the spec of ARG. Format is \"--build-arg <arg>=<value>\"")
 	buildCmd.PersistentFlags().BoolVar(&buildCmd.allowModifyFS, "modifyfs", false, "Allow makisu to modify files outside of its internal storage dir")
 	buildCmd.PersistentFlags().StringVar(&buildCmd.commit, "commit", "implicit", "Set to explicit to only commit at steps with '#!COMMIT' annotations; Set to implicit to commit at every ADD/COPY/RUN step")
@@ -207,7 +209,7 @@ func (cmd *buildCmd) newBuildPlan(
 
 	// Create BuildPlan and validate it.
 	return builder.NewBuildPlan(
-		buildContext, imageName, replicas, cacheMgr, dockerfile, cmd.allowModifyFS, forceCommit)
+		buildContext, imageName, replicas, cacheMgr, dockerfile, cmd.allowModifyFS, forceCommit, cmd.target)
 }
 
 // Build image from the specified dockerfile.
diff --git a/lib/builder/build_plan.go b/lib/builder/build_plan.go
index 6c77e2c..d022be3 100644
--- a/lib/builder/build_plan.go
+++ b/lib/builder/build_plan.go
@@ -45,6 +45,8 @@ type BuildPlan struct {
 
 	// stages contains the build stages defined in dockerfile.
 	stages []*buildStage
+	// Which stage is the target for this plan
+	stageTarget string
 
 	// TODO: this is not used for now.
 	// Aliases of stages.
@@ -63,7 +65,7 @@ type BuildPlan struct {
 // returns a new BuildPlan.
 func NewBuildPlan(
 	ctx *context.BuildContext, target image.Name, replicas []image.Name, cacheMgr cache.Manager,
-	parsedStages []*dockerfile.Stage, allowModifyFS, forceCommit bool) (*BuildPlan, error) {
+	parsedStages []*dockerfile.Stage, allowModifyFS, forceCommit bool, stageTarget string) (*BuildPlan, error) {
 
 	plan := &BuildPlan{
 		baseCtx:           ctx,
@@ -72,6 +74,7 @@ func NewBuildPlan(
 		replicas:          replicas,
 		cacheMgr:          cacheMgr,
 		stages:            make([]*buildStage, 0),
+		stageTarget:       stageTarget,
 		stageAliases:      make(map[string]struct{}),
 		stageIndexAliases: make(map[string]*buildStage),
 		opts: &buildPlanOptions{
@@ -156,6 +159,13 @@ func (plan *BuildPlan) processStagesAndAliases(
 		// scratch before executing a stage.
 		seedCacheID = stage.nodes[len(stage.nodes)-1].CacheID()
 	}
+	plan.stageAliases = existingAliases
+
+	if plan.stageTarget != "" {
+		if _, ok := plan.stageAliases[plan.stageTarget]; !ok {
+			return fmt.Errorf("target stage not found in dockerfile %s", plan.stageTarget)
+		}
+	}
 
 	return nil
 }
@@ -188,6 +198,11 @@ func (plan *BuildPlan) Execute() (*image.DistributionManifest, error) {
 		for k, v := range orignalEnv {
 			os.Setenv(k, v)
 		}
+
+		if plan.stageTarget != "" && currStage.alias == plan.stageTarget {
+			log.Info("Finished building target stage")
+			break
+		}
 	}
 
 	// Wait for cache layers to be pushed. This will make them available to


Test patch:
diff --git a/lib/builder/build_plan_test.go b/lib/builder/build_plan_test.go
index 2b88858..9d7df87 100644
--- a/lib/builder/build_plan_test.go
+++ b/lib/builder/build_plan_test.go
@@ -49,7 +49,7 @@ func TestBuildPlanExecution(t *testing.T) {
 	}
 	stages := []*dockerfile.Stage{{from, directives}}
 
-	plan, err := NewBuildPlan(ctx, target, nil, cacheMgr, stages, true, false)
+	plan, err := NewBuildPlan(ctx, target, nil, cacheMgr, stages, true, false, "")
 	require.NoError(err)
 
 	manifest, err := plan.Execute()
@@ -93,7 +93,7 @@ func TestBuildPlanContextDirs(t *testing.T) {
 	// Here we need to set the allowModifyFS to true because we copy
 	// files across stages.
 	// TODO(pourchet): support copy --from without relying on FS.
-	plan, err := NewBuildPlan(ctx, target, nil, cacheMgr, stages, true, false)
+	plan, err := NewBuildPlan(ctx, target, nil, cacheMgr, stages, true, false, "")
 	require.NoError(err)
 	require.Contains(plan.copyFromDirs, "stage1")
 	require.Len(plan.copyFromDirs, 1)
@@ -108,7 +108,7 @@ func TestBuildPlanContextDirs(t *testing.T) {
 	}
 	stages = []*dockerfile.Stage{{from, directives}}
 
-	_, err = NewBuildPlan(ctx, target, nil, cacheMgr, stages, false, false)
+	_, err = NewBuildPlan(ctx, target, nil, cacheMgr, stages, false, false, "")
 	require.Error(err)
 
 	// Copy from subsequent stage.
@@ -119,7 +119,7 @@ func TestBuildPlanContextDirs(t *testing.T) {
 	from2 = dockerfile.FromDirectiveFixture("", envImage.String(), "stage2")
 	stages = []*dockerfile.Stage{{from1, directives1}, {from2, nil}}
 
-	_, err = NewBuildPlan(ctx, target, nil, cacheMgr, stages, false, false)
+	_, err = NewBuildPlan(ctx, target, nil, cacheMgr, stages, false, false, "")
 	require.Error(err)
 }
 
@@ -142,7 +142,7 @@ func TestBuildPlanBadRun(t *testing.T) {
 	}
 	stages := []*dockerfile.Stage{{from, directives}}
 
-	plan, err := NewBuildPlan(ctx, target, nil, cacheMgr, stages, true, false)
+	plan, err := NewBuildPlan(ctx, target, nil, cacheMgr, stages, true, false, "")
 	require.NoError(err)
 
 	_, err = plan.Execute()
@@ -166,7 +166,7 @@ func TestDuplicateStageAlias(t *testing.T) {
 	from2 := dockerfile.FromDirectiveFixture("", envImage.String(), "alias")
 	stages := []*dockerfile.Stage{{from1, nil}, {from2, nil}}
 
-	_, err = NewBuildPlan(ctx, target, nil, cacheMgr, stages, false, false)
+	_, err = NewBuildPlan(ctx, target, nil, cacheMgr, stages, false, false, "")
 	require.Error(err)
 
 	// Same image different alias.
@@ -174,6 +174,49 @@ func TestDuplicateStageAlias(t *testing.T) {
 	from2 = dockerfile.FromDirectiveFixture("", envImage.String(), "alias2")
 	stages = []*dockerfile.Stage{{from1, nil}, {from2, nil}}
 
-	_, err = NewBuildPlan(ctx, target, nil, cacheMgr, stages, false, false)
+	_, err = NewBuildPlan(ctx, target, nil, cacheMgr, stages, false, false, "")
+	require.NoError(err)
+}
+
+func TestTargetStageMissing(t *testing.T) {
+	require := require.New(t)
+
+	ctx, cleanup := context.BuildContextFixture()
+	defer cleanup()
+
+	target := image.NewImageName("", "testrepo", "testtag")
+	envImage, err := image.ParseName("scratch")
+	require.NoError(err)
+
+	cacheMgr := cache.New(ctx.ImageStore, nil, registry.NoopClientFixture())
+
+	// Same image same alias.
+	from1 := dockerfile.FromDirectiveFixture("", envImage.String(), "alias1")
+	from2 := dockerfile.FromDirectiveFixture("", envImage.String(), "alias2")
+	stages := []*dockerfile.Stage{{from1, nil}, {from2, nil}}
+
+	_, err = NewBuildPlan(ctx, target, nil, cacheMgr, stages, false, false, "alias3")
+	require.Error(err)
+}
+
+func TestTargetStageOk(t *testing.T) {
+	require := require.New(t)
+
+	ctx, cleanup := context.BuildContextFixture()
+	defer cleanup()
+
+	target := image.NewImageName("", "testrepo", "testtag")
+	envImage, err := image.ParseName("scratch")
+	require.NoError(err)
+
+	cacheMgr := cache.New(ctx.ImageStore, nil, registry.NoopClientFixture())
+
+	// Same image same alias.
+	from1 := dockerfile.FromDirectiveFixture("", envImage.String(), "alias1")
+	from2 := dockerfile.FromDirectiveFixture("", envImage.String(), "alias2")
+	from3 := dockerfile.FromDirectiveFixture("", envImage.String(), "alias3")
+	stages := []*dockerfile.Stage{{from1, nil}, {from2, nil}, {from3, nil}}
+
+	_, err = NewBuildPlan(ctx, target, nil, cacheMgr, stages, false, false, "alias2")
 	require.NoError(err)
 }
diff --git a/test/python/test_build.py b/test/python/test_build.py
index 9b5ced5..35e70a2 100644
--- a/test/python/test_build.py
+++ b/test/python/test_build.py
@@ -300,3 +300,13 @@ def test_build_global_arg(registry1, storage_dir):
         registry=registry1.addr, docker_args=docker_build_args)
     code, err = docker_run_image(registry1.addr, new_image)
     assert code == 0, err
+
+def test_build_target(registry1, storage_dir):
+    new_image = new_image_name()
+    context_dir = os.path.join(
+        os.getcwd(), 'testdata/build-context/target')
+    makisu_build_image(
+        new_image, context_dir, storage_dir,
+        registry=registry1.addr, target="second")
+    code, err = docker_run_image(registry1.addr, new_image)
+    assert code == 0, err
diff --git a/test/python/utils.py b/test/python/utils.py
index abc84d1..36b3384 100644
--- a/test/python/utils.py
+++ b/test/python/utils.py
@@ -108,7 +108,7 @@ def makisu_run_cmd(volumes, args):
 def makisu_build_image(
         new_image_tag, context_dir, storage_dir, cache_dir=None, volumes=None,
         docker_args=None, load=False, registry=None, replicas=None,
-        registry_cfg=None):
+        registry_cfg=None, target=None):
 
     volumes = volumes or {}
     volumes[storage_dir] = storage_dir  # Sandbox and file store
@@ -144,6 +144,9 @@ def makisu_build_image(
     if not cache_dir:
         args.extend(['--local-cache-ttl', '0s'])
 
+    if target:
+        args.extend(['--target', target])
+
     args.append('/context')
 
     exit_code = makisu_run_cmd(volumes, args)
diff --git a/testdata/build-context/target/Dockerfile b/testdata/build-context/target/Dockerfile
new file mode 100644
index 0000000..aac0b22
--- /dev/null
+++ b/testdata/build-context/target/Dockerfile
@@ -0,0 +1,11 @@
+FROM alpine
+
+CMD echo "This is not the correct one, with target"; exit 1;
+
+FROM alpine:latest as second
+
+CMD echo "This is the correct one, with target"; exit 0;
+
+FROM alpine:latest as third
+
+CMD echo "This is not the correct one, with target"; exit 1;