From bc52d40edda952edd95c53bf49fc9f467067c578 Mon Sep 17 00:00:00 2001 From: caihaomin Date: Tue, 8 Dec 2020 15:42:48 +0800 Subject: [PATCH] isula-build:fix printing FROM command double times to console Signed-off-by: caihaomin --- VERSION-openeuler | 2 +- git-commit | 2 +- isula-build.spec | 5 +- ...FROM-command-double-times-to-console.patch | 434 ++++++++++++++++++ series.conf | 1 + 5 files changed, 441 insertions(+), 3 deletions(-) create mode 100644 patch/0074-fix-printing-FROM-command-double-times-to-console.patch diff --git a/VERSION-openeuler b/VERSION-openeuler index f98c31e..61830a6 100644 --- a/VERSION-openeuler +++ b/VERSION-openeuler @@ -1 +1 @@ -0.9.4-11 +0.9.4-12 diff --git a/git-commit b/git-commit index 1285887..71fea19 100644 --- a/git-commit +++ b/git-commit @@ -1 +1 @@ -7def8f00bd20c7be542ff86a6c3009eed8cb6149 +2c3d247b4b89071ce93669e85c47ac846820c1c6 diff --git a/isula-build.spec b/isula-build.spec index 9141ec6..fc02cbf 100644 --- a/isula-build.spec +++ b/isula-build.spec @@ -2,7 +2,7 @@ Name: isula-build Version: 0.9.4 -Release: 11 +Release: 12 Summary: A tool to build container images License: Mulan PSL V2 URL: https://gitee.com/openeuler/isula-build @@ -85,6 +85,9 @@ fi /usr/share/bash-completion/completions/isula-build %changelog +* Tue Dec 08 2020 caihaomin - 0.9.4-12 +- Fix printing FROM command double times to console + * Tue Dec 08 2020 caihaomin - 0.9.4-11 - Fix problems found by code review diff --git a/patch/0074-fix-printing-FROM-command-double-times-to-console.patch b/patch/0074-fix-printing-FROM-command-double-times-to-console.patch new file mode 100644 index 0000000..95fbde8 --- /dev/null +++ b/patch/0074-fix-printing-FROM-command-double-times-to-console.patch @@ -0,0 +1,434 @@ +From f93ea5cf3ca956943711bbf0d4d86e330f248534 Mon Sep 17 00:00:00 2001 +From: xingweizheng 00591739 +Date: Mon, 30 Nov 2020 17:13:15 +0800 +Subject: [PATCH 4/4] fix printing FROM command double times to console + +--- + builder/dockerfile/builder_test.go | 16 ++--- + builder/dockerfile/cmd_builder_test.go | 41 ++++++------- + builder/dockerfile/parser/parser.go | 12 ++-- + builder/dockerfile/parser/parser_test.go | 61 ++++++++++++++++++- + .../preprocess/busybox_line_with_spaces | 0 + .../testfiles/preprocess/busybox_no_command | 2 +- + .../preprocess/busybox_ubuntu_centos | 3 + + .../preprocess/busybox_with_from_only | 1 + + .../compelte_stage_with_single_from_stage | 3 + + ...single_from_stage_depend_on_previous_stage | 3 + + .../final_stage_depend_on_previous_stage | 3 + + .../single_from_stage_with_complete_stage | 3 + + builder/dockerfile/stage_builder.go | 12 +++- + 13 files changed, 122 insertions(+), 38 deletions(-) + mode change 100755 => 100644 builder/dockerfile/parser/testfiles/preprocess/busybox_line_with_spaces + create mode 100755 builder/dockerfile/parser/testfiles/preprocess/busybox_ubuntu_centos + create mode 100755 builder/dockerfile/parser/testfiles/preprocess/busybox_with_from_only + create mode 100755 builder/dockerfile/parser/testfiles/preprocess/compelte_stage_with_single_from_stage + create mode 100755 builder/dockerfile/parser/testfiles/preprocess/final_single_from_stage_depend_on_previous_stage + create mode 100755 builder/dockerfile/parser/testfiles/preprocess/final_stage_depend_on_previous_stage + create mode 100755 builder/dockerfile/parser/testfiles/preprocess/single_from_stage_with_complete_stage + +diff --git a/builder/dockerfile/builder_test.go b/builder/dockerfile/builder_test.go +index b02768ea..94842c71 100644 +--- a/builder/dockerfile/builder_test.go ++++ b/builder/dockerfile/builder_test.go +@@ -248,13 +248,13 @@ RUN ls + } + + // check the arg and env taken by the command: RUN ls +- assert.DeepEqual(t, b.stageBuilders[0].commands[1].args, ++ assert.DeepEqual(t, b.stageBuilders[0].commands[0].args, + map[string]string{"no_proxy": "10.0.0.0"}) +- assert.DeepEqual(t, b.stageBuilders[1].commands[2].args, ++ assert.DeepEqual(t, b.stageBuilders[1].commands[1].args, + map[string]string{"testArg": "0.1", "no_proxy": "10.0.0.0"}) +- assert.DeepEqual(t, b.stageBuilders[2].commands[3].args, ++ assert.DeepEqual(t, b.stageBuilders[2].commands[2].args, + map[string]string{"no_proxy": "10.0.0.0"}) +- assert.DeepEqual(t, b.stageBuilders[2].commands[3].envs, ++ assert.DeepEqual(t, b.stageBuilders[2].commands[2].envs, + map[string]string{"testArg": "1.0"}) + } + +@@ -291,13 +291,13 @@ RUN ls + } + + // check the arg and env taken by the command: RUN ls +- assert.DeepEqual(t, b.stageBuilders[0].commands[1].args, ++ assert.DeepEqual(t, b.stageBuilders[0].commands[0].args, + map[string]string{"HTTPS_PROXY": "127.0.0.1"}) +- assert.DeepEqual(t, b.stageBuilders[1].commands[2].args, ++ assert.DeepEqual(t, b.stageBuilders[1].commands[1].args, + map[string]string{"testArg": "0.1", "HTTPS_PROXY": "127.0.0.1"}) +- assert.DeepEqual(t, b.stageBuilders[2].commands[3].args, ++ assert.DeepEqual(t, b.stageBuilders[2].commands[2].args, + map[string]string{"HTTPS_PROXY": "127.0.0.1"}) +- assert.DeepEqual(t, b.stageBuilders[2].commands[3].envs, ++ assert.DeepEqual(t, b.stageBuilders[2].commands[2].envs, + map[string]string{"testArg": "1.0"}) + } + +diff --git a/builder/dockerfile/cmd_builder_test.go b/builder/dockerfile/cmd_builder_test.go +index df1c08f9..471314ba 100644 +--- a/builder/dockerfile/cmd_builder_test.go ++++ b/builder/dockerfile/cmd_builder_test.go +@@ -113,7 +113,7 @@ func TestExecuteHealthCheck(t *testing.T) { + } + err := s.analyzeStage(context.Background()) + assert.NilError(t, err) +- if err := s.commands[1].cmdExecutor(); (err != nil) != tt.wantErr { ++ if err := s.commands[0].cmdExecutor(); (err != nil) != tt.wantErr { + t.Errorf("CmdExecutor() error: %v, wantErr: %v", err, tt.wantErr) + } + tt.funcCheck(t, s) +@@ -195,7 +195,7 @@ CMD [""]`, + } + err := s.analyzeStage(context.Background()) + assert.NilError(t, err) +- if err := s.commands[1].cmdExecutor(); (err != nil) != tt.wantErr { ++ if err := s.commands[0].cmdExecutor(); (err != nil) != tt.wantErr { + t.Errorf("cmdExecutor() error: %v, wantErr: %v", err, tt.wantErr) + } + tt.funcCheck(t, s) +@@ -288,7 +288,7 @@ SHELL ["/bin/bash", "-c"]`, + } + err := s.analyzeStage(context.Background()) + assert.NilError(t, err) +- if err := s.commands[1].cmdExecutor(); (err != nil) != tt.wantErr { ++ if err := s.commands[0].cmdExecutor(); (err != nil) != tt.wantErr { + t.Errorf("SHELL cmdExecutor() error: %v, wantErr: %v", err, tt.wantErr) + } + tt.funcCheck(t, s) +@@ -316,15 +316,15 @@ CMD ls` + err := s.analyzeStage(context.Background()) + assert.NilError(t, err) + +- if err := s.commands[1].cmdExecutor(); err != nil { ++ if err := s.commands[0].cmdExecutor(); err != nil { + t.Errorf("CMD cmdExecutor() error: %v", err) + } + assert.DeepEqual(t, s.docker.Config.Cmd, strslice.StrSlice{"/bin/sh", "-c", "ls"}) + +- if err := s.commands[2].cmdExecutor(); err != nil { ++ if err := s.commands[1].cmdExecutor(); err != nil { + t.Errorf("SHELL cmdExecutor() error: %v", err) + } +- if err := s.commands[3].cmdExecutor(); err != nil { ++ if err := s.commands[2].cmdExecutor(); err != nil { + t.Errorf("CMD cmdExecutor() error: %v", err) + } + assert.DeepEqual(t, s.shellForm, strslice.StrSlice{"/bin/bash", "-c"}) +@@ -360,10 +360,9 @@ func TestExecuteNoop(t *testing.T) { + + // the "STEP 1: FROM alpine" in production is done at stageBuilder.prepare() + // no cmdExecutor for FROM, so no print for FROM here +- expectedString := `STEP 1: FROM alpine +-STEP 2: ARG testArg +-STEP 3: ENV env1=env2 +-STEP 4: ONBUILD CMD ls ++ expectedString := `STEP 1: ARG testArg ++STEP 2: ENV env1=env2 ++STEP 3: ONBUILD CMD ls + ` + assert.Equal(t, stepPrints, expectedString) + } +@@ -442,7 +441,7 @@ ENTRYPOINT [""]`, + } + err := s.analyzeStage(context.Background()) + assert.NilError(t, err) +- if err := s.commands[1].cmdExecutor(); (err != nil) != tt.wantErr { ++ if err := s.commands[0].cmdExecutor(); (err != nil) != tt.wantErr { + t.Errorf("cmdExecutor() error: %v, wantErr: %v", err, tt.wantErr) + } + tt.funcCheck(t, s) +@@ -904,15 +903,15 @@ WORKDIR /c` + } + err := s.analyzeStage(context.Background()) + assert.NilError(t, err) +- if err := s.commands[1].cmdExecutor(); err != nil { ++ if err := s.commands[0].cmdExecutor(); err != nil { + t.Errorf("WORKDIR cmdExecutor() error: %v", err) + } + assert.DeepEqual(t, s.docker.Config.WorkingDir, "/a") +- if err := s.commands[2].cmdExecutor(); err != nil { ++ if err := s.commands[1].cmdExecutor(); err != nil { + t.Errorf("WORKDIR cmdExecutor() error: %v", err) + } + assert.DeepEqual(t, s.docker.Config.WorkingDir, "/b") +- if err := s.commands[3].cmdExecutor(); err != nil { ++ if err := s.commands[2].cmdExecutor(); err != nil { + t.Errorf("WORKDIR cmdExecutor() error: %v", err) + } + assert.DeepEqual(t, s.docker.Config.WorkingDir, "/c") +@@ -939,15 +938,15 @@ WORKDIR c` + } + err := s.analyzeStage(context.Background()) + assert.NilError(t, err) +- if err := s.commands[1].cmdExecutor(); err != nil { ++ if err := s.commands[0].cmdExecutor(); err != nil { + t.Errorf("WORKDIR cmdExecutor() error: %v", err) + } + assert.DeepEqual(t, s.docker.Config.WorkingDir, "/a") +- if err := s.commands[2].cmdExecutor(); err != nil { ++ if err := s.commands[1].cmdExecutor(); err != nil { + t.Errorf("WORKDIR cmdExecutor() error: %v", err) + } + assert.DeepEqual(t, s.docker.Config.WorkingDir, "/a/b") +- if err := s.commands[3].cmdExecutor(); err != nil { ++ if err := s.commands[2].cmdExecutor(); err != nil { + t.Errorf("WORKDIR cmdExecutor() error: %v", err) + } + assert.DeepEqual(t, s.docker.Config.WorkingDir, "/a/b/c") +@@ -974,15 +973,15 @@ WORKDIR $DIRPATH/$DIRNAME` + } + err := s.analyzeStage(context.Background()) + assert.NilError(t, err) +- if err = s.commands[1].cmdExecutor(); err != nil { ++ if err = s.commands[0].cmdExecutor(); err != nil { + t.Errorf("WORKDIR cmdExecutor() error: %v", err) + } + assert.NilError(t, err) +- if err = s.commands[2].cmdExecutor(); err != nil { ++ if err = s.commands[1].cmdExecutor(); err != nil { + t.Errorf("WORKDIR cmdExecutor() error: %v", err) + } + assert.NilError(t, err) +- if err = s.commands[3].cmdExecutor(); err != nil { ++ if err = s.commands[2].cmdExecutor(); err != nil { + t.Errorf("WORKDIR cmdExecutor() error: %v", err) + } + assert.NilError(t, err) +@@ -1026,7 +1025,7 @@ Maintainer iSula iSula@huawei.com`, + } + err := s.analyzeStage(context.Background()) + assert.NilError(t, err) +- if err := s.commands[1].cmdExecutor(); (err != nil) != tt.wantErr { ++ if err := s.commands[0].cmdExecutor(); (err != nil) != tt.wantErr { + t.Errorf("cmdExecutor() error: %v, wantErr: %v", err, tt.wantErr) + } + tt.funcCheck(t, s) +diff --git a/builder/dockerfile/parser/parser.go b/builder/dockerfile/parser/parser.go +index 82835e67..821e18ea 100644 +--- a/builder/dockerfile/parser/parser.go ++++ b/builder/dockerfile/parser/parser.go +@@ -238,19 +238,21 @@ func constructPages(lines []*parser.Line, onbuild bool) ([]*parser.Page, error) + } + pageMap[page.Name] = page + // if the base image for current stage is from the previous stage, +- // mark the previous stage need to commit +- if from, ok := pageMap[line.Cells[0].Value]; ok { ++ // mark the previous stage need to commit, for only from command we don't commit ++ if from, ok := pageMap[line.Cells[0].Value]; ok && len(from.Lines) > 1 { + from.NeedCommit = true + } + currentPage = page + } + // because a valid dockerfile is always start with 'FROM' command here, so no need +- // to check currentPage wheather is nil ++ // to check whether currentPage is nil or not + currentPage.End = line.End + currentPage.AddLine(line) + } +- // the last stage always need to commit +- currentPage.NeedCommit = true ++ // the last stage always need to commit except page that contains only from command ++ if len(currentPage.Lines) > 1 { ++ currentPage.NeedCommit = true ++ } + pages = append(pages, currentPage) + + if len(pages) == 0 { +diff --git a/builder/dockerfile/parser/parser_test.go b/builder/dockerfile/parser/parser_test.go +index fe27dd95..8580b84c 100644 +--- a/builder/dockerfile/parser/parser_test.go ++++ b/builder/dockerfile/parser/parser_test.go +@@ -119,7 +119,6 @@ func TestFormatWithSpacesAfterEscapeToken(t *testing.T) { + name: "busybox_line_with_spaces", + expect: []int{12, 20, 96, 87, 10}, + }, +- + } + + for _, tc := range testcases { +@@ -194,8 +193,10 @@ func TestParse(t *testing.T) { + r, err := os.Open(file) + assert.NilError(t, err) + defer r.Close() ++ + df := dockerfile{} + _, err = df.Parse(r, false) ++ + if !tc.isErr { + assert.NilError(t, err, file) + } else { +@@ -205,6 +206,64 @@ func TestParse(t *testing.T) { + } + } + ++func TestParseContainSingleFrom(t *testing.T) { ++ testcases := []struct { ++ name string ++ isErr bool ++ committed bool ++ }{ ++ { ++ name: "busybox_with_from_only", ++ isErr: false, ++ committed: false, ++ }, { ++ name: "busybox_ubuntu_centos", ++ isErr: false, ++ committed: false, ++ }, { ++ name: "compelte_stage_with_single_from_stage", ++ isErr: false, ++ committed: false, ++ }, { ++ name: "single_from_stage_with_complete_stage", ++ isErr: false, ++ committed: true, ++ }, { ++ name: "final_single_from_stage_depend_on_previous_stage", ++ isErr: false, ++ committed: true, ++ }, { ++ name: "final_stage_depend_on_previous_stage", ++ isErr: false, ++ committed: true, ++ }, ++ } ++ ++ for _, tc := range testcases { ++ t.Run(tc.name, func(t *testing.T) { ++ file := filepath.Join("testfiles", "preprocess", tc.name) ++ r, err := os.Open(file) ++ assert.NilError(t, err) ++ defer r.Close() ++ ++ df := dockerfile{} ++ playbook := &parser.PlayBook{} ++ playbook, err = df.Parse(r, false) ++ ++ if !tc.isErr { ++ assert.NilError(t, err, file) ++ if tc.committed { ++ needCommit := false ++ for _, page := range playbook.Pages { ++ needCommit = page.NeedCommit || needCommit ++ } ++ assert.Equal(t, needCommit, true) ++ } ++ } ++ }) ++ } ++} ++ + func TestParseIgnore(t *testing.T) { + dockerignore := ` + # comment +diff --git a/builder/dockerfile/parser/testfiles/preprocess/busybox_line_with_spaces b/builder/dockerfile/parser/testfiles/preprocess/busybox_line_with_spaces +old mode 100755 +new mode 100644 +diff --git a/builder/dockerfile/parser/testfiles/preprocess/busybox_no_command b/builder/dockerfile/parser/testfiles/preprocess/busybox_no_command +index da465805..a424e7e0 100644 +--- a/builder/dockerfile/parser/testfiles/preprocess/busybox_no_command ++++ b/builder/dockerfile/parser/testfiles/preprocess/busybox_no_command +@@ -6,5 +6,5 @@ ENTRYPOINT ["sh"] + RUN ["ls"] + RUN echo "hello world" + +-# fail at here ++# Support single FROM command at here + FROM busybox +diff --git a/builder/dockerfile/parser/testfiles/preprocess/busybox_ubuntu_centos b/builder/dockerfile/parser/testfiles/preprocess/busybox_ubuntu_centos +new file mode 100755 +index 00000000..57f870ad +--- /dev/null ++++ b/builder/dockerfile/parser/testfiles/preprocess/busybox_ubuntu_centos +@@ -0,0 +1,3 @@ ++FROM busybox ++FROM ubuntu ++FROM centos +\ No newline at end of file +diff --git a/builder/dockerfile/parser/testfiles/preprocess/busybox_with_from_only b/builder/dockerfile/parser/testfiles/preprocess/busybox_with_from_only +new file mode 100755 +index 00000000..84662517 +--- /dev/null ++++ b/builder/dockerfile/parser/testfiles/preprocess/busybox_with_from_only +@@ -0,0 +1 @@ ++FROM busybox +\ No newline at end of file +diff --git a/builder/dockerfile/parser/testfiles/preprocess/compelte_stage_with_single_from_stage b/builder/dockerfile/parser/testfiles/preprocess/compelte_stage_with_single_from_stage +new file mode 100755 +index 00000000..636a75df +--- /dev/null ++++ b/builder/dockerfile/parser/testfiles/preprocess/compelte_stage_with_single_from_stage +@@ -0,0 +1,3 @@ ++FROM busybox ++RUN touch /tmp/a_test.txt ++FROM ubuntu +\ No newline at end of file +diff --git a/builder/dockerfile/parser/testfiles/preprocess/final_single_from_stage_depend_on_previous_stage b/builder/dockerfile/parser/testfiles/preprocess/final_single_from_stage_depend_on_previous_stage +new file mode 100755 +index 00000000..9493cbc0 +--- /dev/null ++++ b/builder/dockerfile/parser/testfiles/preprocess/final_single_from_stage_depend_on_previous_stage +@@ -0,0 +1,3 @@ ++FROM busybox as new_busybox ++RUN touch /tmp/a_test.txt ++FROM new_busybox +\ No newline at end of file +diff --git a/builder/dockerfile/parser/testfiles/preprocess/final_stage_depend_on_previous_stage b/builder/dockerfile/parser/testfiles/preprocess/final_stage_depend_on_previous_stage +new file mode 100755 +index 00000000..43bde170 +--- /dev/null ++++ b/builder/dockerfile/parser/testfiles/preprocess/final_stage_depend_on_previous_stage +@@ -0,0 +1,3 @@ ++FROM busybox as new_busybox ++FROM new_busybox ++RUN touch /tmp/a_test.txt +\ No newline at end of file +diff --git a/builder/dockerfile/parser/testfiles/preprocess/single_from_stage_with_complete_stage b/builder/dockerfile/parser/testfiles/preprocess/single_from_stage_with_complete_stage +new file mode 100755 +index 00000000..b4cf1190 +--- /dev/null ++++ b/builder/dockerfile/parser/testfiles/preprocess/single_from_stage_with_complete_stage +@@ -0,0 +1,3 @@ ++FROM ubuntu ++FROM busybox ++RUN touch /tmp/a_test.txt +\ No newline at end of file +diff --git a/builder/dockerfile/stage_builder.go b/builder/dockerfile/stage_builder.go +index 47f55bf1..23f488cb 100644 +--- a/builder/dockerfile/stage_builder.go ++++ b/builder/dockerfile/stage_builder.go +@@ -172,7 +172,9 @@ func (s *stageBuilder) analyzeStage(ctx context.Context) error { + cb := newCmdBuilder(ctx, line, s, stageArgs, stageEnvs) + + switch line.Command { ++ // From cmd is already pre-processed, we just pass it + case dockerfile.From: ++ continue + case dockerfile.Arg: + if cb.args, err = analyzeArg(s.builder, line, stageArgs, stageEnvs); err != nil { + return err +@@ -209,10 +211,16 @@ func (s *stageBuilder) stageBuild(ctx context.Context) (string, error) { + + // 3. commit for new image if needed + if s.rawStage.NeedCommit { +- s.imageID, err = s.commit(ctx) ++ if s.imageID, err = s.commit(ctx); err != nil { ++ return s.imageID, errors.Wrapf(err, "commit image for stage %s failed", s.name) ++ } ++ } ++ // for only from command in Dockerfile, there is no imageID committed, use fromImageID ++ if s.imageID == "" { ++ s.imageID = s.fromImageID + } + +- return s.imageID, errors.Wrapf(err, "commit image for stage %s failed", s.name) ++ return s.imageID, nil + } + + func prepareImage(opt *image.PrepareImageOptions) (*image.Describe, error) { +-- +2.27.0 + diff --git a/series.conf b/series.conf index cf6d35f..80c0d1b 100644 --- a/series.conf +++ b/series.conf @@ -11,3 +11,4 @@ patch/0070-hack-add-compile-flag-ftrapv.patch patch/0071-imporve-daemon-push-and-pull-unit-test.patch patch/0072-fuzz-add-more-fuzz-tests.patch patch/0073-cleancode-fix-problems-found-by-code-review.patch +patch/0074-fix-printing-FROM-command-double-times-to-console.patch