From 7c81e16b8af4175859a4f79997d1ba9e9f6c23f0 Mon Sep 17 00:00:00 2001 From: Edgar Lee Date: Wed, 20 May 2020 18:48:09 -0700 Subject: [PATCH] Fix duplicate source maps and fix issue preventing multiple locations per source map Signed-off-by: Edgar Lee --- client/client_test.go | 27 +++++++++++------ client/llb/sourcemap.go | 41 +++++++++++++------------ client/llb/state_test.go | 48 ++++++++++++++++++++++++++++++ frontend/dockerfile/errors_test.go | 2 +- 4 files changed, 89 insertions(+), 29 deletions(-) diff --git a/client/client_test.go b/client/client_test.go index 7fa74105..2ff592a8 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -2697,6 +2697,7 @@ func testSourceMap(t *testing.T, sb integration.Sandbox) { llb.Shlex("not-exist"), sm1.Location([]*pb.Range{{Start: pb.Position{Line: 7}}}), sm2.Location([]*pb.Range{{Start: pb.Position{Line: 8}}}), + sm1.Location([]*pb.Range{{Start: pb.Position{Line: 9}}}), ) def, err := st.Marshal(context.TODO()) @@ -2706,27 +2707,35 @@ func testSourceMap(t *testing.T, sb integration.Sandbox) { require.Error(t, err) srcs := errdefs.Sources(err) - require.Equal(t, 2, len(srcs)) + require.Equal(t, 3, len(srcs)) // Source errors are wrapped in the order provided as llb.ConstraintOpts, so // when they are unwrapped, the first unwrapped error is the last location // provided. - require.Equal(t, "bar", srcs[0].Info.Filename) - require.Equal(t, []byte("data2"), srcs[0].Info.Data) + require.Equal(t, "foo", srcs[0].Info.Filename) + require.Equal(t, []byte("data1"), srcs[0].Info.Data) require.Nil(t, srcs[0].Info.Definition) - require.Equal(t, 1, len(srcs[0].Ranges)) - require.Equal(t, int32(8), srcs[0].Ranges[0].Start.Line) - require.Equal(t, int32(0), srcs[0].Ranges[0].Start.Character) + require.Equal(t, 1, len(srcs[2].Ranges)) + require.Equal(t, int32(9), srcs[2].Ranges[0].Start.Line) + require.Equal(t, int32(0), srcs[2].Ranges[0].Start.Character) - require.Equal(t, "foo", srcs[1].Info.Filename) - require.Equal(t, []byte("data1"), srcs[1].Info.Data) + require.Equal(t, "bar", srcs[1].Info.Filename) + require.Equal(t, []byte("data2"), srcs[1].Info.Data) require.Nil(t, srcs[1].Info.Definition) require.Equal(t, 1, len(srcs[1].Ranges)) - require.Equal(t, int32(7), srcs[1].Ranges[0].Start.Line) + require.Equal(t, int32(8), srcs[1].Ranges[0].Start.Line) require.Equal(t, int32(0), srcs[1].Ranges[0].Start.Character) + require.Equal(t, "foo", srcs[2].Info.Filename) + require.Equal(t, []byte("data1"), srcs[2].Info.Data) + require.Nil(t, srcs[2].Info.Definition) + + require.Equal(t, 1, len(srcs[2].Ranges)) + require.Equal(t, int32(7), srcs[2].Ranges[0].Start.Line) + require.Equal(t, int32(0), srcs[2].Ranges[0].Start.Character) + } func testSourceMapFromRef(t *testing.T, sb integration.Sandbox) { diff --git a/client/llb/sourcemap.go b/client/llb/sourcemap.go index ca180fb6..87afde99 100644 --- a/client/llb/sourcemap.go +++ b/client/llb/sourcemap.go @@ -40,14 +40,15 @@ type SourceLocation struct { } type sourceMapCollector struct { - ranges []map[digest.Digest][]*pb.Range - maps []*SourceMap - index map[*SourceMap]int + maps []*SourceMap + index map[*SourceMap]int + locations map[digest.Digest][]*SourceLocation } func newSourceMapCollector() *sourceMapCollector { return &sourceMapCollector{ - index: map[*SourceMap]int{}, + index: map[*SourceMap]int{}, + locations: map[digest.Digest][]*SourceLocation{}, } } @@ -57,17 +58,17 @@ func (smc *sourceMapCollector) Add(dgst digest.Digest, ls []*SourceLocation) { if !ok { idx = len(smc.maps) smc.maps = append(smc.maps, l.SourceMap) - smc.ranges = append(smc.ranges, map[digest.Digest][]*pb.Range{}) } - smc.ranges[idx][dgst] = l.Ranges + smc.index[l.SourceMap] = idx } + smc.locations[dgst] = ls } func (smc *sourceMapCollector) Marshal(ctx context.Context, co ...ConstraintsOpt) (*pb.Source, error) { s := &pb.Source{ Locations: make(map[string]*pb.Locations), } - for i, m := range smc.maps { + for _, m := range smc.maps { def := m.Definition if def == nil && m.State != nil { var err error @@ -88,20 +89,22 @@ func (smc *sourceMapCollector) Marshal(ctx context.Context, co ...ConstraintsOpt } s.Infos = append(s.Infos, info) + } - for dgst, ranges := range smc.ranges[i] { - locs, ok := s.Locations[dgst.String()] - if !ok { - locs = &pb.Locations{} - } - - locs.Locations = append(locs.Locations, &pb.Location{ - SourceIndex: int32(i), - Ranges: ranges, - }) - - s.Locations[dgst.String()] = locs + for dgst, locs := range smc.locations { + pbLocs, ok := s.Locations[dgst.String()] + if !ok { + pbLocs = &pb.Locations{} } + + for _, loc := range locs { + pbLocs.Locations = append(pbLocs.Locations, &pb.Location{ + SourceIndex: int32(smc.index[loc.SourceMap]), + Ranges: loc.Ranges, + }) + } + + s.Locations[dgst.String()] = pbLocs } return s, nil diff --git a/client/llb/state_test.go b/client/llb/state_test.go index b602db7c..06ebdedb 100644 --- a/client/llb/state_test.go +++ b/client/llb/state_test.go @@ -4,6 +4,8 @@ import ( "context" "testing" + "github.com/moby/buildkit/solver/pb" + digest "github.com/opencontainers/go-digest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -53,6 +55,52 @@ func TestFormattingPatterns(t *testing.T) { assert.Equal(t, "/foo/bar1", getDirHelper(t, s2)) } +func TestStateSourceMapMarshal(t *testing.T) { + t.Parallel() + + sm1 := NewSourceMap(nil, "foo", []byte("data1")) + sm2 := NewSourceMap(nil, "bar", []byte("data2")) + + s := Image( + "myimage", + sm1.Location([]*pb.Range{{Start: pb.Position{Line: 7}}}), + sm2.Location([]*pb.Range{{Start: pb.Position{Line: 8}}}), + sm1.Location([]*pb.Range{{Start: pb.Position{Line: 9}}}), + ) + + def, err := s.Marshal(context.TODO()) + require.NoError(t, err) + + require.Equal(t, 2, len(def.Def)) + dgst := digest.FromBytes(def.Def[0]) + + require.Equal(t, 2, len(def.Source.Infos)) + require.Equal(t, 1, len(def.Source.Locations)) + + require.Equal(t, "foo", def.Source.Infos[0].Filename) + require.Equal(t, []byte("data1"), def.Source.Infos[0].Data) + require.Nil(t, def.Source.Infos[0].Definition) + + require.Equal(t, "bar", def.Source.Infos[1].Filename) + require.Equal(t, []byte("data2"), def.Source.Infos[1].Data) + require.Nil(t, def.Source.Infos[1].Definition) + + require.NotNil(t, def.Source.Locations[dgst.String()]) + require.Equal(t, 3, len(def.Source.Locations[dgst.String()].Locations)) + + require.Equal(t, int32(0), def.Source.Locations[dgst.String()].Locations[0].SourceIndex) + require.Equal(t, 1, len(def.Source.Locations[dgst.String()].Locations[0].Ranges)) + require.Equal(t, int32(7), def.Source.Locations[dgst.String()].Locations[0].Ranges[0].Start.Line) + + require.Equal(t, int32(1), def.Source.Locations[dgst.String()].Locations[1].SourceIndex) + require.Equal(t, 1, len(def.Source.Locations[dgst.String()].Locations[1].Ranges)) + require.Equal(t, int32(8), def.Source.Locations[dgst.String()].Locations[1].Ranges[0].Start.Line) + + require.Equal(t, int32(0), def.Source.Locations[dgst.String()].Locations[2].SourceIndex) + require.Equal(t, 1, len(def.Source.Locations[dgst.String()].Locations[2].Ranges)) + require.Equal(t, int32(9), def.Source.Locations[dgst.String()].Locations[2].Ranges[0].Start.Line) +} + func getEnvHelper(t *testing.T, s State, k string) (string, bool) { t.Helper() v, ok, err := s.GetEnv(context.TODO(), k) diff --git a/frontend/dockerfile/errors_test.go b/frontend/dockerfile/errors_test.go index e1f3ca41..0fc67071 100644 --- a/frontend/dockerfile/errors_test.go +++ b/frontend/dockerfile/errors_test.go @@ -105,7 +105,7 @@ env bar=baz`, next: for _, l := range tc.errorLine { - for _, l2 := range srcs[0].Locations { + for _, l2 := range srcs[0].Ranges { if l2.Start.Line == int32(l) { continue next }