diff --git a/pkg/buildplan/buildplan.go b/pkg/buildplan/buildplan.go index 83f0ee353e..302d7df633 100644 --- a/pkg/buildplan/buildplan.go +++ b/pkg/buildplan/buildplan.go @@ -37,9 +37,10 @@ func Unmarshal(data []byte) (*BuildPlan, error) { return nil, errs.Wrap(err, "error hydrating build plan") } - if len(b.artifacts) == 0 || len(b.ingredients) == 0 || len(b.platforms) == 0 { - return nil, errs.New("Buildplan unmarshalling failed as it got zero artifacts (%d), ingredients (%d) and or platforms (%d).", - len(b.artifacts), len(b.ingredients), len(b.platforms)) + if len(b.artifacts) == 0 || len(b.platforms) == 0 { + // Ingredients are not considered here because certain builds (eg. camel) won't be able to relate to a single ingredient + return nil, errs.New("Buildplan unmarshalling failed as it got zero artifacts (%d) and/or platforms (%d).", + len(b.artifacts), len(b.platforms)) } return b, nil diff --git a/pkg/buildplan/filters.go b/pkg/buildplan/filters.go index 0c3bf6dbc1..b68fa0e93c 100644 --- a/pkg/buildplan/filters.go +++ b/pkg/buildplan/filters.go @@ -47,7 +47,7 @@ func FilterStateArtifacts() FilterArtifact { internalIngredients := sliceutils.Filter(a.Ingredients, func(i *Ingredient) bool { return i.Namespace == NamespaceInternal }) - if len(a.Ingredients) == len(internalIngredients) { + if len(a.Ingredients) > 0 && len(a.Ingredients) == len(internalIngredients) { return false } if strings.Contains(a.URL, "as-builds/noop") { diff --git a/pkg/buildplan/hydrate.go b/pkg/buildplan/hydrate.go index fcb7e40110..05f84302b9 100644 --- a/pkg/buildplan/hydrate.go +++ b/pkg/buildplan/hydrate.go @@ -55,7 +55,9 @@ func (b *BuildPlan) hydrate() error { } ingredient, ok := ingredientLookup[source.IngredientID] if !ok { - return errs.New("missing ingredient for source ID: %s", req.Source) + // It's possible that we haven't associated a source to an artifact if that source links to multiple artifacts. + // In this case we cannot determine which artifact relates to which source. + continue } b.requirements = append(b.requirements, &Requirement{ Requirement: req.Requirement, @@ -80,7 +82,7 @@ func (b *BuildPlan) hydrate() error { } func (b *BuildPlan) hydrateWithBuildClosure(nodeIDs []strfmt.UUID, platformID *strfmt.UUID, artifactLookup map[strfmt.UUID]*Artifact) error { - err := b.raw.WalkViaSteps(nodeIDs, raw.TagDependency, func(node interface{}, parent *raw.Artifact) error { + err := b.raw.WalkViaSteps(nodeIDs, raw.WalkViaDeps, func(node interface{}, parent *raw.Artifact) error { switch v := node.(type) { case *raw.Artifact: // logging.Debug("Walking build closure artifact '%s (%s)'", v.DisplayName, v.NodeID) @@ -151,51 +153,44 @@ func (b *BuildPlan) hydrateWithRuntimeClosure(nodeIDs []strfmt.UUID, platformID } func (b *BuildPlan) hydrateWithIngredients(artifact *Artifact, platformID *strfmt.UUID, ingredientLookup map[strfmt.UUID]*Ingredient) error { - err := b.raw.WalkViaSteps([]strfmt.UUID{artifact.ArtifactID}, raw.TagSource, + err := b.raw.WalkViaSteps([]strfmt.UUID{artifact.ArtifactID}, raw.WalkViaSingleSource, func(node interface{}, parent *raw.Artifact) error { - switch v := node.(type) { - case *raw.Source: - // logging.Debug("Walking source '%s (%s)'", v.Name, v.NodeID) - - // Ingredients aren't explicitly represented in buildplans. Technically all sources are ingredients - // but this may not always be true in the future. For our purposes we will initialize our own ingredients - // based on the source information, but we do not want to make the assumption in our logic that all - // sources are ingredients. - ingredient, ok := ingredientLookup[v.IngredientID] - if !ok { - ingredient = &Ingredient{ - IngredientSource: &v.IngredientSource, - platforms: []strfmt.UUID{}, - Artifacts: []*Artifact{}, - } - b.ingredients = append(b.ingredients, ingredient) - ingredientLookup[v.IngredientID] = ingredient - } + // logging.Debug("Walking source '%s (%s)'", v.Name, v.NodeID) + v, ok := node.(*raw.Source) + if !ok { + return nil // continue + } - // With multiple terminals it's possible we encounter the same combination multiple times. - // And an artifact usually only has one ingredient, so this is the cheapest lookup. - if !sliceutils.Contains(artifact.Ingredients, ingredient) { - artifact.Ingredients = append(artifact.Ingredients, ingredient) - ingredient.Artifacts = append(ingredient.Artifacts, artifact) - } - if platformID != nil { - ingredient.platforms = append(ingredient.platforms, *platformID) + // Ingredients aren't explicitly represented in buildplans. Technically all sources are ingredients + // but this may not always be true in the future. For our purposes we will initialize our own ingredients + // based on the source information, but we do not want to make the assumption in our logic that all + // sources are ingredients. + ingredient, ok := ingredientLookup[v.IngredientID] + if !ok { + ingredient = &Ingredient{ + IngredientSource: &v.IngredientSource, + platforms: []strfmt.UUID{}, + Artifacts: []*Artifact{}, } + b.ingredients = append(b.ingredients, ingredient) + ingredientLookup[v.IngredientID] = ingredient + } - if artifact.IsBuildtimeDependency { - ingredient.IsBuildtimeDependency = true - } - if artifact.IsRuntimeDependency { - ingredient.IsRuntimeDependency = true - } + // With multiple terminals it's possible we encounter the same combination multiple times. + // And an artifact usually only has one ingredient, so this is the cheapest lookup. + if !sliceutils.Contains(artifact.Ingredients, ingredient) { + artifact.Ingredients = append(artifact.Ingredients, ingredient) + ingredient.Artifacts = append(ingredient.Artifacts, artifact) + } + if platformID != nil { + ingredient.platforms = append(ingredient.platforms, *platformID) + } - return nil - default: - if a, ok := v.(*raw.Artifact); ok && a.NodeID == artifact.ArtifactID { - return nil // continue - } - // Source ingredients are only relevant when they link DIRECTLY to the artifact - return raw.WalkInterrupt{} + if artifact.IsBuildtimeDependency { + ingredient.IsBuildtimeDependency = true + } + if artifact.IsRuntimeDependency { + ingredient.IsRuntimeDependency = true } return nil @@ -211,14 +206,6 @@ func (b *BuildPlan) hydrateWithIngredients(artifact *Artifact, platformID *strfm // If there are duplicates we're likely to see failures down the chain if live, though that's by no means guaranteed. // Surfacing it here will make it easier to reason about the failure. func (b *BuildPlan) sanityCheck() error { - // Ensure all artifacts have an associated ingredient - // If this fails either the API is bugged or the hydrate logic is bugged - for _, a := range b.Artifacts() { - if raw.IsStateToolMimeType(a.MimeType) && len(a.Ingredients) == 0 { - return errs.New("artifact '%s (%s)' does not have an ingredient", a.ArtifactID, a.DisplayName) - } - } - // The remainder of sanity checks aren't checking for error conditions so much as they are checking for smoking guns // If these fail then it's likely the API has changed in a backward incompatible way, or we broke something. // In any case it does not necessarily mean runtime sourcing is broken. diff --git a/pkg/buildplan/hydrate_test.go b/pkg/buildplan/hydrate_test.go index da95c9826e..9ca9ba42bb 100644 --- a/pkg/buildplan/hydrate_test.go +++ b/pkg/buildplan/hydrate_test.go @@ -18,16 +18,22 @@ func TestBuildPlan_hydrateWithIngredients(t *testing.T) { }{ { "Ingredient solves for simple artifact > src hop", - &BuildPlan{raw: mock.BuildWithRuntimeDepsViaSrc}, + &BuildPlan{raw: mock.BuildWithInstallerDepsViaSrc}, &Artifact{ArtifactID: "00000000-0000-0000-0000-000000000007"}, "00000000-0000-0000-0000-000000000009", }, { "Installer should not resolve to an ingredient as it doesn't have a direct source", - &BuildPlan{raw: mock.BuildWithRuntimeDepsViaSrc}, + &BuildPlan{raw: mock.BuildWithInstallerDepsViaSrc}, &Artifact{ArtifactID: "00000000-0000-0000-0000-000000000002"}, "", }, + { + "State artifact should resolve to source even when hopping through a python wheel", + &BuildPlan{raw: mock.BuildWithStateArtifactThroughPyWheel}, + &Artifact{ArtifactID: "00000000-0000-0000-0000-000000000002"}, + "00000000-0000-0000-0000-000000000009", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -35,10 +41,17 @@ func TestBuildPlan_hydrateWithIngredients(t *testing.T) { if err := b.hydrateWithIngredients(tt.inputArtifact, nil, map[strfmt.UUID]*Ingredient{}); err != nil { t.Fatalf("hydrateWithIngredients() error = %v", errs.JoinMessage(err)) } + + // Use string slice so testify doesn't just dump a bunch of pointer addresses on failure -.- + ingredients := []string{} + for _, i := range tt.inputArtifact.Ingredients { + ingredients = append(ingredients, i.IngredientID.String()) + } if tt.wantIngredient == "" { - require.Empty(t, tt.inputArtifact.Ingredients) + require.Empty(t, ingredients) return } + if len(tt.inputArtifact.Ingredients) != 1 { t.Fatalf("expected 1 ingredient resolution, got %d", len(tt.inputArtifact.Ingredients)) } diff --git a/pkg/buildplan/mock/mock.go b/pkg/buildplan/mock/mock.go index aaf242ec7a..ed575535ee 100644 --- a/pkg/buildplan/mock/mock.go +++ b/pkg/buildplan/mock/mock.go @@ -153,7 +153,8 @@ var BuildWithRuntimeDeps = &raw.Build{ }, } -var BuildWithRuntimeDepsViaSrc = &raw.Build{ +// BuildWithInstallerDepsViaSrc is a build that includes an installer which has two artifacts as its dependencies. +var BuildWithInstallerDepsViaSrc = &raw.Build{ Terminals: []*raw.NamedTarget{ { Tag: "platform:00000000-0000-0000-0000-000000000001", @@ -165,7 +166,12 @@ var BuildWithRuntimeDepsViaSrc = &raw.Build{ StepID: "00000000-0000-0000-0000-000000000003", Outputs: []string{"00000000-0000-0000-0000-000000000002"}, Inputs: []*raw.NamedTarget{ - {Tag: "src", NodeIDs: []strfmt.UUID{"00000000-0000-0000-0000-000000000007"}}, + { + Tag: "src", NodeIDs: []strfmt.UUID{ + "00000000-0000-0000-0000-000000000007", + "00000000-0000-0000-0000-000000000010", + }, + }, }, }, { @@ -175,6 +181,13 @@ var BuildWithRuntimeDepsViaSrc = &raw.Build{ {Tag: "src", NodeIDs: []strfmt.UUID{"00000000-0000-0000-0000-000000000009"}}, }, }, + { + StepID: "00000000-0000-0000-0000-000000000011", + Outputs: []string{"00000000-0000-0000-0000-000000000010"}, + Inputs: []*raw.NamedTarget{ + {Tag: "src", NodeIDs: []strfmt.UUID{"00000000-0000-0000-0000-000000000012"}}, + }, + }, }, Artifacts: []*raw.Artifact{ { @@ -190,6 +203,66 @@ var BuildWithRuntimeDepsViaSrc = &raw.Build{ MimeType: types.XActiveStateArtifactMimeType, GeneratedBy: "00000000-0000-0000-0000-000000000008", }, + { + NodeID: "00000000-0000-0000-0000-000000000010", + DisplayName: "pkgTwo", + MimeType: types.XActiveStateArtifactMimeType, + GeneratedBy: "00000000-0000-0000-0000-000000000011", + }, + }, + Sources: []*raw.Source{ + { + "00000000-0000-0000-0000-000000000009", + raw.IngredientSource{ + IngredientID: "00000000-0000-0000-0000-000000000009", + }, + }, + { + "00000000-0000-0000-0000-000000000012", + raw.IngredientSource{ + IngredientID: "00000000-0000-0000-0000-000000000012", + }, + }, + }, +} + +// BuildWithStateArtifactThroughPyWheel is a build with a state tool artifact that has a python wheel as its dependency +var BuildWithStateArtifactThroughPyWheel = &raw.Build{ + Terminals: []*raw.NamedTarget{ + { + Tag: "platform:00000000-0000-0000-0000-000000000001", + NodeIDs: []strfmt.UUID{"00000000-0000-0000-0000-000000000002"}, + }, + }, + Steps: []*raw.Step{ + { + StepID: "00000000-0000-0000-0000-000000000003", + Outputs: []string{"00000000-0000-0000-0000-000000000002"}, + Inputs: []*raw.NamedTarget{ + { + Tag: "src", NodeIDs: []strfmt.UUID{"00000000-0000-0000-0000-000000000007"}, + }, + }, + }, + { + StepID: "00000000-0000-0000-0000-000000000008", + Outputs: []string{"00000000-0000-0000-0000-000000000007"}, + Inputs: []*raw.NamedTarget{ + {Tag: "src", NodeIDs: []strfmt.UUID{"00000000-0000-0000-0000-000000000009"}}, + }, + }, + }, + Artifacts: []*raw.Artifact{ + { + NodeID: "00000000-0000-0000-0000-000000000002", + DisplayName: "pkgStateArtifact", + GeneratedBy: "00000000-0000-0000-0000-000000000003", + }, + { + NodeID: "00000000-0000-0000-0000-000000000007", + DisplayName: "pkgPyWheel", + GeneratedBy: "00000000-0000-0000-0000-000000000008", + }, }, Sources: []*raw.Source{ { diff --git a/pkg/buildplan/raw/walk.go b/pkg/buildplan/raw/walk.go index c9aa7058d6..89294a8cac 100644 --- a/pkg/buildplan/raw/walk.go +++ b/pkg/buildplan/raw/walk.go @@ -1,8 +1,6 @@ package raw import ( - "errors" - "github.com/go-openapi/strfmt" "github.com/ActiveState/cli/internal/errs" @@ -11,23 +9,30 @@ import ( type walkFunc func(node interface{}, parent *Artifact) error -type WalkNodeContext struct { - Node interface{} - ParentArtifact *Artifact - tag StepInputTag - lookup map[strfmt.UUID]interface{} +type WalkStrategy struct { + tag StepInputTag + stopAtMultiSource bool // If true, we will stop walking if the artifact relates to multiple sources (eg. installer, docker img) } -type WalkInterrupt struct{} - -func (w WalkInterrupt) Error() string { - return "interrupt walk" -} +var ( + WalkViaSingleSource = WalkStrategy{ + TagSource, + true, + } + WalkViaMultiSource = WalkStrategy{ + TagSource, + false, + } + WalkViaDeps = WalkStrategy{ + TagDependency, + false, + } +) // WalkViaSteps walks the graph and reports on nodes it encounters // Note that the same node can be encountered multiple times if it is referenced in the graph multiple times. // In this case the context around the node may be different, even if the node itself isn't. -func (b *Build) WalkViaSteps(nodeIDs []strfmt.UUID, inputTag StepInputTag, walk walkFunc) error { +func (b *Build) WalkViaSteps(nodeIDs []strfmt.UUID, strategy WalkStrategy, walk walkFunc) error { lookup := b.LookupMap() for _, nodeID := range nodeIDs { @@ -35,7 +40,7 @@ func (b *Build) WalkViaSteps(nodeIDs []strfmt.UUID, inputTag StepInputTag, walk if !ok { return errs.New("node ID '%s' does not exist in lookup table", nodeID) } - if err := b.walkNodeViaSteps(node, nil, inputTag, walk); err != nil { + if err := b.walkNodeViaSteps(node, nil, strategy, walk); err != nil { return errs.Wrap(err, "could not recurse over node IDs") } } @@ -43,13 +48,10 @@ func (b *Build) WalkViaSteps(nodeIDs []strfmt.UUID, inputTag StepInputTag, walk return nil } -func (b *Build) walkNodeViaSteps(node interface{}, parent *Artifact, tag StepInputTag, walk walkFunc) error { +func (b *Build) walkNodeViaSteps(node interface{}, parent *Artifact, strategy WalkStrategy, walk walkFunc) error { lookup := b.LookupMap() if err := walk(node, parent); err != nil { - if errors.As(err, &WalkInterrupt{}) { - return nil - } return errs.Wrap(err, "error walking over node") } @@ -74,24 +76,29 @@ func (b *Build) walkNodeViaSteps(node interface{}, parent *Artifact, tag StepInp // tool, but it's technically possible to happen if someone requested a builder as part of their order. _, isSource = generatedByNode.(*Source) if isSource { - if err := b.walkNodeViaSteps(generatedByNode, ar, tag, walk); err != nil { + if err := b.walkNodeViaSteps(generatedByNode, ar, strategy, walk); err != nil { return errs.Wrap(err, "error walking source from generatedBy") } return nil // Sources are at the end of the recursion. } - nodeIDs, err := b.inputNodeIDsFromStep(ar, tag) + nodeIDs, err := b.inputNodeIDsFromStep(ar, strategy.tag) if err != nil { return errs.Wrap(err, "error walking over step inputs") } + // Stop if the next step has multiple input node ID's; this means we cannot determine a single source + if strategy.stopAtMultiSource && len(nodeIDs) > 1 { + return nil + } + for _, id := range nodeIDs { // Grab subnode that we want to iterate over next subNode, ok := lookup[id] if !ok { return errs.New("node ID '%s' does not exist in lookup table", id) } - if err := b.walkNodeViaSteps(subNode, ar, tag, walk); err != nil { + if err := b.walkNodeViaSteps(subNode, ar, strategy, walk); err != nil { return errs.Wrap(err, "error iterating over %s", id) } } diff --git a/pkg/buildplan/raw/walk_test.go b/pkg/buildplan/raw/walk_test.go index de002aef36..12a887cf1b 100644 --- a/pkg/buildplan/raw/walk_test.go +++ b/pkg/buildplan/raw/walk_test.go @@ -24,7 +24,7 @@ func TestRawBuild_walkNodesViaSteps(t *testing.T) { tests := []struct { name string nodeIDs []strfmt.UUID - tag raw.StepInputTag + strategy raw.WalkStrategy build *raw.Build wantCalls []walkCall wantErr bool @@ -32,7 +32,7 @@ func TestRawBuild_walkNodesViaSteps(t *testing.T) { { "Ingredient from step", []strfmt.UUID{"00000000-0000-0000-0000-000000000002"}, - raw.TagSource, + raw.WalkViaSingleSource, mock.BuildWithSourceFromStep, []walkCall{ {"00000000-0000-0000-0000-000000000002", "Artifact", ""}, @@ -44,7 +44,7 @@ func TestRawBuild_walkNodesViaSteps(t *testing.T) { { "Ingredient from generatedBy, multiple artifacts to same ingredient", []strfmt.UUID{"00000000-0000-0000-0000-000000000002", "00000000-0000-0000-0000-000000000003"}, - raw.TagSource, + raw.WalkViaSingleSource, mock.BuildWithSourceFromGeneratedBy, []walkCall{ {"00000000-0000-0000-0000-000000000002", "Artifact", ""}, @@ -54,10 +54,24 @@ func TestRawBuild_walkNodesViaSteps(t *testing.T) { }, false, }, + { + "Multiple sources through installer artifact", + []strfmt.UUID{"00000000-0000-0000-0000-000000000002"}, + raw.WalkViaMultiSource, + mock.BuildWithInstallerDepsViaSrc, + []walkCall{ + {"00000000-0000-0000-0000-000000000002", "Artifact", ""}, + {"00000000-0000-0000-0000-000000000007", "Artifact", "00000000-0000-0000-0000-000000000002"}, + {"00000000-0000-0000-0000-000000000009", "Source", strfmt.UUID("00000000-0000-0000-0000-000000000007")}, + {"00000000-0000-0000-0000-000000000010", "Artifact", "00000000-0000-0000-0000-000000000002"}, + {"00000000-0000-0000-0000-000000000012", "Source", strfmt.UUID("00000000-0000-0000-0000-000000000010")}, + }, + false, + }, { "Build time deps", []strfmt.UUID{"00000000-0000-0000-0000-000000000002"}, - raw.TagDependency, + raw.WalkViaDeps, mock.BuildWithBuildDeps, []walkCall{ {"00000000-0000-0000-0000-000000000002", "Artifact", ""}, @@ -93,7 +107,7 @@ func TestRawBuild_walkNodesViaSteps(t *testing.T) { return nil } - if err := tt.build.WalkViaSteps(tt.nodeIDs, tt.tag, walk); (err != nil) != tt.wantErr { + if err := tt.build.WalkViaSteps(tt.nodeIDs, tt.strategy, walk); (err != nil) != tt.wantErr { t.Errorf("walkNodes() error = %v, wantErr %v", errs.JoinMessage(err), tt.wantErr) } @@ -140,10 +154,11 @@ func TestRawBuild_walkNodesViaRuntimeDeps(t *testing.T) { }, { "Runtime deps via src step", - mock.BuildWithRuntimeDepsViaSrc.Terminals[0].NodeIDs, - mock.BuildWithRuntimeDepsViaSrc, + mock.BuildWithInstallerDepsViaSrc.Terminals[0].NodeIDs, + mock.BuildWithInstallerDepsViaSrc, []walkCall{ {"00000000-0000-0000-0000-000000000007", "Artifact", "00000000-0000-0000-0000-000000000002"}, + {"00000000-0000-0000-0000-000000000010", "Artifact", "00000000-0000-0000-0000-000000000002"}, }, false, }, diff --git a/pkg/runtime/depot.go b/pkg/runtime/depot.go index c17b4aa9ea..610418e952 100644 --- a/pkg/runtime/depot.go +++ b/pkg/runtime/depot.go @@ -137,6 +137,9 @@ func (d *depot) Path(id strfmt.UUID) string { // necessary information. Writing externally is preferred because otherwise the depot would need a lot of specialized // logic that ultimately don't really need to be a concern of the depot. func (d *depot) Put(id strfmt.UUID) error { + d.fsMutex.Lock() + defer d.fsMutex.Unlock() + if !fileutils.TargetExists(d.Path(id)) { return errs.New("could not put %s, as dir does not exist: %s", id, d.Path(id)) }