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/build: make misc-compile-all trybot type check tests too #20119

Open
bradfitz opened this Issue Apr 25, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@bradfitz
Member

bradfitz commented Apr 25, 2017

The "misc-compile-*" Trybot builders try to at least compile & type check every GOOS/GOARCH pair, even when we don't have sufficient hardware to run the tests.

But they apparently don't compile or type check tests.

https://go-review.googlesource.com/c/41691/ broke Plan 9 without any warnings.

Fix that.

/cc @mvdan @kevinburke @adams-sarah @cybrcodr

@gopherbot gopherbot added this to the Unreleased milestone Apr 25, 2017

@gopherbot gopherbot added the Builders label Apr 25, 2017

@gopherbot

This comment has been minimized.

gopherbot commented Apr 25, 2017

CL https://golang.org/cl/41752 mentions this issue.

gopherbot pushed a commit that referenced this issue Apr 25, 2017

net/http/cgi: fix plan9 build
Cleanup CL https://golang.org/cl/41691 broke the plan9 build by removing
a use of a package but not removing the package import.

Trybots don't check that. I filed #20119 for that.

Change-Id: Ia030e6924665dfb871ca964455b899d51b0200c2
Reviewed-on: https://go-review.googlesource.com/41752
Reviewed-by: David du Colombier <0intro@gmail.com>
@bradfitz

This comment has been minimized.

Member

bradfitz commented May 2, 2017

Notes:

The misc builders are defined here:
https://github.com/golang/build/blob/60446e188e72edd4658dc6d9f6e3c1b9f5227014/dashboard/builders.go#L809

They use buildall.bash to run: (ignore the typo in docs that says buildall.sh)
https://github.com/golang/build/blob/60446e188e72edd4658dc6d9f6e3c1b9f5227014/dashboard/builders.go#L578
https://github.com/golang/go/blob/master/src/buildall.bash

All that basically does (from a fast linux-amd64 kubernetes container) is:

$ GOOS=something GOARCH=something build -a std cmd

We'd also need a way to also make buildall.bash run "go test -c $PKG" for each $PKG. That is #15513.

@paranoiacblack

This comment has been minimized.

Contributor

paranoiacblack commented May 2, 2017

Yeah, I tried just modifying buildall.bash to run $GOPATH/bin/go tool dist test --compile-only --rebuild to avoid having to particularly solve #15513. However, it seems that --compile-only does not seem to actually compile the tests? I tried reintroducing the same error you removed in https://golang.org/cl/41752 but --compile-only passes just fine. Maybe I'll just go fix #15513 instead, though, since it would make this trivial.

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 2, 2017

Note that dist test's --compile-only is not a promise but just a hint from the builders as an optimization to skip tests if convenient. It would be a bit more work to make sure --compile-only had stronger guarantees.

Alternatively, buildall.bash could just loop over $(go list std cmd) and run go test -c $PKG

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 2, 2017

Packages with tests:

$ for PKG in $(go list -f "{{.ImportPath}} {{.TestGoFiles}}" std cmd | \
   grep -v ' \[\]$' | perl -npe 's/ .+//'); do \
       echo $PKG; \
   done
@paranoiacblack

This comment has been minimized.

Contributor

paranoiacblack commented May 2, 2017

Until #15513 is resolved, would it be okay to use the go test -run=^$ std cmd workaround? Since ideally, go test -c std cmd should just work anyways.

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 2, 2017

@paranoiacblack, no, because buildall.bash always runs on linux-amd64 and go test -run runs the (cross-) compiled binary.

@paranoiacblack paranoiacblack self-assigned this May 3, 2017

@gopherbot

This comment has been minimized.

gopherbot commented May 3, 2017

CL https://golang.org/cl/42531 mentions this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment