From 6e8592a89bfba184c6aa8b703965d9edd3e5af32 Mon Sep 17 00:00:00 2001 From: Edoardo Rizzardi <138333833+edorizzardi@users.noreply.github.com> Date: Fri, 25 Oct 2024 18:29:24 +0200 Subject: [PATCH] [runx] Fix platform compatibility check for artifacts (#401) ## Summary This fixes two errors in the `findArtifactForPlatform` function: * If the artifact from the GitHub repository is not compatible with the invoker's platform, now it correctly returns a `types.ErrPlatformNotSupported` error, and it returns a `types.NoKnownArchive` error in case the artifact isn't packaged in a known artifact format * The way the artifact name string was parsed wasn't working with `x86_64` architecture: the string was split in two because of the underscore, thus never matching correctly. I simplified the logic, by simply checking if the artifact name string, forced to lowercase, contains the invoker's OS and architecture ## How was it tested? The first issue was tested by creating an ad-hoc GitHub project, compiling it for windows only, and uploading the compiled artifact to a GitHub release, and trying to donwload the binary via both standalone `runx` and with a local `devbox` build from a `linux/amd64` machine. The `NoKnownArchive` error was tested by trying to download an ad-hoc private `.deb` package. The second issue was tested by downloading the package `pb33f/wiretap@latest` from GitHub, previously not possible, with both standalone `runx` and a local `devbox` build. --- pkg/runx/impl/registry/artifact.go | 61 ++++++++------------- pkg/runx/impl/registry/registry.go | 6 +- pkg/runx/impl/registry/registry_test.go | 73 +++++++++++++++++++++++++ pkg/runx/impl/types/errors.go | 1 + 4 files changed, 98 insertions(+), 43 deletions(-) diff --git a/pkg/runx/impl/registry/artifact.go b/pkg/runx/impl/registry/artifact.go index 77141f30..a0178b49 100644 --- a/pkg/runx/impl/registry/artifact.go +++ b/pkg/runx/impl/registry/artifact.go @@ -3,55 +3,46 @@ package registry import ( "path/filepath" "strings" - "unicode" "go.jetpack.io/pkg/runx/impl/types" ) -func findArtifactForPlatform(artifacts []types.ArtifactMetadata, platform types.Platform) *types.ArtifactMetadata { - var artifactForPlatform types.ArtifactMetadata +func findArtifactForPlatform(artifacts []types.ArtifactMetadata, platform types.Platform) (types.ArtifactMetadata, error) { + foundPlatform := false for _, artifact := range artifacts { - if isArtifactForPlatform(artifact, platform) { - artifactForPlatform = artifact + if isArtifactForPlatform(artifact.Name, platform) { + foundPlatform = true if isKnownArchive(artifact.Name) { // We only consider known archives because sometimes releases contain multiple files // for the same platform. Some times those files are alternative installation methods // like `.dmg`, `.msi`, or `.deb`, and sometimes they are metadata files like `.sha256` // or a `.sig` file. We don't want to install those. - return &artifact + return artifact, nil } } } - // Best attempt: - return &artifactForPlatform + if !foundPlatform { + return types.ArtifactMetadata{}, types.ErrPlatformNotSupported + } + return types.ArtifactMetadata{}, types.ErrNoKnownArchive } -func isArtifactForPlatform(artifact types.ArtifactMetadata, platform types.Platform) bool { +func isArtifactForPlatform(artifactName string, platform types.Platform) bool { // Invalid platform: if platform.Arch() == "" || platform.OS() == "" { return false } - // As a heuristic we tokenize the name of the artifact, and return the artifact that has - // tokens for both the OS and the Architecture. - tokens := strings.FieldsFunc(strings.ToLower(artifact.Name), func(r rune) bool { - return !unicode.IsLetter(r) && !unicode.IsNumber(r) - }) hasOS := false hasArch := false - for _, token := range tokens { - if matchesOS(platform, token) { - hasOS = true - continue - } - if matchesArch(platform, token) { - hasArch = true - continue - } - if hasOS && hasArch { - return true - } + // We just check that the artifact name, forced to lowercase, + // contains the OS and architecture of the invoking system + if matchesOS(platform, strings.ToLower(artifactName)) { + hasOS = true + } + if matchesArch(platform, strings.ToLower(artifactName)) { + hasArch = true } return hasOS && hasArch } @@ -60,17 +51,14 @@ var alternateOSNames = map[string][]string{ "darwin": {"macos", "mac"}, } -func matchesOS(platform types.Platform, token string) bool { - if token == platform.OS() { - return true - } +func matchesOS(platform types.Platform, artifactName string) bool { alts := alternateOSNames[platform.OS()] for _, alt := range alts { - if token == alt { + if strings.Contains(artifactName, alt) { return true } } - return false + return strings.Contains(artifactName, platform.OS()) } var alternateArchNames = map[string][]string{ @@ -79,17 +67,14 @@ var alternateArchNames = map[string][]string{ "amd64": {"x86_64", "universal"}, } -func matchesArch(platform types.Platform, token string) bool { - if token == platform.Arch() { - return true - } +func matchesArch(platform types.Platform, artifactName string) bool { alts := alternateArchNames[platform.Arch()] for _, alt := range alts { - if token == alt { + if strings.Contains(artifactName, alt) { return true } } - return false + return strings.Contains(artifactName, platform.Arch()) } var knownExts = []string{ diff --git a/pkg/runx/impl/registry/registry.go b/pkg/runx/impl/registry/registry.go index 4ea1deb0..1bc98a82 100644 --- a/pkg/runx/impl/registry/registry.go +++ b/pkg/runx/impl/registry/registry.go @@ -72,11 +72,7 @@ func (r *Registry) GetArtifactMetadata(ctx context.Context, ref types.PkgRef, pl return types.ArtifactMetadata{}, err } - artifact := findArtifactForPlatform(release.Artifacts, platform) - if artifact == nil { - return types.ArtifactMetadata{}, types.ErrPlatformNotSupported - } - return *artifact, nil + return findArtifactForPlatform(release.Artifacts, platform) } func (r *Registry) GetArtifact(ctx context.Context, ref types.PkgRef, platform types.Platform) (string, error) { diff --git a/pkg/runx/impl/registry/registry_test.go b/pkg/runx/impl/registry/registry_test.go index 2dac7457..aa142b94 100644 --- a/pkg/runx/impl/registry/registry_test.go +++ b/pkg/runx/impl/registry/registry_test.go @@ -1,8 +1,12 @@ package registry import ( + "errors" "os" + "slices" "testing" + + "go.jetpack.io/pkg/runx/impl/types" ) func TestIsBinary(t *testing.T) { @@ -42,3 +46,72 @@ func TestIsBinary(t *testing.T) { }) } } + +func TestIsKnownArchive(t *testing.T) { + tests := []struct { + name string + want bool + }{ + {"archive.tar", true}, + {"archive.tar.gz", true}, + {"archive.deb", false}, + {"archive", false}, + } + for _, test := range tests { + got := isKnownArchive(test.name) + if got != test.want { + t.Errorf("isKnownArchive(%s) = %v, want %v", test.name, got, test.want) + } + } +} + +func TestIsArtifactForPlatform(t *testing.T) { + tests := []struct { + name string + platform types.Platform + want bool + }{ + {"linux-x86_64", types.NewPlatform("linux", "x86_64"), true}, + {"linux_x86_64_1.1.0_SNAPSHOT-abcde12345", types.NewPlatform("linux", "x86_64"), true}, + {"linux_x86_64-1.2.3-rc2.tar.gz", types.NewPlatform("linux", "amd64"), true}, + {"no-os-no-arch", types.NewPlatform("", ""), false}, + {"no-os-no-arch", types.NewPlatform("linux", "amd64"), false}, + {"no_os_arm64", types.NewPlatform("linux", "arm64"), false}, + {"linux_no-arch", types.NewPlatform("linux", "arm64"), false}, + {"macos-universal-1.2.3", types.NewPlatform("darwin", "arm64"), true}, + {"mac_386-1.2.3-snapshot_abcdef123456", types.NewPlatform("darwin", "386"), true}, + } + + for _, test := range tests { + got := isArtifactForPlatform(test.name, test.platform) + if got != test.want { + t.Errorf("isArtifactForPlatform(%s) = %v, want %v", test.name, got, test.want) + } + } +} + +func TestFindArtifactForPlatform(t *testing.T) { + tests := []struct { + artifacts []types.ArtifactMetadata + platform types.Platform + want bool + errTypeWant error + }{ + {[]types.ArtifactMetadata{{Name: "mac-amd64.tar.gz"}}, types.NewPlatform("darwin", "amd64"), true, nil}, + {[]types.ArtifactMetadata{{Name: "linux-amd64.deb"}}, types.NewPlatform("linux", "amd64"), false, types.ErrNoKnownArchive}, + {[]types.ArtifactMetadata{{Name: "linux-amd64"}}, types.NewPlatform("linux", "amd64"), false, types.ErrNoKnownArchive}, + {[]types.ArtifactMetadata{{Name: "darwin_arm64.tar"}}, types.NewPlatform("windows", "amd64"), false, types.ErrPlatformNotSupported}, + {[]types.ArtifactMetadata{{Name: "darwin_arm64"}}, types.NewPlatform("windows", "amd64"), false, types.ErrPlatformNotSupported}, + {[]types.ArtifactMetadata{{Name: "mac-amd64.tar.gz"}}, types.NewPlatform("", ""), false, types.ErrPlatformNotSupported}, + } + + for _, test := range tests { + got, err := findArtifactForPlatform(test.artifacts, test.platform) + if slices.Contains(test.artifacts, got) != test.want { + t.Errorf("findArtifactForPlatform() didn't return %v", got) + } + if !errors.Is(err, test.errTypeWant) { + t.Errorf("findArtifactForPlatform() returned %v error instead of %v", err, test.errTypeWant) + } + } +} diff --git a/pkg/runx/impl/types/errors.go b/pkg/runx/impl/types/errors.go index ab733839..e2268190 100644 --- a/pkg/runx/impl/types/errors.go +++ b/pkg/runx/impl/types/errors.go @@ -5,3 +5,4 @@ import "errors" var ErrPackageNotFound = errors.New("package not found") var ErrReleaseNotFound = errors.New("release not found") var ErrPlatformNotSupported = errors.New("package doesn't support platform") +var ErrNoKnownArchive = errors.New("package doesn't come in a known archive")