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

cmd/go: tests failing on linux-amd64-longtest #31946

Closed
jayconrod opened this issue May 9, 2019 · 14 comments
Closed

cmd/go: tests failing on linux-amd64-longtest #31946

jayconrod opened this issue May 9, 2019 · 14 comments
Assignees
Milestone

Comments

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented May 9, 2019

Several tests are failing:

  • TestScript/mod_get_pseudo_other_branch
  • TestScript/mod_get_pseudo_prefix
  • TestScript/cover_pkgall_multiple_mains

https://build.golang.org/log/a96b4779e564bb3bae7d9495a6088b0d9018ad9c

@jayconrod jayconrod added this to the Go1.13 milestone May 9, 2019
@jayconrod jayconrod self-assigned this May 9, 2019
@jayconrod

This comment has been minimized.

Copy link
Contributor Author

@jayconrod jayconrod commented May 9, 2019

@cherrymui Could you take a look at the failure with go test cmd/go -run=TestScript/cover_pkgall_multiple_mains? A bisect indicates it was broken by CL 174657. For reference, the test case was added for CL 164877 and CL 168200. Those CLs fixed a link error that showed up when multiple main packages are linked into the same executable, which is possible with -coverpkg=all.

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented May 9, 2019

@jayconrod thanks for catching this.

I'm not very familiar with -coverpkg=all, so I'm not sure I understand how it supposes to work. Running go test -coverpkg=all -vet=off -x -work ./mainonly ./xtest (disabled vet for simplicity) in the tree of cover_pkgall_multiple_mains test, I got

...
cat >$WORK/b009/importcfg.link << 'EOF' # internal
packagefile example.com/cov/xtest.test=$WORK/b009/_pkg_.a
packagefile os=$WORK/b011/_pkg_.a
packagefile testing=$WORK/b025/_pkg_.a
packagefile testing/internal/testdeps=$WORK/b041/_pkg_.a
packagefile runtime=$WORK/b002/_pkg_.a
packagefile internal/cpu=$WORK/b004/_pkg_.a
packagefile internal/bytealg=$WORK/b003/_pkg_.a
packagefile runtime/internal/atomic=$WORK/b006/_pkg_.a
packagefile runtime/internal/sys=$WORK/b008/_pkg_.a
packagefile runtime/internal/math=$WORK/b007/_pkg_.a
packagefile example.com/cov/mainonly=$WORK/b054/_pkg_.a
packagefile internal/errinternal=$WORK/b013/_pkg_.a
packagefile internal/reflectlite=$WORK/b014/_pkg_.a
packagefile errors=$WORK/b012/_pkg_.a
packagefile internal/race=$WORK/b019/_pkg_.a
packagefile sync/atomic=$WORK/b020/_pkg_.a
packagefile sync=$WORK/b018/_pkg_.a
packagefile io=$WORK/b017/_pkg_.a
packagefile unicode=$WORK/b027/_pkg_.a
packagefile unicode/utf8=$WORK/b028/_pkg_.a
packagefile bytes=$WORK/b026/_pkg_.a
packagefile math/bits=$WORK/b034/_pkg_.a
packagefile math=$WORK/b033/_pkg_.a
packagefile strconv=$WORK/b035/_pkg_.a
packagefile reflect=$WORK/b032/_pkg_.a
packagefile sort=$WORK/b036/_pkg_.a
packagefile internal/fmtsort=$WORK/b031/_pkg_.a
packagefile internal/oserror=$WORK/b015/_pkg_.a
packagefile syscall=$WORK/b021/_pkg_.a
packagefile time=$WORK/b022/_pkg_.a
packagefile internal/poll=$WORK/b016/_pkg_.a
packagefile internal/syscall/unix=$WORK/b023/_pkg_.a
packagefile internal/testlog=$WORK/b024/_pkg_.a
packagefile strings=$WORK/b037/_pkg_.a
packagefile fmt=$WORK/b030/_pkg_.a
packagefile flag=$WORK/b029/_pkg_.a
packagefile runtime/debug=$WORK/b038/_pkg_.a
packagefile context=$WORK/b040/_pkg_.a
packagefile runtime/trace=$WORK/b039/_pkg_.a
packagefile example.com/cov/xtest=$WORK/b055/_pkg_.a
packagefile example.com/cov/xtest_test=$WORK/b056/_pkg_.a
packagefile bufio=$WORK/b042/_pkg_.a
packagefile regexp=$WORK/b043/_pkg_.a
packagefile runtime/pprof=$WORK/b045/_pkg_.a
packagefile regexp/syntax=$WORK/b044/_pkg_.a
packagefile compress/gzip=$WORK/b046/_pkg_.a
packagefile encoding/binary=$WORK/b048/_pkg_.a
packagefile io/ioutil=$WORK/b051/_pkg_.a
packagefile text/tabwriter=$WORK/b053/_pkg_.a
packagefile compress/flate=$WORK/b047/_pkg_.a
packagefile hash/crc32=$WORK/b049/_pkg_.a
packagefile path/filepath=$WORK/b052/_pkg_.a
packagefile hash=$WORK/b050/_pkg_.a
EOF
cd .
/Users/cherryyz/src/go/pkg/tool/darwin_amd64/link -o $WORK/b009/xtest.test -importcfg $WORK/b009/importcfg.link -s -w -buildmode=exe -buildid=cFSniVrc4KAyuUkCTFpY/TaDY2tf6UZUCfoMX5CGy/WvpKoGw2_XEdpH0o4Z9v/cFSniVrc4KAyuUkCTFpY -extld=clang $WORK/b009/_pkg_.a
# example.com/cov/xtest.test
2019/05/09 17:23:12 duplicate symbol main.main (types 1 and 1) in main and $WORK/b054/_pkg_.a(_go_.o)

