Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/exp/cmd/gorelease: cannot process package _test files #44440

Open
carnott-snap opened this issue Feb 19, 2021 · 7 comments
Open

x/exp/cmd/gorelease: cannot process package _test files #44440

carnott-snap opened this issue Feb 19, 2021 · 7 comments

Comments

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Feb 19, 2021

When I add a test file like:

package my_test
//...

in a sample module:

module github.com/user/repo
// ...

gorelease now starts failing:

$ make gorelease
github.com/user/repo
------------------------------------
Incompatible changes:
- ErrXxx: removed
- Yyy: removed

Inferred base version: v1.2.1
go.mod: the following requirements are needed
        github.com/user/repo@v1.1.0
Run 'go mod tidy' to add missing requirements.
make: *** [Makefile:104: gorelease] Error 1

I can confirm that I did not remove Yyy or ErrXxx, and that the the rest of the toolchain can find them, e.g. go doc Yyy. This may end up being a bug in apidiff, but I have not dug into the root cause.

@gopherbot gopherbot added this to the Unreleased milestone Feb 19, 2021
@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Mar 2, 2021

if you want a repro; iiuc, the Incompatible changes block from my redacted example is just fallout from the root cause, thus it is not in the repro.

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Mar 3, 2021

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Apr 6, 2021

Not sure what the cause is, but I was able to reproduce this at d352d2db2ceb4c0411268ee2de764e3f82e27280.

The message comes from gorelease itself when loading packages, and it happens with -base=none, so I don't think apidiff is involved.

cc @jadekler

@jadekler
Copy link
Member

@jadekler jadekler commented Apr 7, 2021

(will also try to debug this friday if nobody gets there before me!)

@jadekler
Copy link
Member

@jadekler jadekler commented Apr 10, 2021

I did some debugging and found the cause: cyclic imports are not considered in prepareLoadDir's go.mod / go.sum comparison sections. The following hacky change fixes the issue:

deklerk at deklerk1 in ~/workspace/exp/cmd/gorelease on master*
$ git diff
diff --git a/cmd/gorelease/gorelease.go b/cmd/gorelease/gorelease.go
index 3e09351..54bdac7 100644
--- a/cmd/gorelease/gorelease.go
+++ b/cmd/gorelease/gorelease.go
@@ -80,6 +80,7 @@
 package main

 import (
+       "bufio"
        "bytes"
        "encoding/json"
        "errors"
@@ -1018,6 +1019,9 @@ func prepareLoadDir(modFile *modfile.File, modPath, modRoot, version string, cac
                }
                lines := make([]string, len(modFile.Require))
                for i, req := range modFile.Require {
+                       if req.Mod.Path == modPath {
+                               continue
+                       }
                        lines[i] = req.Mod.String()
                }
                sort.Strings(lines)
@@ -1068,7 +1072,24 @@ func prepareLoadDir(modFile *modfile.File, modPath, modRoot, version string, cac
                        // "empty go.sum".
                }

-               if bytes.Compare(goSumData, newGoSumData) != 0 {
+               var oldSum, newSum string
+               scanner := bufio.NewScanner(bytes.NewReader(goSumData))
+               for scanner.Scan() {
+                       line := scanner.Text()
+                       if strings.Contains(line, modPath) {
+                               continue
+                       }
+                       oldSum += line
+               }
+               scanner = bufio.NewScanner(bytes.NewReader(newGoSumData))
+               for scanner.Scan() {
+                       line := scanner.Text()
+                       if strings.Contains(line, modPath) {
+                               continue
+                       }
+                       newSum += line
+               }
+               if newSum != oldSum {
                        diagnostics = append(diagnostics, "go.sum: one or more sums are missing. Run 'go mod tidy' to add missing sums.")
                }
        }

I actually ran into this independently in https://go-review.googlesource.com/c/exp/+/288032, funnily enough.


Why is this happening:

In the provided repro, there's a module cycle:

$ go mod graph
gist.github.com/c2a4204c76b6ee2956c0f3c855e593d4.git gist.github.com/ac489642a42c01530c15516160048aea.git@v1.0.0
gist.github.com/c2a4204c76b6ee2956c0f3c855e593d4.git golang.org/x/exp@v0.0.0-20210220032938-85be41e4509f
gist.github.com/ac489642a42c01530c15516160048aea.git@v1.0.0 gist.github.com/c2a4204c76b6ee2956c0f3c855e593d4.git@v1.0.0
[...]

So, in copyModuleToTempDir, we create a temp version of module gist.github.com/c2a4204c76b6ee2956c0f3c855e593d4.git with version v0.0.0-gorelease (here.

Then later in prepareLoadDir, we depend on this v0.0.0-gorelease version (here and here).

Well, when we run go get -d ., it upgrades the v0.0.0-gorelease to v1.0.0, presumably because that's the latest version in the mod graph cycle. (maybe different reason? that's my guess) (see here)

So anyways, the code above basically ignores anything with the same modPath as the base module when doing the go.mod and go.sum missing checks.

I'll work on getting a test out for this, and a better solution.


@jayconrod , let me know if you can think of a nicer way to tackle this than my proposal above. It's late on a Friday so I may be missing something obvious. :)

@jadekler jadekler self-assigned this Apr 10, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 15, 2021

Change https://golang.org/cl/310369 mentions this issue: cmd/gorelease: skip circular imports when inspect go.sum and go.mod differences

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 16, 2021

Change https://golang.org/cl/310809 mentions this issue: cmd/gorelease: skip circular imports when inspect go.sum and go.mod differences

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants