Simplifies target matching logic per spec PR review (#1166)

* Update units without updating code Signed-off-by: Natalie Arellano <[email protected]> * Update code Signed-off-by: Natalie Arellano <[email protected]> * Unpend test Signed-off-by: Natalie Arellano <[email protected]> * Add units for rebase without updating code Signed-off-by: Natalie Arellano <[email protected]> * Update rebase code Signed-off-by: Natalie Arellano <[email protected]> * Fix lint Signed-off-by: Natalie Arellano <[email protected]> * When we read the descriptor file, don't fill in "*" as a magic value as missing values are wildcard matches Signed-off-by: Natalie Arellano <[email protected]> * Stricter validation for rebase Signed-off-by: Natalie Arellano <[email protected]> --------- Signed-off-by: Natalie Arellano <[email protected]>
buildpacks · Jul 31, 2023 · 87d4f05 · 87d4f05
1 parent dc6af53
commit 87d4f05
Show file tree

Hide file tree

Showing 8 changed files with 187 additions and 81 deletions.
diff --git a/buildpack/bp_descriptor.go b/buildpack/bp_descriptor.go
@@ -77,7 +77,7 @@ func ReadBpDescriptor(path string) (*BpDescriptor, error) {
 			if stack.ID == "io.buildpacks.stacks.bionic" {
 				descriptor.Targets = append(descriptor.Targets, TargetMetadata{OS: "linux", Arch: "amd64", Distributions: []OSDistribution{{Name: "ubuntu", Version: "18.04"}}})
 			} else if stack.ID == "*" {
-				descriptor.Targets = append(descriptor.Targets, TargetMetadata{OS: "*", Arch: "*", Distributions: []OSDistribution{}})
+				descriptor.Targets = append(descriptor.Targets, TargetMetadata{}) // matches any
 			}
 		}
 	}
@@ -93,10 +93,10 @@ func ReadBpDescriptor(path string) (*BpDescriptor, error) {
 				bf := binFiles[len(binFiles)-i-1] // we're iterating backwards b/c os.ReadDir sorts "build.exe" after "build" but we want to preferentially detect windows first.
 				fname := bf.Name()
 				if fname == "build.exe" || fname == "build.bat" {
-					descriptor.Targets = append(descriptor.Targets, TargetMetadata{OS: "windows", Arch: "*"})
+					descriptor.Targets = append(descriptor.Targets, TargetMetadata{OS: "windows"})
 				}
 				if fname == "build" {
-					descriptor.Targets = append(descriptor.Targets, TargetMetadata{OS: "linux", Arch: "*"})
+					descriptor.Targets = append(descriptor.Targets, TargetMetadata{OS: "linux"})
 				}
 			}
 		}

diff --git a/buildpack/bp_descriptor_test.go b/buildpack/bp_descriptor_test.go
@@ -152,7 +152,7 @@ func testBpDescriptor(t *testing.T, when spec.G, it spec.S) {
 			h.AssertEq(t, descriptor.Stacks[0].ID, "some.non-magic.value")
 			h.AssertEq(t, len(descriptor.Targets), 1)
 			h.AssertEq(t, len(descriptor.Targets), 1)
-			h.AssertEq(t, descriptor.Targets[0].Arch, "*")
+			h.AssertEq(t, descriptor.Targets[0].Arch, "")
 			h.AssertEq(t, descriptor.Targets[0].OS, "linux")
 			h.AssertEq(t, len(descriptor.Targets[0].Distributions), 0)
 		})
@@ -170,9 +170,9 @@ func testBpDescriptor(t *testing.T, when spec.G, it spec.S) {
 			h.AssertEq(t, descriptor.Buildpack.SBOM, []string{"application/vnd.cyclonedx+json"})
 			// specific behaviors for this test
 			h.AssertEq(t, len(descriptor.Targets), 2)
-			h.AssertEq(t, descriptor.Targets[0].Arch, "*")
+			h.AssertEq(t, descriptor.Targets[0].Arch, "")
 			h.AssertEq(t, descriptor.Targets[0].OS, "windows")
-			h.AssertEq(t, descriptor.Targets[1].Arch, "*")
+			h.AssertEq(t, descriptor.Targets[1].Arch, "")
 			h.AssertEq(t, descriptor.Targets[1].OS, "linux")
 		})
 	})

diff --git a/buildpack/ext_descriptor.go b/buildpack/ext_descriptor.go
@@ -48,19 +48,18 @@ func (d *ExtDescriptor) inferTargets() error {
 				bf := binFiles[len(binFiles)-i-1] // we're iterating backwards b/c os.ReadDir sorts "foo.exe" after "foo" but we want to preferentially detect windows first.
 				fname := bf.Name()
 				if !windowsDetected && (fname == "detect.exe" || fname == "detect.bat" || fname == "generate.exe" || fname == "generate.bat") {
-					d.Targets = append(d.Targets, TargetMetadata{OS: "windows", Arch: "*"})
+					d.Targets = append(d.Targets, TargetMetadata{OS: "windows"})
 					windowsDetected = true
 				}
 				if !linuxDetected && (fname == "detect" || fname == "generate") {
-					d.Targets = append(d.Targets, TargetMetadata{OS: "linux", Arch: "*"})
+					d.Targets = append(d.Targets, TargetMetadata{OS: "linux"})
 					linuxDetected = true
 				}
 			}
 		}
 	}
-	// fallback: if nothing worked just mark it */*
 	if len(d.Targets) == 0 {
-		d.Targets = append(d.Targets, TargetMetadata{OS: "*", Arch: "*"})
+		d.Targets = append(d.Targets, TargetMetadata{}) // matches any
 	}
 	return nil
 }

diff --git a/buildpack/ext_descriptor_test.go b/buildpack/ext_descriptor_test.go
@@ -29,23 +29,23 @@ func testExtDescriptor(t *testing.T, when spec.G, it spec.S) {
 			h.AssertEq(t, descriptor.Extension.Homepage, "Extension A Homepage")
 			h.AssertEq(t, len(descriptor.Targets), 1)
 			h.AssertEq(t, descriptor.Targets[0].OS, "linux")
-			h.AssertEq(t, descriptor.Targets[0].Arch, "*")
+			h.AssertEq(t, descriptor.Targets[0].Arch, "")
 		})
 		it("infers */* if there's no files to infer from", func() {
 			path := filepath.Join("testdata", "extension", "by-id", "B", "v1", "extension.toml")
 			descriptor, err := buildpack.ReadExtDescriptor(path)
 			h.AssertNil(t, err)
 			h.AssertEq(t, len(descriptor.Targets), 1)
-			h.AssertEq(t, descriptor.Targets[0].OS, "*")
-			h.AssertEq(t, descriptor.Targets[0].Arch, "*")
+			h.AssertEq(t, descriptor.Targets[0].OS, "")
+			h.AssertEq(t, descriptor.Targets[0].Arch, "")
 		})
 		it("slices, it dices, it even does windows", func() {
 			path := filepath.Join("testdata", "extension", "by-id", "D", "v1", "extension.toml")
 			descriptor, err := buildpack.ReadExtDescriptor(path)
 			h.AssertNil(t, err)
 			h.AssertEq(t, len(descriptor.Targets), 1)
 			h.AssertEq(t, descriptor.Targets[0].OS, "windows")
-			h.AssertEq(t, descriptor.Targets[0].Arch, "*")
+			h.AssertEq(t, descriptor.Targets[0].Arch, "")
 		})
 	})
 }
