Skip to content

Commit 64d3b2a

Browse files
NaatanNaatan
authored
Merge pull request ActiveState#3508 from ActiveState/DX-3066
Map to ingredient when its source can be identified as a single straight path
2 parents 51af785 + b7b933f commit 64d3b2a

File tree

8 files changed

+186
-87
lines changed

8 files changed

+186
-87
lines changed

‎pkg/buildplan/buildplan.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,10 @@ func Unmarshal(data []byte) (*BuildPlan, error) {
3737
return nil, errs.Wrap(err, "error hydrating build plan")
3838
}
3939

40-
if len(b.artifacts) == 0 || len(b.ingredients) == 0 || len(b.platforms) == 0 {
41-
return nil, errs.New("Buildplan unmarshalling failed as it got zero artifacts (%d), ingredients (%d) and or platforms (%d).",
42-
len(b.artifacts), len(b.ingredients), len(b.platforms))
40+
if len(b.artifacts) == 0 || len(b.platforms) == 0 {
41+
// Ingredients are not considered here because certain builds (eg. camel) won't be able to relate to a single ingredient
42+
return nil, errs.New("Buildplan unmarshalling failed as it got zero artifacts (%d) and/or platforms (%d).",
43+
len(b.artifacts), len(b.platforms))
4344
}
4445

4546
return b, nil

‎pkg/buildplan/filters.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ func FilterStateArtifacts() FilterArtifact {
4747
internalIngredients := sliceutils.Filter(a.Ingredients, func(i *Ingredient) bool {
4848
return i.Namespace == NamespaceInternal
4949
})
50-
if len(a.Ingredients) == len(internalIngredients) {
50+
if len(a.Ingredients) > 0 && len(a.Ingredients) == len(internalIngredients) {
5151
return false
5252
}
5353
if strings.Contains(a.URL, "as-builds/noop") {

‎pkg/buildplan/hydrate.go‎

Lines changed: 37 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,9 @@ func (b *BuildPlan) hydrate() error {
5555
}
5656
ingredient, ok := ingredientLookup[source.IngredientID]
5757
if !ok {
58-
return errs.New("missing ingredient for source ID: %s", req.Source)
58+
// It's possible that we haven't associated a source to an artifact if that source links to multiple artifacts.
59+
// In this case we cannot determine which artifact relates to which source.
60+
continue
5961
}
6062
b.requirements = append(b.requirements, &Requirement{
6163
Requirement: req.Requirement,
@@ -80,7 +82,7 @@ func (b *BuildPlan) hydrate() error {
8082
}
8183

8284
func (b *BuildPlan) hydrateWithBuildClosure(nodeIDs []strfmt.UUID, platformID *strfmt.UUID, artifactLookup map[strfmt.UUID]*Artifact) error {
83-
err := b.raw.WalkViaSteps(nodeIDs, raw.TagDependency, func(node interface{}, parent *raw.Artifact) error {
85+
err := b.raw.WalkViaSteps(nodeIDs, raw.WalkViaDeps, func(node interface{}, parent *raw.Artifact) error {
8486
switch v := node.(type) {
8587
case *raw.Artifact:
8688
// logging.Debug("Walking build closure artifact '%s (%s)'", v.DisplayName, v.NodeID)
@@ -151,51 +153,44 @@ func (b *BuildPlan) hydrateWithRuntimeClosure(nodeIDs []strfmt.UUID, platformID
151153
}
152154

153155
func (b *BuildPlan) hydrateWithIngredients(artifact *Artifact, platformID *strfmt.UUID, ingredientLookup map[strfmt.UUID]*Ingredient) error {
154-
err := b.raw.WalkViaSteps([]strfmt.UUID{artifact.ArtifactID}, raw.TagSource,
156+
err := b.raw.WalkViaSteps([]strfmt.UUID{artifact.ArtifactID}, raw.WalkViaSingleSource,
155157
func(node interface{}, parent *raw.Artifact) error {
156-
switch v := node.(type) {
157-
case *raw.Source:
158-
// logging.Debug("Walking source '%s (%s)'", v.Name, v.NodeID)
159-
160-
// Ingredients aren't explicitly represented in buildplans. Technically all sources are ingredients
161-
// but this may not always be true in the future. For our purposes we will initialize our own ingredients
162-
// based on the source information, but we do not want to make the assumption in our logic that all
163-
// sources are ingredients.
164-
ingredient, ok := ingredientLookup[v.IngredientID]
165-
if !ok {
166-
ingredient = &Ingredient{
167-
IngredientSource: &v.IngredientSource,
168-
platforms: []strfmt.UUID{},
169-
Artifacts: []*Artifact{},
170-
}
171-
b.ingredients = append(b.ingredients, ingredient)
172-
ingredientLookup[v.IngredientID] = ingredient
173-
}
158+
// logging.Debug("Walking source '%s (%s)'", v.Name, v.NodeID)
159+
v, ok := node.(*raw.Source)
160+
if !ok {
161+
return nil // continue
162+
}
174163

175-
// With multiple terminals it's possible we encounter the same combination multiple times.
176-
// And an artifact usually only has one ingredient, so this is the cheapest lookup.
177-
if !sliceutils.Contains(artifact.Ingredients, ingredient) {
178-
artifact.Ingredients = append(artifact.Ingredients, ingredient)
179-
ingredient.Artifacts = append(ingredient.Artifacts, artifact)
180-
}
181-
if platformID != nil {
182-
ingredient.platforms = append(ingredient.platforms, *platformID)
164+
// Ingredients aren't explicitly represented in buildplans. Technically all sources are ingredients
165+
// but this may not always be true in the future. For our purposes we will initialize our own ingredients
166+
// based on the source information, but we do not want to make the assumption in our logic that all
167+
// sources are ingredients.
168+
ingredient, ok := ingredientLookup[v.IngredientID]
169+
if !ok {
170+
ingredient = &Ingredient{
171+
IngredientSource: &v.IngredientSource,
172+
platforms: []strfmt.UUID{},
173+
Artifacts: []*Artifact{},
183174
}
175+
b.ingredients = append(b.ingredients, ingredient)
176+
ingredientLookup[v.IngredientID] = ingredient
177+
}
184178

185-
if artifact.isBuildtimeDependency {
186-
ingredient.IsBuildtimeDependency = true
187-
}
188-
if artifact.isRuntimeDependency {
189-
ingredient.IsRuntimeDependency = true
190-
}
179+
// With multiple terminals it's possible we encounter the same combination multiple times.
180+
// And an artifact usually only has one ingredient, so this is the cheapest lookup.
181+
if !sliceutils.Contains(artifact.Ingredients, ingredient) {
182+
artifact.Ingredients = append(artifact.Ingredients, ingredient)
183+
ingredient.Artifacts = append(ingredient.Artifacts, artifact)
184+
}
185+
if platformID != nil {
186+
ingredient.platforms = append(ingredient.platforms, *platformID)
187+
}
191188

192-
return nil
193-
default:
194-
if a, ok := v.(*raw.Artifact); ok && a.NodeID == artifact.ArtifactID {
195-
return nil // continue
196-
}
197-
// Source ingredients are only relevant when they link DIRECTLY to the artifact
198-
return raw.WalkInterrupt{}
189+
if artifact.isBuildtimeDependency {
190+
ingredient.IsBuildtimeDependency = true
191+
}
192+
if artifact.isRuntimeDependency {
193+
ingredient.IsRuntimeDependency = true
199194
}
200195

201196
return nil
@@ -211,14 +206,6 @@ func (b *BuildPlan) hydrateWithIngredients(artifact *Artifact, platformID *strfm
211206
// If there are duplicates we're likely to see failures down the chain if live, though that's by no means guaranteed.
212207
// Surfacing it here will make it easier to reason about the failure.
213208
func (b *BuildPlan) sanityCheck() error {
214-
// Ensure all artifacts have an associated ingredient
215-
// If this fails either the API is bugged or the hydrate logic is bugged
216-
for _, a := range b.Artifacts() {
217-
if raw.IsStateToolMimeType(a.MimeType) && len(a.Ingredients) == 0 {
218-
return errs.New("artifact '%s (%s)' does not have an ingredient", a.ArtifactID, a.DisplayName)
219-
}
220-
}
221-
222209
// The remainder of sanity checks aren't checking for error conditions so much as they are checking for smoking guns
223210
// If these fail then it's likely the API has changed in a backward incompatible way, or we broke something.
224211
// In any case it does not necessarily mean runtime sourcing is broken.

‎pkg/buildplan/hydrate_test.go‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,27 +18,40 @@ func TestBuildPlan_hydrateWithIngredients(t *testing.T) {
1818
}{
1919
{
2020
"Ingredient solves for simple artifact > src hop",
21-
&BuildPlan{raw: mock.BuildWithRuntimeDepsViaSrc},
21+
&BuildPlan{raw: mock.BuildWithInstallerDepsViaSrc},
2222
&Artifact{ArtifactID: "00000000-0000-0000-0000-000000000007"},
2323
"00000000-0000-0000-0000-000000000009",
2424
},
2525
{
2626
"Installer should not resolve to an ingredient as it doesn't have a direct source",
27-
&BuildPlan{raw: mock.BuildWithRuntimeDepsViaSrc},
27+
&BuildPlan{raw: mock.BuildWithInstallerDepsViaSrc},
2828
&Artifact{ArtifactID: "00000000-0000-0000-0000-000000000002"},
2929
"",
3030
},
31+
{
32+
"State artifact should resolve to source even when hopping through a python wheel",
33+
&BuildPlan{raw: mock.BuildWithStateArtifactThroughPyWheel},
34+
&Artifact{ArtifactID: "00000000-0000-0000-0000-000000000002"},
35+
"00000000-0000-0000-0000-000000000009",
36+
},
3137
}
3238
for _, tt := range tests {
3339
t.Run(tt.name, func(t *testing.T) {
3440
b := tt.buildplan
3541
if err := b.hydrateWithIngredients(tt.inputArtifact, nil, map[strfmt.UUID]*Ingredient{}); err != nil {
3642
t.Fatalf("hydrateWithIngredients() error = %v", errs.JoinMessage(err))
3743
}
44+
45+
// Use string slice so testify doesn't just dump a bunch of pointer addresses on failure -.-
46+
ingredients := []string{}
47+
for _, i := range tt.inputArtifact.Ingredients {
48+
ingredients = append(ingredients, i.IngredientID.String())
49+
}
3850
if tt.wantIngredient == "" {
39-
require.Empty(t, tt.inputArtifact.Ingredients)
51+
require.Empty(t, ingredients)
4052
return
4153
}
54+
4255
if len(tt.inputArtifact.Ingredients) != 1 {
4356
t.Fatalf("expected 1 ingredient resolution, got %d", len(tt.inputArtifact.Ingredients))
4457
}

‎pkg/buildplan/mock/mock.go‎

Lines changed: 75 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,8 @@ var BuildWithRuntimeDeps = &raw.Build{
153153
},
154154
}
155155

156-
var BuildWithRuntimeDepsViaSrc = &raw.Build{
156+
// BuildWithInstallerDepsViaSrc is a build that includes an installer which has two artifacts as its dependencies.
157+
var BuildWithInstallerDepsViaSrc = &raw.Build{
157158
Terminals: []*raw.NamedTarget{
158159
{
159160
Tag: "platform:00000000-0000-0000-0000-000000000001",
@@ -165,7 +166,12 @@ var BuildWithRuntimeDepsViaSrc = &raw.Build{
165166
StepID: "00000000-0000-0000-0000-000000000003",
166167
Outputs: []string{"00000000-0000-0000-0000-000000000002"},
167168
Inputs: []*raw.NamedTarget{
168-
{Tag: "src", NodeIDs: []strfmt.UUID{"00000000-0000-0000-0000-000000000007"}},
169+
{
170+
Tag: "src", NodeIDs: []strfmt.UUID{
171+
"00000000-0000-0000-0000-000000000007",
172+
"00000000-0000-0000-0000-000000000010",
173+
},
174+
},
169175
},
170176
},
171177
{
@@ -175,6 +181,13 @@ var BuildWithRuntimeDepsViaSrc = &raw.Build{
175181
{Tag: "src", NodeIDs: []strfmt.UUID{"00000000-0000-0000-0000-000000000009"}},
176182
},
177183
},
184+
{
185+
StepID: "00000000-0000-0000-0000-000000000011",
186+
Outputs: []string{"00000000-0000-0000-0000-000000000010"},
187+
Inputs: []*raw.NamedTarget{
188+
{Tag: "src", NodeIDs: []strfmt.UUID{"00000000-0000-0000-0000-000000000012"}},
189+
},
190+
},
178191
},
179192
Artifacts: []*raw.Artifact{
180193
{
@@ -190,6 +203,66 @@ var BuildWithRuntimeDepsViaSrc = &raw.Build{
190203
MimeType: types.XActiveStateArtifactMimeType,
191204
GeneratedBy: "00000000-0000-0000-0000-000000000008",
192205
},
206+
{
207+
NodeID: "00000000-0000-0000-0000-000000000010",
208+
DisplayName: "pkgTwo",
209+
MimeType: types.XActiveStateArtifactMimeType,
210+
GeneratedBy: "00000000-0000-0000-0000-000000000011",
211+
},
212+
},
213+
Sources: []*raw.Source{
214+
{
215+
"00000000-0000-0000-0000-000000000009",
216+
raw.IngredientSource{
217+
IngredientID: "00000000-0000-0000-0000-000000000009",
218+
},
219+
},
220+
{
221+
"00000000-0000-0000-0000-000000000012",
222+
raw.IngredientSource{
223+
IngredientID: "00000000-0000-0000-0000-000000000012",
224+
},
225+
},
226+
},
227+
}
228+
229+
// BuildWithStateArtifactThroughPyWheel is a build with a state tool artifact that has a python wheel as its dependency
230+
var BuildWithStateArtifactThroughPyWheel = &raw.Build{
231+
Terminals: []*raw.NamedTarget{
232+
{
233+
Tag: "platform:00000000-0000-0000-0000-000000000001",
234+
NodeIDs: []strfmt.UUID{"00000000-0000-0000-0000-000000000002"},
235+
},
236+
},
237+
Steps: []*raw.Step{
238+
{
239+
StepID: "00000000-0000-0000-0000-000000000003",
240+
Outputs: []string{"00000000-0000-0000-0000-000000000002"},
241+
Inputs: []*raw.NamedTarget{
242+
{
243+
Tag: "src", NodeIDs: []strfmt.UUID{"00000000-0000-0000-0000-000000000007"},
244+
},
245+
},
246+
},
247+
{
248+
StepID: "00000000-0000-0000-0000-000000000008",
249+
Outputs: []string{"00000000-0000-0000-0000-000000000007"},
250+
Inputs: []*raw.NamedTarget{
251+
{Tag: "src", NodeIDs: []strfmt.UUID{"00000000-0000-0000-0000-000000000009"}},
252+
},
253+
},
254+
},
255+
Artifacts: []*raw.Artifact{
256+
{
257+
NodeID: "00000000-0000-0000-0000-000000000002",
258+
DisplayName: "pkgStateArtifact",
259+
GeneratedBy: "00000000-0000-0000-0000-000000000003",
260+
},
261+
{
262+
NodeID: "00000000-0000-0000-0000-000000000007",
263+
DisplayName: "pkgPyWheel",
264+
GeneratedBy: "00000000-0000-0000-0000-000000000008",
265+
},
193266
},
194267
Sources: []*raw.Source{
195268
{

‎pkg/buildplan/raw/walk.go‎

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
package raw
22

33
import (
4-
"errors"
5-
64
"github.com/go-openapi/strfmt"
75

86
"github.com/ActiveState/cli/internal/errs"
@@ -11,45 +9,49 @@ import (
119

1210
type walkFunc func(node interface{}, parent *Artifact) error
1311

14-
type WalkNodeContext struct {
15-
Node interface{}
16-
ParentArtifact *Artifact
17-
tag StepInputTag
18-
lookup map[strfmt.UUID]interface{}
12+
type WalkStrategy struct {
13+
tag StepInputTag
14+
stopAtMultiSource bool // If true, we will stop walking if the artifact relates to multiple sources (eg. installer, docker img)
1915
}
2016

21-
type WalkInterrupt struct{}
22-
23-
func (w WalkInterrupt) Error() string {
24-
return "interrupt walk"
25-
}
17+
var (
18+
WalkViaSingleSource = WalkStrategy{
19+
TagSource,
20+
true,
21+
}
22+
WalkViaMultiSource = WalkStrategy{
23+
TagSource,
24+
false,
25+
}
26+
WalkViaDeps = WalkStrategy{
27+
TagDependency,
28+
false,
29+
}
30+
)
2631

2732
// WalkViaSteps walks the graph and reports on nodes it encounters
2833
// Note that the same node can be encountered multiple times if it is referenced in the graph multiple times.
2934
// In this case the context around the node may be different, even if the node itself isn't.
30-
func (b *Build) WalkViaSteps(nodeIDs []strfmt.UUID, inputTag StepInputTag, walk walkFunc) error {
35+
func (b *Build) WalkViaSteps(nodeIDs []strfmt.UUID, strategy WalkStrategy, walk walkFunc) error {
3136
lookup := b.LookupMap()
3237

3338
for _, nodeID := range nodeIDs {
3439
node, ok := lookup[nodeID]
3540
if !ok {
3641
return errs.New("node ID '%s' does not exist in lookup table", nodeID)
3742
}
38-
if err := b.walkNodeViaSteps(node, nil, inputTag, walk); err != nil {
43+
if err := b.walkNodeViaSteps(node, nil, strategy, walk); err != nil {
3944
return errs.Wrap(err, "could not recurse over node IDs")
4045
}
4146
}
4247

4348
return nil
4449
}
4550

46-
func (b *Build) walkNodeViaSteps(node interface{}, parent *Artifact, tag StepInputTag, walk walkFunc) error {
51+
func (b *Build) walkNodeViaSteps(node interface{}, parent *Artifact, strategy WalkStrategy, walk walkFunc) error {
4752
lookup := b.LookupMap()
4853

4954
if err := walk(node, parent); err != nil {
50-
if errors.As(err, &WalkInterrupt{}) {
51-
return nil
52-
}
5355
return errs.Wrap(err, "error walking over node")
5456
}
5557

@@ -74,24 +76,29 @@ func (b *Build) walkNodeViaSteps(node interface{}, parent *Artifact, tag StepInp
7476
// tool, but it's technically possible to happen if someone requested a builder as part of their order.
7577
_, isSource = generatedByNode.(*Source)
7678
if isSource {
77-
if err := b.walkNodeViaSteps(generatedByNode, ar, tag, walk); err != nil {
79+
if err := b.walkNodeViaSteps(generatedByNode, ar, strategy, walk); err != nil {
7880
return errs.Wrap(err, "error walking source from generatedBy")
7981
}
8082
return nil // Sources are at the end of the recursion.
8183
}
8284

83-
nodeIDs, err := b.inputNodeIDsFromStep(ar, tag)
85+
nodeIDs, err := b.inputNodeIDsFromStep(ar, strategy.tag)
8486
if err != nil {
8587
return errs.Wrap(err, "error walking over step inputs")
8688
}
8789

90+
// Stop if the next step has multiple input node ID's; this means we cannot determine a single source
91+
if strategy.stopAtMultiSource && len(nodeIDs) > 1 {
92+
return nil
93+
}
94+
8895
for _, id := range nodeIDs {
8996
// Grab subnode that we want to iterate over next
9097
subNode, ok := lookup[id]
9198
if !ok {
9299
return errs.New("node ID '%s' does not exist in lookup table", id)
93100
}
94-
if err := b.walkNodeViaSteps(subNode, ar, tag, walk); err != nil {
101+
if err := b.walkNodeViaSteps(subNode, ar, strategy, walk); err != nil {
95102
return errs.Wrap(err, "error iterating over %s", id)
96103
}
97104
}

0 commit comments

Comments
 (0)