This is linking xtest.test, but I don't understand why it links example.com/cov/mainonly into it (the packagefile example.com/cov/mainonly=$WORK/b054/_pkg_.a line above).

If linking this in is actually necessary, could the go command pass -p example.com/cov/mainonly when compiling mainonly.cover.go instead of -p main?

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented May 9, 2019

Experimented a little more, with a module m, which has three main packages, m1, m2, m3, each with test,

  • Doing go test -coverpkg=all ./... would build three binaries m1.test, m2.test, m3.test. Each of them is trying to link against other main packages, i.e. it is building m1.test with m2 and m3 (which compiled with -p main) linked in, which fails.
  • Without the -coverpkg=all, it builds three binaries, each of which is built without linking against other main packages, i.e. m2 and m3 are not linked into m1.test in non-coverpkg=all mode. This works fine.
  • Doing go test -coverpkg=m/m1,m/m2 ./..., it builds m1.test with m2 linked in but not m3. So apparently packages mentioned in the -coverpkg flag gets linked in.

I don't think we should link in other main packages, because they cannot be imported, even with -coverpkg=all, they are surely not covered.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 9, 2019

Change https://golang.org/cl/176417 mentions this issue: cmd/go: move two vcs test repos to vcs-test.golang.org

@jayconrod

This comment has been minimized.

Copy link
Contributor Author

@jayconrod jayconrod commented May 9, 2019

I don't think we should link in other main packages, because they cannot be imported, even with -coverpkg=all, they are surely not covered.

Each synthetic test main package (the actual main) imports all packages mentioned by the -coverpkg pattern. testing.RegisterCover gets called with the cover blocks from all these packages. When we compute the coverage percentage, these packages count, even though their code may be unreachable. Their inits may have side effects, too.

If linking this in is actually necessary, could the go command pass -p example.com/cov/mainonly when compiling mainonly.cover.go instead of -p main?

Maybe. I'll check on this tomorrow.

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented May 9, 2019

Each synthetic test main package (the actual main) imports all packages mentioned by the -coverpkg pattern. testing.RegisterCover gets called with the cover blocks from all these packages. When we compute the coverage percentage, these packages count, even though their code may be unreachable. Their inits may have side effects, too.

Thanks for the explanation. This seems weird to me, as -coverpkg could actually change the behavior of the test (e.g. from pass to fail). But I guess the decision has already been made...

gopherbot pushed a commit that referenced this issue May 10, 2019
Follow-up to CL 174061. This also fixes a break after GOSUMDB
was introduced.

Updates #31946
Updates #31673
Updates #31287
Updates #27171

Change-Id: I8e91e857f301b6b73cc90f2f2c68523412e22b46
Reviewed-on: https://go-review.googlesource.com/c/go/+/176417
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 10, 2019

Change https://golang.org/cl/176558 mentions this issue: cmd/go: force -coverpkg main packages to be built as libraries

@jayconrod

This comment has been minimized.

Copy link
Contributor Author

@jayconrod jayconrod commented May 10, 2019

@cherrymui Agreed, it's definitely weird. It looks like -p with the full import path fixes it though. I don't think any change needs to be made in pluginPath.

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented May 10, 2019

@jayconrod, thanks for the fix!

I don't think any change needs to be made in pluginPath.

I'm not sure why pluginPath is relevant here, though.

@jayconrod

This comment has been minimized.

Copy link
Contributor Author

@jayconrod jayconrod commented May 10, 2019

I initially thought pluginPath might be relevant, since it may be called to set the -p argument, and it was changed in CL 174657. I don't think it would ever be called for tests or coverage though.

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented May 10, 2019

Ok, thanks for the explanation. pluginPath is only used in buildmode=plugin, and is not used here.

@jayconrod

This comment has been minimized.

Copy link
Contributor Author

@jayconrod jayconrod commented May 28, 2019

@gopherbot Please open backport issue to 1.12

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 28, 2019

Backport issue(s) opened: #32295 (for 1.12).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jun 6, 2019

Change https://golang.org/cl/179677 mentions this issue: [release-branch.go1.12] cmd/go: force -coverpkg main packages to be built as libraries

gopherbot pushed a commit that referenced this issue Jun 7, 2019
…uilt as libraries

This fixes TestScript/cover_pkgall_multiple_mains, which started
failing after CL 174657.

When compiling main packages with coverage instrumentation
(e.g., for -coverpkg all), we now pass -p with the full import path
instead of '-p main'. This avoids link errors
'duplicate symbol main.main (types 1 and 1)'.

Fixes #32295
Updates #31946
Updates #32150

Change-Id: Id147527b1dbdc14bb33ac133c30d50c250b4365c
Reviewed-on: https://go-review.googlesource.com/c/go/+/176558
Run-TryBot: Jay Conrod <jayconrod@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit 3b8c804)
Reviewed-on: https://go-review.googlesource.com/c/go/+/179677
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.