From ddf51e8cafeac05424839e79a96dc59d9124fc9b Mon Sep 17 00:00:00 2001 From: Matthew Manela Date: Mon, 13 May 2024 16:33:46 -0400 Subject: [PATCH 01/11] Add test to validate sha extraction for version from pseudo version --- internal/loader/loader_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/internal/loader/loader_test.go b/internal/loader/loader_test.go index 6e1f4aa..82cf1ad 100644 --- a/internal/loader/loader_test.go +++ b/internal/loader/loader_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/sourcegraph/scip-go/internal/config" + "golang.org/x/mod/module" "golang.org/x/tools/go/packages" ) @@ -39,6 +40,34 @@ func TestBuiltinFormat(t *testing.T) { } } +func TestPseudoVersion(t *testing.T) { + wd, _ := os.Getwd() + root, _ := filepath.Abs(filepath.Join(wd, "../../")) + pkgConfig := getConfig(root, config.IndexOpts{}) + pkgConfig.Tests = false + + pkgs, err := packages.Load(pkgConfig, "github.com/efritz/pentimento") + if err != nil { + t.Fatal(err) + } + + if len(pkgs) != 1 { + t.Fatalf("Too many packages: %s", pkgs) + } + + pkg := pkgs[0] + + if !module.IsPseudoVersion(pkg.Module.Version) { + t.Fatal("Package did not have a pseudo version: pre ensure") + } + + normalizePackage(&config.IndexOpts{}, pkg) + + if pkg.Module.Version != "ade47d831101" { + t.Fatal("Package pseudo-version was not extracted into a sha: post ensure") + } +} + func TestPackageWithinModule(t *testing.T) { wd, _ := os.Getwd() root, _ := filepath.Abs(filepath.Join(wd, "../../")) From 968cc9624a45d774d30291cfa704bfabbc3115a0 Mon Sep 17 00:00:00 2001 From: Matthew Manela Date: Mon, 13 May 2024 16:34:55 -0400 Subject: [PATCH 02/11] Add code to parse hash version from pseudo version --- internal/loader/loader.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/internal/loader/loader.go b/internal/loader/loader.go index 546b76c..a716629 100644 --- a/internal/loader/loader.go +++ b/internal/loader/loader.go @@ -10,6 +10,7 @@ import ( "github.com/sourcegraph/scip-go/internal/newtypes" "github.com/sourcegraph/scip-go/internal/output" "golang.org/x/mod/modfile" + "golang.org/x/mod/module" "golang.org/x/tools/go/packages" ) @@ -221,5 +222,22 @@ func normalizePackage(opts *config.IndexOpts, pkg *packages.Package) *packages.P } } + // Check if the module version is a pseudo-version. + // If it is, we will grab just the sha from it + if module.IsPseudoVersion(pkg.Module.Version) { + rev, err := module.PseudoVersionRev(pkg.Module.Version) + if err != nil { + // Only panic when running in debug mode. + fmt.Println(handler.ErrOrPanic( + "Unable to find rev from pseudo-version: %s %s", + pkg.Module.Path, + pkg.Module.Version, + )) + + } else { + pkg.Module.Version = rev + } + } + return pkg } From d2f5574dc2048aefd147c6c5979e968cfe76df39 Mon Sep 17 00:00:00 2001 From: Matthew Manela Date: Mon, 13 May 2024 22:11:46 -0400 Subject: [PATCH 03/11] Fix incorrect comment --- internal/git/infer_module_version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/git/infer_module_version.go b/internal/git/infer_module_version.go index cc2f312..4776ff2 100644 --- a/internal/git/infer_module_version.go +++ b/internal/git/infer_module_version.go @@ -8,7 +8,7 @@ import ( // InferModuleVersion returns the version of the module declared in the given // directory. This will be either the work tree commit's tag, or it will be the -// most recent tag with a short revhash appended to it. +// short revhash of the HEAD commit. func InferModuleVersion(dir string) (string, error) { version, err := command.Run(dir, "git", "tag", "-l", "--points-at", "HEAD") if err != nil { From e9a8b995be4ebcfa486dd8787c133c5cd0020fe1 Mon Sep 17 00:00:00 2001 From: Matthew Manela Date: Mon, 13 May 2024 22:12:05 -0400 Subject: [PATCH 04/11] Add another test for more permutations of versions --- internal/loader/loader_test.go | 58 +++++++++++++++++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-) diff --git a/internal/loader/loader_test.go b/internal/loader/loader_test.go index 82cf1ad..15674ea 100644 --- a/internal/loader/loader_test.go +++ b/internal/loader/loader_test.go @@ -40,7 +40,63 @@ func TestBuiltinFormat(t *testing.T) { } } -func TestPseudoVersion(t *testing.T) { +type normalizeTestCase struct { + Raw string + Normalized string +} + +func TestNormalizePackageModuleVersion(t *testing.T) { + cases := []normalizeTestCase{ + { + Raw: "v0.0.0-20180920160851-f15b22f93c73", + Normalized: "f15b22f93c73", + }, + { + Raw: "v0.3.1-0.20230414160720-beea233bdc0b", + Normalized: "beea233bdc0b", + }, + { + Raw: "v2.0.0-20180818164646-67afb5ed74ec", + Normalized: "67afb5ed74ec", + }, + { + Raw: "v1.1.1", + Normalized: "v1.1.1", + }, + { + Raw: "v1.0.0-beta.1", + Normalized: "v1.0.0-beta.1", + }, + { + Raw: "v0.0.0", + Normalized: "v0.0.0", + }, + { + Raw: "v2.0.0+incompatible", + Normalized: "v2.0.0+incompatible", + }, + { + Raw: "", + Normalized: ".", + }, + } + + for _, testCase := range cases { + pkg := &packages.Package{ + PkgPath: "github.com/fake_name/fake_module/fake_package", + Module: &packages.Module{ + Path: "github.com/fake_name/fake_module", + Version: testCase.Raw, + }, + } + normalizePackage(&config.IndexOpts{}, pkg) + if pkg.Module.Version != testCase.Normalized { + t.Errorf("Got: %s, Expected: %s", pkg.Module.Version, testCase.Normalized) + } + } +} + +func TestPackagePseudoVersion(t *testing.T) { wd, _ := os.Getwd() root, _ := filepath.Abs(filepath.Join(wd, "../../")) pkgConfig := getConfig(root, config.IndexOpts{}) From d076207a2c372ba6cac954e037583d9deb79c345 Mon Sep 17 00:00:00 2001 From: Matthew Manela Date: Mon, 13 May 2024 22:31:13 -0400 Subject: [PATCH 05/11] update snapshots --- .../output/generallyeric/new_operators.go | 14 +++++++------- .../replace-directives/replace_directives.go | 6 +++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/internal/testdata/snapshots/output/generallyeric/new_operators.go b/internal/testdata/snapshots/output/generallyeric/new_operators.go index 3cf43b6..ddc964a 100755 --- a/internal/testdata/snapshots/output/generallyeric/new_operators.go +++ b/internal/testdata/snapshots/output/generallyeric/new_operators.go @@ -2,19 +2,19 @@ // ^^^^^^^^^^^^^ reference 0.1.test `sg/generallyeric`/ import "golang.org/x/exp/constraints" -// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ reference golang.org/x/exp v0.0.0-20221205204356-47842c84f3db `golang.org/x/exp/constraints`/ +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ reference golang.org/x/exp 47842c84f3db `golang.org/x/exp/constraints`/ type Number interface { // ^^^^^^ definition 0.1.test `sg/generallyeric`/Number# // documentation ```go // documentation ```go constraints.Float | constraints.Integer | constraints.Complex -// ^^^^^^^^^^^ reference golang.org/x/exp v0.0.0-20221205204356-47842c84f3db `golang.org/x/exp/constraints`/ -// ^^^^^ reference golang.org/x/exp v0.0.0-20221205204356-47842c84f3db `golang.org/x/exp/constraints`/Float# -// ^^^^^^^^^^^ reference golang.org/x/exp v0.0.0-20221205204356-47842c84f3db `golang.org/x/exp/constraints`/ -// ^^^^^^^ reference golang.org/x/exp v0.0.0-20221205204356-47842c84f3db `golang.org/x/exp/constraints`/Integer# -// ^^^^^^^^^^^ reference golang.org/x/exp v0.0.0-20221205204356-47842c84f3db `golang.org/x/exp/constraints`/ -// ^^^^^^^ reference golang.org/x/exp v0.0.0-20221205204356-47842c84f3db `golang.org/x/exp/constraints`/Complex# +// ^^^^^^^^^^^ reference golang.org/x/exp 47842c84f3db `golang.org/x/exp/constraints`/ +// ^^^^^ reference golang.org/x/exp 47842c84f3db `golang.org/x/exp/constraints`/Float# +// ^^^^^^^^^^^ reference golang.org/x/exp 47842c84f3db `golang.org/x/exp/constraints`/ +// ^^^^^^^ reference golang.org/x/exp 47842c84f3db `golang.org/x/exp/constraints`/Integer# +// ^^^^^^^^^^^ reference golang.org/x/exp 47842c84f3db `golang.org/x/exp/constraints`/ +// ^^^^^^^ reference golang.org/x/exp 47842c84f3db `golang.org/x/exp/constraints`/Complex# } func Double[T Number](value T) T { diff --git a/internal/testdata/snapshots/output/replace-directives/replace_directives.go b/internal/testdata/snapshots/output/replace-directives/replace_directives.go index b4f462a..3df6f88 100755 --- a/internal/testdata/snapshots/output/replace-directives/replace_directives.go +++ b/internal/testdata/snapshots/output/replace-directives/replace_directives.go @@ -7,7 +7,7 @@ // ^^^ reference github.com/golang/go/src go1.22 fmt/ "github.com/dghubble/gologin" -// ^^^^^^^^^^^^^^^^^^^^^^^^^^^ reference github.com/sourcegraph/gologin v1.0.2-0.20181110030308-c6f1b62954d8 `github.com/dghubble/gologin`/ +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^ reference github.com/sourcegraph/gologin c6f1b62954d8 `github.com/dghubble/gologin`/ ) func Something() { @@ -16,7 +16,7 @@ fmt.Println(gologin.DefaultCookieConfig) // ^^^ reference github.com/golang/go/src go1.22 fmt/ // ^^^^^^^ reference github.com/golang/go/src go1.22 fmt/Println(). -// ^^^^^^^ reference github.com/sourcegraph/gologin v1.0.2-0.20181110030308-c6f1b62954d8 `github.com/dghubble/gologin`/ -// ^^^^^^^^^^^^^^^^^^^ reference github.com/sourcegraph/gologin v1.0.2-0.20181110030308-c6f1b62954d8 `github.com/dghubble/gologin`/DefaultCookieConfig. +// ^^^^^^^ reference github.com/sourcegraph/gologin c6f1b62954d8 `github.com/dghubble/gologin`/ +// ^^^^^^^^^^^^^^^^^^^ reference github.com/sourcegraph/gologin c6f1b62954d8 `github.com/dghubble/gologin`/DefaultCookieConfig. } From d731c72b60bd9b5c5aa7535d3c5fde6ca02b5dae Mon Sep 17 00:00:00 2001 From: Matthew Manela Date: Wed, 15 May 2024 15:42:03 -0400 Subject: [PATCH 06/11] PR feedback --- internal/loader/loader.go | 13 +++++++------ internal/loader/loader_test.go | 22 +++++++--------------- 2 files changed, 14 insertions(+), 21 deletions(-) diff --git a/internal/loader/loader.go b/internal/loader/loader.go index a716629..edff9ae 100644 --- a/internal/loader/loader.go +++ b/internal/loader/loader.go @@ -220,11 +220,13 @@ func normalizePackage(opts *config.IndexOpts, pkg *packages.Package) *packages.P pkg.Module.Version = "." } - } - - // Check if the module version is a pseudo-version. - // If it is, we will grab just the sha from it - if module.IsPseudoVersion(pkg.Module.Version) { + } else if module.IsPseudoVersion(pkg.Module.Version) { + // Check if the module version is a pseudo-version. + // If it is, we will grab just the sha from it + // Note: According to the go mod spec (https://go.dev/ref/mod#versions) we expect + // versions to always follow semantic versions (either tagged version or pseudo-version). + // Go tidy will ensure that the version is a valid semantic version + // and replace non-canonical versions with v0.0.0 rev, err := module.PseudoVersionRev(pkg.Module.Version) if err != nil { // Only panic when running in debug mode. @@ -233,7 +235,6 @@ func normalizePackage(opts *config.IndexOpts, pkg *packages.Package) *packages.P pkg.Module.Path, pkg.Module.Version, )) - } else { pkg.Module.Version = rev } diff --git a/internal/loader/loader_test.go b/internal/loader/loader_test.go index 15674ea..e3836d1 100644 --- a/internal/loader/loader_test.go +++ b/internal/loader/loader_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/sourcegraph/scip-go/internal/config" + "github.com/stretchr/testify/require" "golang.org/x/mod/module" "golang.org/x/tools/go/packages" ) @@ -90,9 +91,8 @@ func TestNormalizePackageModuleVersion(t *testing.T) { }, } normalizePackage(&config.IndexOpts{}, pkg) - if pkg.Module.Version != testCase.Normalized { - t.Errorf("Got: %s, Expected: %s", pkg.Module.Version, testCase.Normalized) - } + + require.Equal(t, testCase.Normalized, pkg.Module.Version) } } @@ -103,25 +103,17 @@ func TestPackagePseudoVersion(t *testing.T) { pkgConfig.Tests = false pkgs, err := packages.Load(pkgConfig, "github.com/efritz/pentimento") - if err != nil { - t.Fatal(err) - } + require.Nil(t, err) - if len(pkgs) != 1 { - t.Fatalf("Too many packages: %s", pkgs) - } + require.Equal(t, 1, len(pkgs), "Too many packages") pkg := pkgs[0] - if !module.IsPseudoVersion(pkg.Module.Version) { - t.Fatal("Package did not have a pseudo version: pre ensure") - } + require.True(t, module.IsPseudoVersion(pkg.Module.Version), "Package did not have a pseudo version: pre ensure") normalizePackage(&config.IndexOpts{}, pkg) - if pkg.Module.Version != "ade47d831101" { - t.Fatal("Package pseudo-version was not extracted into a sha: post ensure") - } + require.Equal(t, "ade47d831101", pkg.Module.Version, "Package pseudo-version was not extracted into a sha: post ensure") } func TestPackageWithinModule(t *testing.T) { From f55d84dfb4e8d1c1072bb1971c39a95c0eb34817 Mon Sep 17 00:00:00 2001 From: Matthew Manela Date: Thu, 16 May 2024 11:36:02 -0400 Subject: [PATCH 07/11] Handle stripping build suffix --- internal/loader/loader.go | 18 ++++++++++++------ internal/loader/loader_test.go | 2 +- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/internal/loader/loader.go b/internal/loader/loader.go index edff9ae..cab72c9 100644 --- a/internal/loader/loader.go +++ b/internal/loader/loader.go @@ -11,6 +11,7 @@ import ( "github.com/sourcegraph/scip-go/internal/output" "golang.org/x/mod/modfile" "golang.org/x/mod/module" + "golang.org/x/mod/semver" "golang.org/x/tools/go/packages" ) @@ -221,12 +222,11 @@ func normalizePackage(opts *config.IndexOpts, pkg *packages.Package) *packages.P pkg.Module.Version = "." } } else if module.IsPseudoVersion(pkg.Module.Version) { - // Check if the module version is a pseudo-version. - // If it is, we will grab just the sha from it - // Note: According to the go mod spec (https://go.dev/ref/mod#versions) we expect - // versions to always follow semantic versions (either tagged version or pseudo-version). - // Go tidy will ensure that the version is a valid semantic version - // and replace non-canonical versions with v0.0.0 + + // Unpublished versions of dependencies have pseudo-versions in go.mod. + // When the dependency itself is indexed, only the revision will be used. + // For correct cross-repo navigation to such dependencies, only use + // the revision from a pseudo-version. rev, err := module.PseudoVersionRev(pkg.Module.Version) if err != nil { // Only panic when running in debug mode. @@ -238,6 +238,12 @@ func normalizePackage(opts *config.IndexOpts, pkg *packages.Package) *packages.P } else { pkg.Module.Version = rev } + } else { + // The revision can also have build metadata following a `+`. Drop that, + // similar to official Go tooling. (https://go.dev/ref/mod#versions) + // > The build metadata suffix is ignored for the purpose of comparing versions + build := semver.Build(pkg.Module.Version) + pkg.Module.Version = strings.TrimSuffix(pkg.Module.Version, build) } return pkg diff --git a/internal/loader/loader_test.go b/internal/loader/loader_test.go index e3836d1..9fc10b2 100644 --- a/internal/loader/loader_test.go +++ b/internal/loader/loader_test.go @@ -74,7 +74,7 @@ func TestNormalizePackageModuleVersion(t *testing.T) { }, { Raw: "v2.0.0+incompatible", - Normalized: "v2.0.0+incompatible", + Normalized: "v2.0.0", }, { Raw: "", From f83190ee8bae173c2d2789368f8d5d4259af64f5 Mon Sep 17 00:00:00 2001 From: Matthew Manela Date: Thu, 16 May 2024 11:39:13 -0400 Subject: [PATCH 08/11] conditionalize stripping --- internal/loader/loader.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/loader/loader.go b/internal/loader/loader.go index cab72c9..add7b50 100644 --- a/internal/loader/loader.go +++ b/internal/loader/loader.go @@ -243,7 +243,9 @@ func normalizePackage(opts *config.IndexOpts, pkg *packages.Package) *packages.P // similar to official Go tooling. (https://go.dev/ref/mod#versions) // > The build metadata suffix is ignored for the purpose of comparing versions build := semver.Build(pkg.Module.Version) - pkg.Module.Version = strings.TrimSuffix(pkg.Module.Version, build) + if build != "" { + pkg.Module.Version = strings.TrimSuffix(pkg.Module.Version, build) + } } return pkg From e6c11ceb25cf359f18c696a4832cfce26f378070 Mon Sep 17 00:00:00 2001 From: Matthew Manela Date: Thu, 16 May 2024 11:44:39 -0400 Subject: [PATCH 09/11] Make it more idiomatic go --- internal/loader/loader.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/internal/loader/loader.go b/internal/loader/loader.go index add7b50..6e286d2 100644 --- a/internal/loader/loader.go +++ b/internal/loader/loader.go @@ -238,14 +238,8 @@ func normalizePackage(opts *config.IndexOpts, pkg *packages.Package) *packages.P } else { pkg.Module.Version = rev } - } else { - // The revision can also have build metadata following a `+`. Drop that, - // similar to official Go tooling. (https://go.dev/ref/mod#versions) - // > The build metadata suffix is ignored for the purpose of comparing versions - build := semver.Build(pkg.Module.Version) - if build != "" { - pkg.Module.Version = strings.TrimSuffix(pkg.Module.Version, build) - } + } else if build := semver.Build(pkg.Module.Version); build != "" { + pkg.Module.Version = strings.TrimSuffix(pkg.Module.Version, build) } return pkg From 3d91a322a3df144acbdd15530f7dba0de020e535 Mon Sep 17 00:00:00 2001 From: Matthew Manela Date: Thu, 16 May 2024 11:45:43 -0400 Subject: [PATCH 10/11] add back comment --- internal/loader/loader.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/loader/loader.go b/internal/loader/loader.go index 6e286d2..81a69d0 100644 --- a/internal/loader/loader.go +++ b/internal/loader/loader.go @@ -239,6 +239,9 @@ func normalizePackage(opts *config.IndexOpts, pkg *packages.Package) *packages.P pkg.Module.Version = rev } } else if build := semver.Build(pkg.Module.Version); build != "" { + // The revision can also have build metadata following a `+`. Drop that, + // similar to official Go tooling. (https://go.dev/ref/mod#versions) + // > The build metadata suffix is ignored for the purpose of comparing versions pkg.Module.Version = strings.TrimSuffix(pkg.Module.Version, build) } From 1f944b1f150adaaf53c833ce38275a4e60cf56f2 Mon Sep 17 00:00:00 2001 From: Matthew Manela Date: Thu, 16 May 2024 11:46:04 -0400 Subject: [PATCH 11/11] whitespace --- internal/loader/loader.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/loader/loader.go b/internal/loader/loader.go index 81a69d0..10f2ec6 100644 --- a/internal/loader/loader.go +++ b/internal/loader/loader.go @@ -222,7 +222,6 @@ func normalizePackage(opts *config.IndexOpts, pkg *packages.Package) *packages.P pkg.Module.Version = "." } } else if module.IsPseudoVersion(pkg.Module.Version) { - // Unpublished versions of dependencies have pseudo-versions in go.mod. // When the dependency itself is indexed, only the revision will be used. // For correct cross-repo navigation to such dependencies, only use