diff --git a/detector.go b/detector.go
@@ -197,7 +197,7 @@ func isWildcard(t files.TargetMetadata) bool {
 
 func hasWildcard(ts []buildpack.TargetMetadata) bool {
 	for _, tm := range ts {
-		if tm.OS == "*" && tm.Arch == "*" {
+		if tm.OS == "" && tm.Arch == "" {
 			return true
 		}
 	}

diff --git a/detector_test.go b/detector_test.go
@@ -872,7 +872,7 @@ func testDetector(t *testing.T, when spec.G, it spec.S) {
 					WithAPI:   "0.12",
 					Buildpack: buildpack.BpInfo{BaseInfo: buildpack.BaseInfo{ID: "A", Version: "v1"}},
 					Targets: []buildpack.TargetMetadata{
-						{Arch: "*", OS: "*"}},
+						{Arch: "", OS: ""}},
 				}
 				dirStore.EXPECT().LookupBp("A", "v1").Return(bpA1, nil).AnyTimes()
 				executor.EXPECT().Detect(bpA1, gomock.Any(), gomock.Any())

diff --git a/platform/target_data.go b/platform/target_data.go
@@ -76,45 +76,52 @@ func GetTargetOSFromFileSystem(d fsutil.Detector, tm *files.TargetMetadata, logg
 }
 
 // TargetSatisfiedForBuild treats optional fields (ArchVariant and Distributions) as wildcards if empty, returns true if all populated fields match
-func TargetSatisfiedForBuild(t files.TargetMetadata, o buildpack.TargetMetadata) bool {
-	if (o.Arch != "*" && t.Arch != o.Arch) || (o.OS != "*" && t.OS != o.OS) {
+func TargetSatisfiedForBuild(base files.TargetMetadata, module buildpack.TargetMetadata) bool {
+	if !matches(base.OS, module.OS) {
 		return false
 	}
-	if t.ArchVariant != "" && o.ArchVariant != "" && t.ArchVariant != o.ArchVariant {
+	if !matches(base.Arch, module.Arch) {
 		return false
 	}
-
-	// if either of the lengths of Distributions are zero, treat it as a wildcard.
-	if t.Distribution != nil && len(o.Distributions) > 0 {
-		// this could be more efficient but the lists are probably short...
-		found := false
-		for _, odist := range o.Distributions {
-			if t.Distribution.Name == odist.Name && t.Distribution.Version == odist.Version {
-				found = true
-				continue
-			}
-		}
-		if !found {
-			return false
+	if !matches(base.ArchVariant, module.ArchVariant) {
+		return false
+	}
+	if base.Distribution == nil || len(module.Distributions) == 0 {
+		return true
+	}
+	foundMatchingDist := false
+	for _, modDist := range module.Distributions {
+		if matches(base.Distribution.Name, modDist.Name) && matches(base.Distribution.Version, modDist.Version) {
+			foundMatchingDist = true
+			break
 		}
 	}
-	return true
+	return foundMatchingDist
+}
+
+func matches(target1, target2 string) bool {
+	if target1 == "" || target2 == "" {
+		return true
+	}
+	return target1 == target2
 }
 
 // TargetSatisfiedForRebase treats optional fields (ArchVariant and Distribution fields) as wildcards if empty, returns true if all populated fields match
 func TargetSatisfiedForRebase(t files.TargetMetadata, appTargetMetadata files.TargetMetadata) bool {
-	if t.Arch != appTargetMetadata.Arch || t.OS != appTargetMetadata.OS {
+	if t.OS != appTargetMetadata.OS || t.Arch != appTargetMetadata.Arch {
 		return false
 	}
-	if t.ArchVariant != "" && appTargetMetadata.ArchVariant != "" && t.ArchVariant != appTargetMetadata.ArchVariant {
+	if !matches(t.ArchVariant, appTargetMetadata.ArchVariant) {
 		return false
 	}
-
-	if t.Distribution != nil && appTargetMetadata.Distribution != nil {
-		if t.Distribution.Name != appTargetMetadata.Distribution.Name {
+	if t.Distribution != nil {
+		if appTargetMetadata.Distribution == nil {
+			return false
+		}
+		if t.Distribution.Name != "" && t.Distribution.Name != appTargetMetadata.Distribution.Name {
 			return false
 		}
-		if t.Distribution.Version != appTargetMetadata.Distribution.Version {
+		if t.Distribution.Version != "" && t.Distribution.Version != appTargetMetadata.Distribution.Version {
 			return false
 		}
 	}

diff --git a/platform/target_data_test.go b/platform/target_data_test.go
@@ -21,50 +21,150 @@ func TestTargetData(t *testing.T) {
 
 func testTargetData(t *testing.T, when spec.G, it spec.S) {
 	when(".TargetSatisfiedForBuild", func() {
-		it("requires equality of OS and Arch", func() {
-			d := files.TargetMetadata{OS: "Win95", Arch: "Pentium"}
-
-			if platform.TargetSatisfiedForBuild(d, buildpack.TargetMetadata{OS: "Win98", Arch: d.Arch}) {
-				t.Fatal("TargetMetadata with different OS were equal")
-			}
-			if platform.TargetSatisfiedForBuild(d, buildpack.TargetMetadata{OS: d.OS, Arch: "Pentium MMX"}) {
-				t.Fatal("TargetMetadata with different Arch were equal")
-			}
-			if !platform.TargetSatisfiedForBuild(d, buildpack.TargetMetadata{OS: d.OS, Arch: d.Arch, ArchVariant: "MMX"}) {
-				t.Fatal("blank arch variant was not treated as wildcard")
-			}
-			if !platform.TargetSatisfiedForBuild(d, buildpack.TargetMetadata{
-				OS:            d.OS,
-				Arch:          d.Arch,
-				Distributions: []buildpack.OSDistribution{{Name: "a", Version: "2"}},
-			}) {
-				t.Fatal("blank distributions list was not treated as wildcard")
-			}
-
-			d.Distribution = &files.OSDistribution{Name: "A", Version: "1"}
-			if platform.TargetSatisfiedForBuild(d, buildpack.TargetMetadata{OS: d.OS, Arch: d.Arch, Distributions: []buildpack.OSDistribution{{Name: "g", Version: "2"}, {Name: "B", Version: "2"}}}) {
-				t.Fatal("unsatisfactory distribution lists were treated as satisfying")
-			}
-			if !platform.TargetSatisfiedForBuild(d, buildpack.TargetMetadata{OS: d.OS, Arch: d.Arch, Distributions: []buildpack.OSDistribution{}}) {
-				t.Fatal("blank distributions list was not treated as wildcard")
-			}
-			if !platform.TargetSatisfiedForBuild(d, buildpack.TargetMetadata{OS: d.OS, Arch: d.Arch, Distributions: []buildpack.OSDistribution{{Name: "B", Version: "2"}, {Name: "A", Version: "1"}}}) {
-				t.Fatal("distributions list including target's distribution not recognized as satisfying")
-			}
-		})
-
-		it("is cool with starry arches", func() {
-			d := files.TargetMetadata{OS: "windows", Arch: "amd64"}
-			if !platform.TargetSatisfiedForBuild(d, buildpack.TargetMetadata{OS: d.OS, Arch: "*"}) {
-				t.Fatal("Arch wildcard should have been satisfied with whatever we gave it")
-			}
+		var baseTarget files.TargetMetadata
+		when("base image data", func() {
+			when("has os and arch", func() {
+				baseTarget = files.TargetMetadata{OS: "Win95", Arch: "Pentium"}
+
+				when("buildpack data", func() {
+					when("has os and arch", func() {
+						it("must match", func() {
+							h.AssertEq(t, platform.TargetSatisfiedForBuild(baseTarget, buildpack.TargetMetadata{OS: baseTarget.OS, Arch: baseTarget.Arch}), true)
+							h.AssertEq(t, platform.TargetSatisfiedForBuild(baseTarget, buildpack.TargetMetadata{OS: "Win98", Arch: baseTarget.Arch}), false)
+							h.AssertEq(t, platform.TargetSatisfiedForBuild(baseTarget, buildpack.TargetMetadata{OS: baseTarget.OS, Arch: "Pentium MMX"}), false)
+						})
+					})
+
+					when("missing os and arch", func() {
+						it("matches", func() {
+							h.AssertEq(t, platform.TargetSatisfiedForBuild(baseTarget, buildpack.TargetMetadata{OS: "", Arch: ""}), true)
+						})
+					})
+
+					when("has extra information", func() {
+						it("matches", func() {
+							h.AssertEq(t, platform.TargetSatisfiedForBuild(baseTarget, buildpack.TargetMetadata{OS: baseTarget.OS, Arch: baseTarget.Arch, ArchVariant: "MMX"}), true)
+							h.AssertEq(t, platform.TargetSatisfiedForBuild(baseTarget, buildpack.TargetMetadata{
+								OS:            baseTarget.OS,
+								Arch:          baseTarget.Arch,
+								Distributions: []buildpack.OSDistribution{{Name: "a", Version: "2"}},
+							}), true)
+						})
+					})
+				})
+
+				when("has arch variant", func() {
+					baseTarget.ArchVariant = "some-arch-variant"
+
+					when("buildpack data", func() {
+						when("has arch variant", func() {
+							it("must match", func() {
+								h.AssertEq(t, platform.TargetSatisfiedForBuild(baseTarget, buildpack.TargetMetadata{OS: baseTarget.OS, Arch: baseTarget.Arch, ArchVariant: "some-arch-variant"}), true)
+								h.AssertEq(t, platform.TargetSatisfiedForBuild(baseTarget, buildpack.TargetMetadata{OS: baseTarget.OS, Arch: baseTarget.Arch, ArchVariant: "some-other-arch-variant"}), false)
+							})
+						})
+
+						when("missing arch variant", func() {
+							it("matches", func() {
+								h.AssertEq(t, platform.TargetSatisfiedForBuild(baseTarget, buildpack.TargetMetadata{OS: baseTarget.OS, Arch: baseTarget.Arch}), true)
+							})
+						})
+					})
+				})
+
+				when("has distro information", func() {
+					baseTarget.Distribution = &files.OSDistribution{Name: "A", Version: "1"}
+
+					when("buildpack data", func() {
+						when("has distro information", func() {
+							it("must match", func() {
+								h.AssertEq(t, platform.TargetSatisfiedForBuild(baseTarget, buildpack.TargetMetadata{OS: baseTarget.OS, Arch: baseTarget.Arch, Distributions: []buildpack.OSDistribution{{Name: "B", Version: "2"}, {Name: "A", Version: "1"}}}), true)
+								h.AssertEq(t, platform.TargetSatisfiedForBuild(baseTarget, buildpack.TargetMetadata{OS: baseTarget.OS, Arch: baseTarget.Arch, Distributions: []buildpack.OSDistribution{{Name: "g", Version: "2"}, {Name: "B", Version: "2"}}}), false)
+							})
+						})
+
+						when("missing distro information", func() {
+							it("matches", func() {
+								h.AssertEq(t, platform.TargetSatisfiedForBuild(baseTarget, buildpack.TargetMetadata{OS: baseTarget.OS, Arch: baseTarget.Arch}), true)
+							})
+						})
+					})
+				})
+			})
 		})
+	})
 
-		it("is down with OS stars", func() {
-			d := files.TargetMetadata{OS: "plan 9", Arch: "amd64"}
-			if !platform.TargetSatisfiedForBuild(d, buildpack.TargetMetadata{OS: "*", Arch: d.Arch}) {
-				t.Fatal("OS wildcard should have been satisfied by plan 9")
-			}
+	when(".TargetSatisfiedForRebase", func() {
+		var baseTarget files.TargetMetadata
+		when("orig image data", func() {
+			when("has os and arch", func() {
+				baseTarget = files.TargetMetadata{OS: "Win95", Arch: "Pentium"}
+
+				when("new image data", func() {
+					it("must match", func() {
+						h.AssertEq(t, platform.TargetSatisfiedForRebase(baseTarget, files.TargetMetadata{OS: baseTarget.OS, Arch: baseTarget.Arch}), true)
+						h.AssertEq(t, platform.TargetSatisfiedForRebase(baseTarget, files.TargetMetadata{OS: "Win98", Arch: baseTarget.Arch}), false)
+						h.AssertEq(t, platform.TargetSatisfiedForRebase(baseTarget, files.TargetMetadata{OS: baseTarget.OS, Arch: "Pentium MMX"}), false)
+					})
+
+					when("has extra information", func() {
+						it("matches", func() {
+							h.AssertEq(t, platform.TargetSatisfiedForRebase(baseTarget, files.TargetMetadata{OS: baseTarget.OS, Arch: baseTarget.Arch, ArchVariant: "MMX"}), true)
+							h.AssertEq(t, platform.TargetSatisfiedForRebase(baseTarget, files.TargetMetadata{
+								OS:           baseTarget.OS,
+								Arch:         baseTarget.Arch,
+								Distribution: &files.OSDistribution{Name: "a", Version: "2"},
+							}), true)
+						})
+					})
+				})
+
+				when("has arch variant", func() {
+					baseTarget.ArchVariant = "some-arch-variant"
+
+					when("new image data", func() {
+						when("has arch variant", func() {
+							it("must match", func() {
+								h.AssertEq(t, platform.TargetSatisfiedForRebase(baseTarget, files.TargetMetadata{OS: baseTarget.OS, Arch: baseTarget.Arch, ArchVariant: "some-arch-variant"}), true)
+								h.AssertEq(t, platform.TargetSatisfiedForRebase(baseTarget, files.TargetMetadata{OS: baseTarget.OS, Arch: baseTarget.Arch, ArchVariant: "some-other-arch-variant"}), false)
+							})
+						})
+
+						when("missing arch variant", func() {
+							it("matches", func() {
+								h.AssertEq(t, platform.TargetSatisfiedForRebase(baseTarget, files.TargetMetadata{OS: baseTarget.OS, Arch: baseTarget.Arch}), true)
+							})
+						})
+					})
+				})
+
+				when("has distro information", func() {
+					baseTarget.Distribution = &files.OSDistribution{Name: "A", Version: "1"}
+
+					when("new image data", func() {
+						when("has distro information", func() {
+							it("must match", func() {
+								h.AssertEq(t, platform.TargetSatisfiedForRebase(baseTarget, files.TargetMetadata{
+									OS:           baseTarget.OS,
+									Arch:         baseTarget.Arch,
+									Distribution: &files.OSDistribution{Name: "A", Version: "1"},
+								}), true)
+								h.AssertEq(t, platform.TargetSatisfiedForRebase(baseTarget, files.TargetMetadata{
+									OS:           baseTarget.OS,
+									Arch:         baseTarget.Arch,
+									Distribution: &files.OSDistribution{Name: "B", Version: "2"},
+								}), false)
+							})
+						})
+
+						when("missing distro information", func() {
+							it("errors", func() {
+								h.AssertEq(t, platform.TargetSatisfiedForRebase(baseTarget, files.TargetMetadata{OS: baseTarget.OS, Arch: baseTarget.Arch}), false)
+							})
+						})
+					})
+				})
+			})
 		})
 	})