From 49684e0f870430716ce3809dee49a97b3f7b93d1 Mon Sep 17 00:00:00 2001 From: Phillip Nguyen Date: Wed, 21 Feb 2024 21:10:05 +0000 Subject: [PATCH 1/5] Implement repo priorities and integrate them into package version comparisons. --- client/client.go | 77 ++++++++++++++++++---------------- go.mod | 1 + go.sum | 4 +- googet.go | 100 +++++++++++++++++++++++++++----------------- googet_available.go | 8 ++-- googet_test.go | 31 +++++++------- 6 files changed, 127 insertions(+), 94 deletions(-) diff --git a/client/client.go b/client/client.go index a8af407..a6a8f13 100644 --- a/client/client.go +++ b/client/client.go @@ -92,19 +92,28 @@ func (ps *PackageState) Match(pi goolib.PackageInfo) bool { return ps.PackageSpec.Name == pi.Name && (ps.PackageSpec.Arch == pi.Arch || pi.Arch == "") && (ps.PackageSpec.Version == pi.Ver || pi.Ver == "") } +// Repo represents a single downloaded repo. +type Repo struct { + Priority int + Packages []goolib.RepoSpec +} + // RepoMap describes each repo's packages as seen from a client. -type RepoMap map[string][]goolib.RepoSpec +type RepoMap map[string]Repo // AvailableVersions builds a RepoMap from a list of sources. -func AvailableVersions(ctx context.Context, srcs []string, cacheDir string, cacheLife time.Duration, proxyServer string) RepoMap { +func AvailableVersions(ctx context.Context, srcs map[string]int, cacheDir string, cacheLife time.Duration, proxyServer string) RepoMap { rm := make(RepoMap) - for _, r := range srcs { + for r, pri := range srcs { rf, err := unmarshalRepoPackages(ctx, r, cacheDir, cacheLife, proxyServer) if err != nil { logger.Errorf("error reading repo %q: %v", r, err) continue } - rm[r] = rf + rm[r] = Repo{ + Priority: pri, + Packages: rf, + } } return rm } @@ -157,7 +166,7 @@ func decode(index io.ReadCloser, ct, url, cf string) ([]goolib.RepoSpec, error) // unmarshalRepoPackages gets and unmarshals a repository URL or uses the cached contents // if mtime is less than cacheLife. -// Sucessfully unmarshalled contents will be written to a cache. +// Successfully unmarshalled contents will be written to a cache. func unmarshalRepoPackages(ctx context.Context, p, cacheDir string, cacheLife time.Duration, proxyServer string) ([]goolib.RepoSpec, error) { pName := strings.TrimPrefix(p, "oauth-") @@ -306,8 +315,8 @@ func unmarshalRepoPackagesGCS(ctx context.Context, bucket, object, url, cf strin } // FindRepoSpec returns the element of pl whose PackageSpec matches pi. -func FindRepoSpec(pi goolib.PackageInfo, pl []goolib.RepoSpec) (goolib.RepoSpec, error) { - for _, p := range pl { +func FindRepoSpec(pi goolib.PackageInfo, repo Repo) (goolib.RepoSpec, error) { + for _, p := range repo.Packages { ps := p.PackageSpec if ps.Name == pi.Name && ps.Arch == pi.Arch && ps.Version == pi.Ver { return p, nil @@ -316,66 +325,64 @@ func FindRepoSpec(pi goolib.PackageInfo, pl []goolib.RepoSpec) (goolib.RepoSpec, return goolib.RepoSpec{}, fmt.Errorf("no match found for package %s.%s.%s in repo", pi.Name, pi.Arch, pi.Ver) } -func latest(psm map[string][]*goolib.PkgSpec) (ver, repo string) { +func latest(psm map[string][]*goolib.PkgSpec, rm RepoMap) (string, string) { + var ver, repo string + var pri int for r, pl := range psm { - for _, p := range pl { - if ver == "" { + for _, pkg := range pl { + q := rm[r].Priority + if ver == "" || q > pri { repo = r - ver = p.Version + ver = pkg.Version + pri = q continue } - c, err := goolib.Compare(p.Version, ver) + if q < pri { + continue + } + c, err := goolib.Compare(pkg.Version, ver) if err != nil { - logger.Errorf("compare of %s to %s failed with error: %v", p.Version, ver, err) + logger.Errorf("compare of %s to %s failed with error: %v", pkg.Version, ver, err) } if c == 1 { repo = r - ver = p.Version + ver = pkg.Version + pri = q } } } - return + return ver, repo } // FindRepoLatest returns the latest version of a package along with its repo and arch. -func FindRepoLatest(pi goolib.PackageInfo, rm RepoMap, archs []string) (ver, repo, arch string, err error) { +func FindRepoLatest(pi goolib.PackageInfo, rm RepoMap, archs []string) (string, string, string, error) { psm := make(map[string][]*goolib.PkgSpec) + name := pi.Name if pi.Arch != "" { - for r, pl := range rm { - for _, p := range pl { - if p.PackageSpec.Name == pi.Name && p.PackageSpec.Arch == pi.Arch { - psm[r] = append(psm[r], p.PackageSpec) - } - } - } - if len(psm) != 0 { - v, r := latest(psm) - return v, r, pi.Arch, nil - } - return "", "", "", fmt.Errorf("no versions of package %s.%s found in any repo", pi.Name, pi.Arch) + archs = []string{pi.Arch} + name = fmt.Sprintf("%s.%s", pi.Name, pi.Arch) } - for _, a := range archs { - for r, pl := range rm { - for _, p := range pl { + for r, repo := range rm { + for _, p := range repo.Packages { if p.PackageSpec.Name == pi.Name && p.PackageSpec.Arch == a { psm[r] = append(psm[r], p.PackageSpec) } } } if len(psm) != 0 { - v, r := latest(psm) + v, r := latest(psm, rm) return v, r, a, nil } } - return "", "", "", fmt.Errorf("no versions of package %s found in any repo", pi.Name) + return "", "", "", fmt.Errorf("no versions of package %s found in any repo", name) } // WhatRepo returns what repo a package is in. // Name, Arch, and Ver fields of PackageInfo must be provided. func WhatRepo(pi goolib.PackageInfo, rm RepoMap) (string, error) { - for r, pl := range rm { - for _, p := range pl { + for r, repo := range rm { + for _, p := range repo.Packages { if p.PackageSpec.Name == pi.Name && p.PackageSpec.Arch == pi.Arch && p.PackageSpec.Version == pi.Ver { return r, nil } diff --git a/go.mod b/go.mod index 2617b3f..2f0d078 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ require ( github.com/blang/semver v3.5.1+incompatible github.com/dustin/go-humanize v1.0.0 github.com/go-yaml/yaml v2.1.0+incompatible + github.com/google/go-cmp v0.6.0 github.com/google/logger v1.1.1 github.com/google/subcommands v1.2.0 github.com/olekukonko/tablewriter v0.0.5 diff --git a/go.sum b/go.sum index b8750ce..0e5a086 100644 --- a/go.sum +++ b/go.sum @@ -114,8 +114,9 @@ github.com/google/go-cmp v0.5.1/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/ github.com/google/go-cmp v0.5.2/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.5.3/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.5.4/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= -github.com/google/go-cmp v0.5.5 h1:Khx7svrCpmxxtHBq5j2mp/xVjsi8hQMfNLvJFAlrGgU= github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= +github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= github.com/google/logger v1.1.1 h1:+6Z2geNxc9G+4D4oDO9njjjn2d0wN5d7uOo0vOIW1NQ= github.com/google/logger v1.1.1/go.mod h1:BkeJZ+1FhQ+/d087r4dzojEg1u2ZX+ZqG1jTUrLM+zQ= github.com/google/martian v2.1.0+incompatible h1:/CP5g8u/VJHijgedC/Legn3BAbAaWPgecwXBIDzw5no= @@ -391,7 +392,6 @@ golang.org/x/tools v0.6.0/go.mod h1:Xwgl3UAJ/d3gWutnCtw505GrjyAbvKui8lOU390QaIU= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= -golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE= golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= google.golang.org/api v0.4.0/go.mod h1:8k5glujaEP+g9n7WNsDg8QP6cUVNI86fCNMcbazEtwE= google.golang.org/api v0.7.0/go.mod h1:WtwebWUNSVBH/HAw79HIFXZNqEvBhG+Ra+ax0hx3E3M= diff --git a/googet.go b/googet.go index 4ec7a50..a5a30c5 100644 --- a/googet.go +++ b/googet.go @@ -25,6 +25,7 @@ import ( "os" "path" "path/filepath" + "strconv" "strings" "time" @@ -38,13 +39,14 @@ import ( ) const ( - stateFile = "googet.state" - confFile = "googet.conf" - logFile = "googet.log" - cacheDir = "cache" - repoDir = "repos" - envVar = "GooGetRoot" - logSize = 10 * 1024 * 1024 + stateFile = "googet.state" + confFile = "googet.conf" + logFile = "googet.log" + cacheDir = "cache" + repoDir = "repos" + envVar = "GooGetRoot" + logSize = 10 * 1024 * 1024 + defaultPriority = 500 ) var ( @@ -82,6 +84,7 @@ type repoEntry struct { Name string URL string UseOAuth bool + Priority int } // UnmarshalYAML provides custom unmarshalling for repoEntry objects. @@ -98,6 +101,12 @@ func (r *repoEntry) UnmarshalYAML(unmarshal func(interface{}) error) error { r.URL = v case "useoauth": r.UseOAuth = strings.ToLower(v) == "true" + case "priority": + var err error + r.Priority, err = strconv.Atoi(v) + if err != nil { + return fmt.Errorf("invalid priority: %v", v) + } } } if r.URL == "" { @@ -163,44 +172,54 @@ func unmarshalConfFile(p string) (*conf, error) { return &cf, yaml.Unmarshal(b, &cf) } -func repoList(dir string) ([]string, error) { +// validateRepoURL uses the global allowUnsafeURL to determine if u should be checked for https or +// GCS status. +func validateRepoURL(u string) bool { + if allowUnsafeURL { + return true + } + gcs, _, _ := goolib.SplitGCSUrl(u) + parsed, err := url.Parse(u) + if err != nil { + logger.Errorf("Failed to parse URL '%s', skipping repo", u) + return false + } + if parsed.Scheme != "https" && !gcs { + logger.Errorf("%s will not be used as a repository, only https and Google Cloud Storage endpoints will be used unless 'allowunsafeurl' is set to 'true' in googet.conf", u) + return false + } + return true +} + +// repoList returns a deduped set of all repos listed in the repo config files contained in dir. +// The repos are mapped to priority values. If a repo config does not specify a priority, the repo +// is assigned the default priority value. If the same repo appears multiple times with different +// priority values, it is mapped to the highest seen priority value. +func repoList(dir string) (map[string]int, error) { rfs, err := repos(dir) if err != nil { return nil, err } - var rl []string + result := make(map[string]int) for _, rf := range rfs { for _, re := range rf.repoEntries { - switch { - case re.URL != "": - if re.UseOAuth { - rl = append(rl, "oauth-"+re.URL) - } else { - rl = append(rl, re.URL) - } - } - } - } - - if !allowUnsafeURL { - var srl []string - for _, r := range rl { - rTrimmed := strings.TrimPrefix(r, "oauth-") - isGCSURL, _, _ := goolib.SplitGCSUrl(rTrimmed) - parsed, err := url.Parse(rTrimmed) - if err != nil { - logger.Errorf("Failed to parse URL '%s', skipping repo", r) + u := re.URL + if u == "" || !validateRepoURL(u) { continue } - if parsed.Scheme != "https" && !isGCSURL { - logger.Errorf("%s will not be used as a repository, only https and Google Cloud Storage endpoints will be used unless 'allowunsafeurl' is set to 'true' in googet.conf", r) - continue + if re.UseOAuth { + u = "oauth-" + u + } + p := re.Priority + if p <= 0 { + p = defaultPriority + } + if q, ok := result[u]; !ok || p > q { + result[u] = p } - srl = append(srl, r) } - return srl, nil } - return rl, nil + return result, nil } func repos(dir string) ([]repoFile, error) { @@ -273,12 +292,15 @@ func readStateFromPath(sf string) (*client.GooGetState, error) { return client.UnmarshalState(b) } -func buildSources(s string) ([]string, error) { - if s != "" { - srcs := strings.Split(s, ",") - return srcs, nil +func buildSources(s string) (map[string]int, error) { + if s == "" { + return repoList(filepath.Join(rootDir, repoDir)) + } + m := make(map[string]int) + for _, src := range strings.Split(s, ",") { + m[src] = defaultPriority } - return repoList(filepath.Join(rootDir, repoDir)) + return m, nil } func confirmation(msg string) bool { diff --git a/googet_available.go b/googet_available.go index 17cb2da..53141ae 100644 --- a/googet_available.go +++ b/googet_available.go @@ -75,8 +75,8 @@ func (cmd *availableCmd) Execute(ctx context.Context, f *flag.FlagSet, _ ...inte m := make(map[string][]string) rm := client.AvailableVersions(ctx, repos, filepath.Join(rootDir, cacheDir), cacheLife, proxyServer) - for r, pl := range rm { - for _, p := range pl { + for r, repo := range rm { + for _, p := range repo.Packages { m[r] = append(m[r], p.PackageSpec.Name+"."+p.PackageSpec.Arch+"."+p.PackageSpec.Version) } } @@ -111,8 +111,8 @@ func (cmd *availableCmd) Execute(ctx context.Context, f *flag.FlagSet, _ ...inte } func repo(pi goolib.PackageInfo, rm client.RepoMap) { - for r, pl := range rm { - for _, p := range pl { + for r, repo := range rm { + for _, p := range repo.Packages { if p.PackageSpec.Name == pi.Name && p.PackageSpec.Arch == pi.Arch && p.PackageSpec.Version == pi.Ver { info(p.PackageSpec, r) return diff --git a/googet_test.go b/googet_test.go index 3b62afd..45def50 100644 --- a/googet_test.go +++ b/googet_test.go @@ -20,6 +20,8 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/google/googet/v2/client" "github.com/google/googet/v2/goolib" "github.com/google/googet/v2/oswrap" @@ -39,29 +41,30 @@ func TestRepoList(t *testing.T) { repoTests := []struct { content []byte - want []string + want map[string]int allowUnsafeURL bool }{ {[]byte("\n"), nil, false}, {[]byte("# This is just a comment"), nil, false}, - {[]byte("url: " + testRepo), []string{testRepo}, false}, - {[]byte("\n # Comment\nurl: " + testRepo), []string{testRepo}, false}, - {[]byte("- url: " + testRepo), []string{testRepo}, false}, + {[]byte("url: " + testRepo), map[string]int{testRepo: 500}, false}, + {[]byte("\n # Comment\nurl: " + testRepo), map[string]int{testRepo: 500}, false}, + {[]byte("- url: " + testRepo), map[string]int{testRepo: 500}, false}, // The HTTP repo should be dropped. {[]byte("- url: " + testHTTPRepo), nil, false}, // The HTTP repo should not be dropped. - {[]byte("- url: " + testHTTPRepo), []string{testHTTPRepo}, true}, - {[]byte("- URL: " + testRepo), []string{testRepo}, false}, + {[]byte("- url: " + testHTTPRepo), map[string]int{testHTTPRepo: 500}, true}, + {[]byte("- URL: " + testRepo), map[string]int{testRepo: 500}, false}, // The HTTP repo should be dropped. - {[]byte("- url: " + testRepo + "\n\n- URL: " + testHTTPRepo), []string{testRepo}, false}, + {[]byte("- url: " + testRepo + "\n\n- URL: " + testHTTPRepo), map[string]int{testRepo: 500}, false}, // The HTTP repo should not be dropped. - {[]byte("- url: " + testRepo + "\n\n- URL: " + testHTTPRepo), []string{testRepo, testHTTPRepo}, true}, - {[]byte("- url: " + testRepo + "\n\n- URL: " + testRepo), []string{testRepo, testRepo}, false}, - {[]byte("- url: " + testRepo + "\n\n- url: " + testRepo), []string{testRepo, testRepo}, false}, + {[]byte("- url: " + testRepo + "\n\n- URL: " + testHTTPRepo), map[string]int{testRepo: 500, testHTTPRepo: 500}, true}, + {[]byte("- url: " + testRepo + "\n\n- URL: " + testRepo), map[string]int{testRepo: 500}, false}, + {[]byte("- url: " + testRepo + "\n\n- url: " + testRepo), map[string]int{testRepo: 500}, false}, // Should contain oauth- prefix - {[]byte("- url: " + testRepo + "\n useoauth: true"), []string{"oauth-" + testRepo}, false}, + {[]byte("- url: " + testRepo + "\n useoauth: true"), map[string]int{"oauth-" + testRepo: 500}, false}, // Should not contain oauth- prefix - {[]byte("- url: " + testRepo + "\n useoauth: false"), []string{testRepo}, false}, + {[]byte("- url: " + testRepo + "\n useoauth: false"), map[string]int{testRepo: 500}, false}, + {[]byte("- url: " + testRepo + "\n priority: 1200"), map[string]int{testRepo: 1200}, false}, } for i, tt := range repoTests { @@ -73,8 +76,8 @@ func TestRepoList(t *testing.T) { if err != nil { t.Fatal(err) } - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("test case %d: returned repo does not match expected repo: got %q, want %q", i+1, got, tt.want) + if diff := cmp.Diff(tt.want, got, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("test case %d: repoList unexpected diff (-want +got): %v", i+1, diff) } } } From 1aeb4a97302e6814842f64da4d101cb4bd854778 Mon Sep 17 00:00:00 2001 From: Phillip Nguyen Date: Fri, 29 Mar 2024 20:07:44 +0000 Subject: [PATCH 2/5] Use repo priorities when choosing which versions to install or update. Drop the redundant NeedsInstallation check from install.FromRepo. In all calls of FromRepo, prior checks have already been performed and this extra check breaks priority rollbacks. --- client/client.go | 23 +++++++--------- client/client_test.go | 62 +++++++++++++++++++++++-------------------- googet_update.go | 2 +- goolib/goospec.go | 11 ++++++++ install/install.go | 15 ++--------- 5 files changed, 57 insertions(+), 56 deletions(-) diff --git a/client/client.go b/client/client.go index a6a8f13..d986af5 100644 --- a/client/client.go +++ b/client/client.go @@ -314,7 +314,7 @@ func unmarshalRepoPackagesGCS(ctx context.Context, bucket, object, url, cf strin return decode(r, "application/json", url, cf) } -// FindRepoSpec returns the element of pl whose PackageSpec matches pi. +// FindRepoSpec returns the RepoSpec in repo whose PackageSpec matches pi. func FindRepoSpec(pi goolib.PackageInfo, repo Repo) (goolib.RepoSpec, error) { for _, p := range repo.Packages { ps := p.PackageSpec @@ -325,24 +325,21 @@ func FindRepoSpec(pi goolib.PackageInfo, repo Repo) (goolib.RepoSpec, error) { return goolib.RepoSpec{}, fmt.Errorf("no match found for package %s.%s.%s in repo", pi.Name, pi.Arch, pi.Ver) } +// latest returns the version and repo having the greatest (priority, version) from the set of +// package specs in psm. func latest(psm map[string][]*goolib.PkgSpec, rm RepoMap) (string, string) { var ver, repo string var pri int for r, pl := range psm { for _, pkg := range pl { q := rm[r].Priority - if ver == "" || q > pri { - repo = r - ver = pkg.Version - pri = q - continue - } - if q < pri { - continue - } - c, err := goolib.Compare(pkg.Version, ver) - if err != nil { - logger.Errorf("compare of %s to %s failed with error: %v", pkg.Version, ver, err) + c := 1 + if ver != "" { + var err error + if c, err = goolib.ComparePriorityVersion(q, pkg.Version, pri, ver); err != nil { + logger.Errorf("compare of %s to %s failed with error: %v", pkg.Version, ver, err) + continue + } } if c == 1 { repo = r diff --git a/client/client_test.go b/client/client_test.go index 5562f40..0bd7a3d 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -96,12 +96,14 @@ func TestGetPackageStateNoMatch(t *testing.T) { func TestWhatRepo(t *testing.T) { rm := RepoMap{ - "foo_repo": []goolib.RepoSpec{ - { - PackageSpec: &goolib.PkgSpec{ - Name: "foo_pkg", - Version: "1.2.3@4", - Arch: "noarch", + "foo_repo": Repo{ + Packages: []goolib.RepoSpec{ + { + PackageSpec: &goolib.PkgSpec{ + Name: "foo_pkg", + Version: "1.2.3@4", + Arch: "noarch", + }, }, }, }, @@ -119,26 +121,28 @@ func TestWhatRepo(t *testing.T) { func TestFindRepoLatest(t *testing.T) { archs := []string{"noarch", "x86_64"} rm := RepoMap{ - "foo_repo": []goolib.RepoSpec{ - { - PackageSpec: &goolib.PkgSpec{ - Name: "foo_pkg", - Version: "1.2.3@4", - Arch: "noarch", + "foo_repo": Repo{ + Packages: []goolib.RepoSpec{ + { + PackageSpec: &goolib.PkgSpec{ + Name: "foo_pkg", + Version: "1.2.3@4", + Arch: "noarch", + }, }, - }, - { - PackageSpec: &goolib.PkgSpec{ - Name: "foo_pkg", - Version: "1.0.0@1", - Arch: "noarch", + { + PackageSpec: &goolib.PkgSpec{ + Name: "foo_pkg", + Version: "1.0.0@1", + Arch: "noarch", + }, }, - }, - { - PackageSpec: &goolib.PkgSpec{ - Name: "bar_pkg", - Version: "1.0.0@1", - Arch: "noarch", + { + PackageSpec: &goolib.PkgSpec{ + Name: "bar_pkg", + Version: "1.0.0@1", + Arch: "noarch", + }, }, }, }, @@ -298,12 +302,12 @@ func TestUnmarshalRepoPackagesCache(t *testing.T) { func TestFindRepoSpec(t *testing.T) { want := goolib.RepoSpec{PackageSpec: &goolib.PkgSpec{Name: "test"}} - rs := []goolib.RepoSpec{ + repo := Repo{Packages: []goolib.RepoSpec{ want, {PackageSpec: &goolib.PkgSpec{Name: "test2"}}, - } + }} - got, err := FindRepoSpec(goolib.PackageInfo{Name: "test", Arch: "", Ver: ""}, rs) + got, err := FindRepoSpec(goolib.PackageInfo{Name: "test", Arch: "", Ver: ""}, repo) if err != nil { t.Errorf("error running FindRepoSpec: %v", err) } @@ -313,9 +317,9 @@ func TestFindRepoSpec(t *testing.T) { } func TestFindRepoSpecNoMatch(t *testing.T) { - rs := []goolib.RepoSpec{{PackageSpec: &goolib.PkgSpec{Name: "test2"}}} + repo := Repo{Packages: []goolib.RepoSpec{{PackageSpec: &goolib.PkgSpec{Name: "test2"}}}} - if _, err := FindRepoSpec(goolib.PackageInfo{Name: "test", Arch: "", Ver: ""}, rs); err == nil { + if _, err := FindRepoSpec(goolib.PackageInfo{Name: "test", Arch: "", Ver: ""}, repo); err == nil { t.Error("did not get expected error when running FindRepoSpec") } } diff --git a/googet_update.go b/googet_update.go index e6c75b7..bf353d4 100644 --- a/googet_update.go +++ b/googet_update.go @@ -112,7 +112,7 @@ func updates(pm packageMap, rm client.RepoMap) []goolib.PackageInfo { logger.Info(err) continue } - c, err := goolib.Compare(v, ver) + c, err := goolib.ComparePriorityVersion(rm[r].Priority, v, defaultPriority, ver) if err != nil { logger.Error(err) continue diff --git a/goolib/goospec.go b/goolib/goospec.go index 34e1e8b..33c5079 100644 --- a/goolib/goospec.go +++ b/goolib/goospec.go @@ -169,6 +169,17 @@ func fixVer(ver string) string { return strings.Join(out, ".") + suffix } +// ComparePriorityVersion compares (p1, v1) to (p2, v2) as priority-version tuples. +func ComparePriorityVersion(p1 int, v1 string, p2 int, v2 string) (int, error) { + if p1 < p2 { + return -1, nil + } + if p1 > p2 { + return 1, nil + } + return Compare(v1, v2) +} + // ParseVersion parses the string version into goospec.Version. ParseVersion // attempts to fix non-compliant Semver strings by removing leading zeros from // components, and replacing any missing components with zero values after diff --git a/install/install.go b/install/install.go index cb54cc8..244b875 100644 --- a/install/install.go +++ b/install/install.go @@ -122,7 +122,7 @@ func installDeps(ctx context.Context, ps *goolib.PkgSpec, cache string, rm clien ins = true } if !ins { - return fmt.Errorf("cannot resolve dependancy, %s.%s version %s or greater not installed and not available in any repo", pi.Name, arch, ver) + return fmt.Errorf("cannot resolve dependency, %s.%s version %s or greater not installed and not available in any repo", pi.Name, arch, ver) } } return resolveReplacements(ctx, ps, state, dbOnly, proxyServer) @@ -130,14 +130,6 @@ func installDeps(ctx context.Context, ps *goolib.PkgSpec, cache string, rm clien // FromRepo installs a package and all dependencies from a repository. func FromRepo(ctx context.Context, pi goolib.PackageInfo, repo, cache string, rm client.RepoMap, archs []string, state *client.GooGetState, dbOnly bool, proxyServer string) error { - ni, err := NeedsInstallation(pi, *state) - if err != nil { - return err - } - if !ni { - return nil - } - logger.Infof("Starting install of %s.%s.%s", pi.Name, pi.Arch, pi.Ver) fmt.Printf("Installing %s.%s.%s and dependencies...\n", pi.Name, pi.Arch, pi.Ver) rs, err := client.FindRepoSpec(pi, rm[repo]) @@ -332,10 +324,7 @@ func copyPkg(src, dst string) (retErr error) { // NeedsInstallation checks if a package version needs installation. func NeedsInstallation(pi goolib.PackageInfo, state client.GooGetState) (bool, error) { for _, p := range state { - if p.PackageSpec.Name == pi.Name { - if p.PackageSpec.Arch != pi.Arch { - continue - } + if p.PackageSpec.Name == pi.Name && p.PackageSpec.Arch == pi.Arch { c, err := goolib.Compare(p.PackageSpec.Version, pi.Ver) if err != nil { return true, err From 92ef39eb644d65158e2bc6a484a1b11c753b949b Mon Sep 17 00:00:00 2001 From: Phillip Nguyen Date: Mon, 1 Apr 2024 18:03:01 +0000 Subject: [PATCH 3/5] Add more tests of FindRepoLatest and ComparePriorityVersion. --- client/client.go | 2 + client/client_test.go | 159 ++++++++++++++++++++++++++++------------- goolib/goospec_test.go | 33 +++++++++ 3 files changed, 144 insertions(+), 50 deletions(-) diff --git a/client/client.go b/client/client.go index d986af5..339d884 100644 --- a/client/client.go +++ b/client/client.go @@ -352,6 +352,8 @@ func latest(psm map[string][]*goolib.PkgSpec, rm RepoMap) (string, string) { } // FindRepoLatest returns the latest version of a package along with its repo and arch. +// The archs are searched in order; if a matching package is found for any arch, it is +// returned immediately even if a later arch might have a later version. func FindRepoLatest(pi goolib.PackageInfo, rm RepoMap, archs []string) (string, string, string, error) { psm := make(map[string][]*goolib.PkgSpec) name := pi.Name diff --git a/client/client_test.go b/client/client_test.go index 0bd7a3d..04cfb31 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -119,64 +119,123 @@ func TestWhatRepo(t *testing.T) { } func TestFindRepoLatest(t *testing.T) { - archs := []string{"noarch", "x86_64"} - rm := RepoMap{ - "foo_repo": Repo{ - Packages: []goolib.RepoSpec{ - { - PackageSpec: &goolib.PkgSpec{ - Name: "foo_pkg", - Version: "1.2.3@4", - Arch: "noarch", + for _, tt := range []struct { + desc string + pi goolib.PackageInfo + archs []string + rm RepoMap + wantVersion string + wantArch string + wantRepo string + wantErr bool + }{ + { + desc: "name and arch", + pi: goolib.PackageInfo{Name: "foo_pkg", Arch: "noarch"}, + archs: []string{"noarch", "x86_64"}, + rm: RepoMap{ + "foo_repo": Repo{Packages: []goolib.RepoSpec{ + {PackageSpec: &goolib.PkgSpec{Name: "foo_pkg", Version: "1.2.3@4", Arch: "noarch"}}, + {PackageSpec: &goolib.PkgSpec{Name: "foo_pkg", Version: "2.0.0@1", Arch: "x86_64"}}, + {PackageSpec: &goolib.PkgSpec{Name: "foo_pkg", Version: "1.0.0@1", Arch: "noarch"}}, + {PackageSpec: &goolib.PkgSpec{Name: "bar_pkg", Version: "2.3.0@1", Arch: "noarch"}}, + }}, + }, + wantVersion: "1.2.3@4", + wantArch: "noarch", + wantRepo: "foo_repo", + }, + { + desc: "name only", + pi: goolib.PackageInfo{Name: "foo_pkg"}, + archs: []string{"noarch", "x86_64"}, + rm: RepoMap{ + "foo_repo": Repo{Packages: []goolib.RepoSpec{ + {PackageSpec: &goolib.PkgSpec{Name: "foo_pkg", Version: "1.2.3@4", Arch: "noarch"}}, + {PackageSpec: &goolib.PkgSpec{Name: "foo_pkg", Version: "2.0.0@1", Arch: "x86_64"}}, + {PackageSpec: &goolib.PkgSpec{Name: "foo_pkg", Version: "1.0.0@1", Arch: "noarch"}}, + {PackageSpec: &goolib.PkgSpec{Name: "bar_pkg", Version: "2.3.0@1", Arch: "noarch"}}, + }}, + }, + wantVersion: "1.2.3@4", + wantArch: "noarch", + wantRepo: "foo_repo", + }, + { + desc: "specified arch not present", + pi: goolib.PackageInfo{Name: "foo_pkg", Arch: "x86_64"}, + archs: []string{"noarch", "x86_64"}, + rm: RepoMap{ + "foo_repo": Repo{Packages: []goolib.RepoSpec{ + {PackageSpec: &goolib.PkgSpec{Name: "foo_pkg", Version: "1.2.3@4", Arch: "noarch"}}, + {PackageSpec: &goolib.PkgSpec{Name: "foo_pkg", Version: "1.0.0@1", Arch: "noarch"}}, + {PackageSpec: &goolib.PkgSpec{Name: "bar_pkg", Version: "2.3.0@1", Arch: "noarch"}}, + }}, + }, + wantErr: true, + }, + { + desc: "multiple repos with same priority", + pi: goolib.PackageInfo{Name: "foo_pkg", Arch: "noarch"}, + archs: []string{"noarch", "x86_64"}, + rm: RepoMap{ + "foo_repo": Repo{ + Priority: 500, + Packages: []goolib.RepoSpec{ + {PackageSpec: &goolib.PkgSpec{Name: "foo_pkg", Version: "1.2.3@4", Arch: "noarch"}}, }, }, - { - PackageSpec: &goolib.PkgSpec{ - Name: "foo_pkg", - Version: "1.0.0@1", - Arch: "noarch", + "bar_repo": Repo{ + Priority: 500, + Packages: []goolib.RepoSpec{ + {PackageSpec: &goolib.PkgSpec{Name: "foo_pkg", Version: "2.4.5@1", Arch: "noarch"}}, }, }, - { - PackageSpec: &goolib.PkgSpec{ - Name: "bar_pkg", - Version: "1.0.0@1", - Arch: "noarch", + }, + wantVersion: "2.4.5@1", + wantArch: "noarch", + wantRepo: "bar_repo", + }, + { + desc: "multiple repos with different priority", + pi: goolib.PackageInfo{Name: "foo_pkg", Arch: "noarch"}, + archs: []string{"noarch", "x86_64"}, + rm: RepoMap{ + "high_priority_repo": Repo{ + Priority: 1500, + Packages: []goolib.RepoSpec{ + {PackageSpec: &goolib.PkgSpec{Name: "foo_pkg", Version: "1.2.3@4", Arch: "noarch"}}, + }, + }, + "low_priority_repo": Repo{ + Priority: 500, + Packages: []goolib.RepoSpec{ + {PackageSpec: &goolib.PkgSpec{Name: "foo_pkg", Version: "2.4.5@1", Arch: "noarch"}}, }, }, }, + wantVersion: "1.2.3@4", + wantArch: "noarch", + wantRepo: "high_priority_repo", }, - } - - table := []struct { - pkg string - arch string - wVer string - wArch string - wRepo string - }{ - {"foo_pkg", "noarch", "1.2.3@4", "noarch", "foo_repo"}, - {"foo_pkg", "", "1.2.3@4", "noarch", "foo_repo"}, - } - for _, tt := range table { - gotVer, gotRepo, gotArch, err := FindRepoLatest(goolib.PackageInfo{Name: tt.pkg, Arch: tt.arch, Ver: ""}, rm, archs) - if err != nil { - t.Fatalf("FindRepoLatest failed: %v", err) - } - if gotVer != tt.wVer { - t.Errorf("FindRepoLatest for %q, %q returned version: %q, want %q", tt.pkg, tt.arch, gotVer, tt.wVer) - } - if gotArch != tt.wArch { - t.Errorf("FindRepoLatest for %q, %q returned arch: %q, want %q", tt.pkg, tt.arch, gotArch, tt.wArch) - } - if gotRepo != tt.wRepo { - t.Errorf("FindRepoLatest for %q, %q returned repo: %q, want %q", tt.pkg, tt.arch, gotRepo, tt.wRepo) - } - } - - werr := "no versions of package bar_pkg.x86_64 found in any repo" - if _, _, _, err := FindRepoLatest(goolib.PackageInfo{Name: "bar_pkg", Arch: "x86_64", Ver: ""}, rm, archs); err.Error() != werr { - t.Errorf("did not get expected error: got %q, want %q", err, werr) + } { + t.Run(tt.desc, func(t *testing.T) { + gotVersion, gotRepo, gotArch, err := FindRepoLatest(tt.pi, tt.rm, tt.archs) + if err != nil && !tt.wantErr { + t.Fatalf("FindRepoLatest(%v, %v, %v) failed: %v", tt.pi, tt.rm, tt.archs, err) + } else if err == nil && tt.wantErr { + t.Fatalf("FindRepoLatest(%v, %v, %v) got nil error, wanted non-nil", tt.pi, tt.rm, tt.archs) + } + if gotVersion != tt.wantVersion { + t.Errorf("FindRepoLatest(%v, %v, %v) got version: %q, want %q", tt.pi, tt.rm, tt.archs, gotVersion, tt.wantVersion) + } + if gotArch != tt.wantArch { + t.Errorf("FindRepoLatest(%v, %v, %v) got arch: %q, want %q", tt.pi, tt.rm, tt.archs, gotArch, tt.wantArch) + } + if gotRepo != tt.wantRepo { + t.Errorf("FindRepoLatest(%v, %v, %v) got repo: %q, want %q", tt.pi, tt.rm, tt.archs, gotRepo, tt.wantRepo) + } + }) } } diff --git a/goolib/goospec_test.go b/goolib/goospec_test.go index 318174f..f1ee834 100644 --- a/goolib/goospec_test.go +++ b/goolib/goospec_test.go @@ -16,6 +16,7 @@ package goolib import ( "archive/tar" "bytes" + "fmt" "path/filepath" "reflect" "runtime" @@ -205,6 +206,38 @@ func TestBadCompare(t *testing.T) { } } +func TestComparePriorityVersion(t *testing.T) { + for _, tc := range []struct { + p1 int + v1 string + p2 int + v2 string + want int + }{ + {500, "1.2.3@1", 500, "1.2.3@2", -1}, + {500, "1.2.3@1", 500, "1.2.4@2", -1}, + {500, "1.2.4@1", 500, "1.2.3@2", 1}, + {500, "1.2.3", 500, "1.2.3", 0}, + {500, "1.2.3@1", 1000, "1.2.3@2", -1}, + {500, "1.2.3@1", 1000, "1.2.4@2", -1}, + {500, "1.2.4@1", 1000, "1.2.3@2", -1}, + {500, "1.2.3", 1000, "1.2.3", -1}, + {1000, "1.2.3@1", 500, "1.2.3@2", 1}, + {1000, "1.2.3@1", 500, "1.2.4@2", 1}, + {1000, "1.2.4@1", 500, "1.2.3@2", 1}, + {1000, "1.2.3", 500, "1.2.3", 1}, + } { + desc := fmt.Sprintf("ComparePriorityVersion(%v, %v, %v, %v)", tc.p1, tc.v1, tc.p2, tc.v2) + t.Run(desc, func(t *testing.T) { + if got, err := ComparePriorityVersion(tc.p1, tc.v1, tc.p2, tc.v2); err != nil { + t.Fatalf("%v: %v", desc, err) + } else if got != tc.want { + t.Fatalf("%v got: %v; want: %v", desc, got, tc.want) + } + }) + } +} + func TestWritePackageSpec(t *testing.T) { es := &PkgSpec{ Name: "test", From 3507071ef7d6ef9a3cf0edf499fa337a224d306f Mon Sep 17 00:00:00 2001 From: Phillip Nguyen Date: Thu, 4 Apr 2024 02:16:19 +0000 Subject: [PATCH 4/5] Name the TestComparePriorityVersion test cases. --- goolib/goospec_test.go | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/goolib/goospec_test.go b/goolib/goospec_test.go index f1ee834..532dcba 100644 --- a/goolib/goospec_test.go +++ b/goolib/goospec_test.go @@ -16,7 +16,6 @@ package goolib import ( "archive/tar" "bytes" - "fmt" "path/filepath" "reflect" "runtime" @@ -208,31 +207,31 @@ func TestBadCompare(t *testing.T) { func TestComparePriorityVersion(t *testing.T) { for _, tc := range []struct { + desc string p1 int v1 string p2 int v2 string want int }{ - {500, "1.2.3@1", 500, "1.2.3@2", -1}, - {500, "1.2.3@1", 500, "1.2.4@2", -1}, - {500, "1.2.4@1", 500, "1.2.3@2", 1}, - {500, "1.2.3", 500, "1.2.3", 0}, - {500, "1.2.3@1", 1000, "1.2.3@2", -1}, - {500, "1.2.3@1", 1000, "1.2.4@2", -1}, - {500, "1.2.4@1", 1000, "1.2.3@2", -1}, - {500, "1.2.3", 1000, "1.2.3", -1}, - {1000, "1.2.3@1", 500, "1.2.3@2", 1}, - {1000, "1.2.3@1", 500, "1.2.4@2", 1}, - {1000, "1.2.4@1", 500, "1.2.3@2", 1}, - {1000, "1.2.3", 500, "1.2.3", 1}, + {"same priority, same version, lesser release", 500, "1.2.3@1", 500, "1.2.3@2", -1}, + {"same priority lesser version", 500, "1.2.3@1", 500, "1.2.4@2", -1}, + {"same priority, greater version", 500, "1.2.4@1", 500, "1.2.3@2", 1}, + {"same priority, same version", 500, "1.2.3", 500, "1.2.3", 0}, + {"lesser priority, same version, lesser release", 500, "1.2.3@1", 1000, "1.2.3@2", -1}, + {"lesser priority, lesser version", 500, "1.2.3@1", 1000, "1.2.4@2", -1}, + {"lesser priority, greater version", 500, "1.2.4@1", 1000, "1.2.3@2", -1}, + {"lesser priority, same version", 500, "1.2.3", 1000, "1.2.3", -1}, + {"greater priority, same version, lesser release", 1000, "1.2.3@1", 500, "1.2.3@2", 1}, + {"greater priority, lesser version", 1000, "1.2.3@1", 500, "1.2.4@2", 1}, + {"greater priority, greater version", 1000, "1.2.4@1", 500, "1.2.3@2", 1}, + {"greater priority, same version", 1000, "1.2.3", 500, "1.2.3", 1}, } { - desc := fmt.Sprintf("ComparePriorityVersion(%v, %v, %v, %v)", tc.p1, tc.v1, tc.p2, tc.v2) - t.Run(desc, func(t *testing.T) { + t.Run(tc.desc, func(t *testing.T) { if got, err := ComparePriorityVersion(tc.p1, tc.v1, tc.p2, tc.v2); err != nil { - t.Fatalf("%v: %v", desc, err) + t.Fatalf("ComparePriorityVersion(%v, %v, %v, %v): %v", tc.p1, tc.v1, tc.p2, tc.v2, err) } else if got != tc.want { - t.Fatalf("%v got: %v; want: %v", desc, got, tc.want) + t.Fatalf("ComparePriorityVersion(%v, %v, %v, %v) got: %v; want: %v", tc.p1, tc.v1, tc.p2, tc.v2, got, tc.want) } }) } From d3903d812393f45f58d1791f79239db26daffed1 Mon Sep 17 00:00:00 2001 From: Phillip Nguyen Date: Wed, 3 Jul 2024 15:05:39 +0000 Subject: [PATCH 5/5] Check for same version already installed in updates code. If the already installed version is the exact same as the version available in a higher priority repo, then there is no reason to reinstall that version. Also added tests for the updates code. --- googet_test.go | 79 ++++++++++++++++++++++++++++++++++++++++++++++++ googet_update.go | 25 ++++++++++++--- 2 files changed, 99 insertions(+), 5 deletions(-) diff --git a/googet_test.go b/googet_test.go index 45def50..ca9a6c9 100644 --- a/googet_test.go +++ b/googet_test.go @@ -391,3 +391,82 @@ func TestCleanPackages(t *testing.T) { t.Errorf("cleanPackages did not remove notWantDir") } } + +func TestUpdates(t *testing.T) { + for _, tc := range []struct { + name string + pm packageMap + rm client.RepoMap + want []goolib.PackageInfo + }{ + { + name: "upgrade to later version", + pm: packageMap{ + "foo.x86_32": "1.0", + "bar.x86_32": "2.0", + }, + rm: client.RepoMap{ + "stable": client.Repo{ + Priority: 500, + Packages: []goolib.RepoSpec{ + {PackageSpec: &goolib.PkgSpec{Name: "foo", Version: "2.0", Arch: "x86_32"}}, + {PackageSpec: &goolib.PkgSpec{Name: "bar", Version: "2.0", Arch: "x86_32"}}, + }, + }, + }, + want: []goolib.PackageInfo{{Name: "foo", Arch: "x86_32", Ver: "2.0"}}, + }, + { + name: "rollback to earlier version", + pm: packageMap{ + "foo.x86_32": "2.0", + "bar.x86_32": "2.0", + }, + rm: client.RepoMap{ + "stable": client.Repo{ + Priority: 500, + Packages: []goolib.RepoSpec{ + {PackageSpec: &goolib.PkgSpec{Name: "foo", Version: "2.0", Arch: "x86_32"}}, + {PackageSpec: &goolib.PkgSpec{Name: "bar", Version: "2.0", Arch: "x86_32"}}, + }, + }, + "rollback": client.Repo{ + Priority: 1500, + Packages: []goolib.RepoSpec{ + {PackageSpec: &goolib.PkgSpec{Name: "foo", Version: "1.0", Arch: "x86_32"}}, + }, + }, + }, + want: []goolib.PackageInfo{{Name: "foo", Arch: "x86_32", Ver: "1.0"}}, + }, + { + name: "no change if rollback version already installed", + pm: packageMap{ + "foo.x86_32": "1.0", + }, + rm: client.RepoMap{ + "stable": client.Repo{ + Priority: 500, + Packages: []goolib.RepoSpec{ + {PackageSpec: &goolib.PkgSpec{Name: "foo", Version: "2.0", Arch: "x86_32"}}, + {PackageSpec: &goolib.PkgSpec{Name: "bar", Version: "2.0", Arch: "x86_32"}}, + }, + }, + "rollback": client.Repo{ + Priority: 1500, + Packages: []goolib.RepoSpec{ + {PackageSpec: &goolib.PkgSpec{Name: "foo", Version: "1.0", Arch: "x86_32"}}, + }, + }, + }, + want: nil, + }, + } { + t.Run(tc.name, func(t *testing.T) { + pi := updates(tc.pm, tc.rm) + if diff := cmp.Diff(tc.want, pi); diff != "" { + t.Errorf("update(%v, %v) got unexpected diff (-want +got):\n%v", tc.pm, tc.rm, diff) + } + }) + } +} diff --git a/googet_update.go b/googet_update.go index bf353d4..f70b66f 100644 --- a/googet_update.go +++ b/googet_update.go @@ -117,13 +117,28 @@ func updates(pm packageMap, rm client.RepoMap) []goolib.PackageInfo { logger.Error(err) continue } - if c == 1 { - fmt.Printf(" %s, %s --> %s from %s\n", p, ver, v, r) - logger.Infof("Update for package %s, %s installed and %s available from %s.", p, ver, v, r) - ud = append(ud, goolib.PackageInfo{Name: pi.Name, Arch: pi.Arch, Ver: v}) + if c < 1 { + logger.Infof("%s - highest priority version already installed", p) continue } - logger.Infof("%s - latest version installed", p) + // The versions might actually be the same even though the priorities are different, + // so do another check to skip reinstall of the same version. + c, err = goolib.Compare(v, ver) + if err != nil { + logger.Error(err) + continue + } + if c == 0 { + logger.Infof("%s - same version installed", p) + continue + } + op := "Upgrade" + if c == -1 { + op = "Downgrade" + } + fmt.Printf(" %s, %s --> %s from %s\n", p, ver, v, r) + logger.Infof("%s for package %s, %s installed and %s available from %s.", op, p, ver, v, r) + ud = append(ud, goolib.PackageInfo{Name: pi.Name, Arch: pi.Arch, Ver: v}) } return ud }