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.

Sign up for GitHub

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

Jump to bottom

cmd/go: TestDocsUpToDate is incompatible with android emu builder environment #58775

Closed
dmitshur opened this issue Feb 27, 2023 · 6 comments
Closed

cmd/go: TestDocsUpToDate is incompatible with android emu builder environment #58775

dmitshur opened this issue Feb 27, 2023 · 6 comments
Assignees
@bcmills
Labels
FrozenDueToAge mobile Android, iOS, and x/mobile NeedsFix The path to resolution is known, but the work has not been done. OS-Android Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations) Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.21

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Feb 27, 2023 •
edited
Loading

CL 468636 (CC @bcmills) recently improved TestDocsUpToDate to invoke go help documentation directly, without the mkalldocs.sh script, but it's not able to run at least in the emu android builders, failing with:

--- FAIL: TestDocsUpToDate (0.00s)
    help_test.go:34:  help documentation: exec: no command
FAIL
exitcode=1
FAIL	cmd/go	0.550s

(https://build.golang.org/log/3cee601aeb5302bcf6b0304d14ef713c9f3bff76)

CC @golang/android.
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 27, 2023
@dmitshur dmitshur added this to the Backlog milestone Feb 27, 2023
@bcmills bcmills self-assigned this Feb 28, 2023
@bcmills bcmills added mobile Android, iOS, and x/mobile Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations) labels Feb 28, 2023
@dmitshur
Copy link
Contributor Author

The problem seems to be that testGo is only set when testenv.HasGoBuild() reports positively, but when GOOS=android it returns false. The test currently doesn't check testenv.HasGoBuild() nor testGo being non-empty.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 28, 2023
@bcmills
Copy link
Contributor

bcmills commented Feb 28, 2023

Agreed — I just noticed that too. Probably we should set testGo if testenv.HasExec is true, since not every Go command is go build.

Independently, I think there are some pretty deep problems with the way the go_android_exec wrapper is setting up the build environment. 😕

@dmitshur dmitshur added the Testing An issue that has been verified to require only test changes, not just a test failure. label Feb 28, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/472095 mentions this issue: cmd/go: set up testGo if the test can exec at all

@dmitshur dmitshur modified the milestones: Backlog, Go1.21 Feb 28, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/472096 mentions this issue: misc/android: rework GOROOT installation

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/472215 mentions this issue: internal/testenv: use 'go env CGO_ENABLED' instead of a build constraint

gopherbot pushed a commit that referenced this issue Mar 1, 2023
@gopherbot
internal/testenv: use 'go env CGO_ENABLED' instead of a build constraint
a5f3cb6 
A build constraint reports whether the test binary was compiled with
cgo enabled, but that doesn't necessarily imply that cgo can be used
in the environment in which the test binary is run.

In particular, cross-compiled builders (such as Android) may compile
the test binaries on the host with CGO enabled but not provide a C
toolchain on the device that runs the test.

For #58775.

Change-Id: Ibf2f44c9e956cd3fa898c3de67af4449e8ef2dd1
Reviewed-on: https://go-review.googlesource.com/c/go/+/472215
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/475456 mentions this issue: internal/testenv: allow 'go build' on android when supported

gopherbot pushed a commit that referenced this issue Apr 28, 2023
@gopherbot
misc/android: rework GOROOT installation
7bc8aac 
- Fall back to 'go env GOROOT' to locate GOROOT if runtime.GOROOT() is
  empty (as may be the case if the tool is built with -trimpath).

- Copy all of $GOROOT/android_$GOARCH/bin, not just cmd/go, to
  $GOROOT/bin.

- For consistency with CL 404134, place $GOROOT/bin at the beginning
  of $PATH, not the end.

- Don't use the install target for the "runtime" package to locate pkg/tool.
  As of Go 1.20 "runtime" doesn't have an install directory anyway.
  Since the real reason we need pkg/tool is for commands in "cmd",
  use an arbitrary command (namely "cmd/compile") to locate it.

- Use 'go list' to determine the package import path for the current
  directory, instead of assuming that it is within GOROOT or GOPATH.
  (That assumption does not hold in module mode.)

Updates #58775.

Change-Id: If76ff22bce76d05175c40678230f046a4aff0940
Reviewed-on: https://go-review.googlesource.com/c/go/+/472096
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Changkun Ou <mail@changkun.de>
Auto-Submit: Bryan Mills <bcmills@google.com>
gopherbot pushed a commit that referenced this issue Apr 28, 2023
@gopherbot
internal/testenv: allow 'go build' on android when supported
71ad46c 
As of CL 472096, it should work on android/arm64 always (because
internal linking is supported on that platform), and on other android
platforms when a C toolchain is present in the test environment.

Updates #58775.

Change-Id: Ifa38dc69b258b38dcc341979dcbf8cd61265c787
Reviewed-on: https://go-review.googlesource.com/c/go/+/475456
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Changkun Ou <mail@changkun.de>
@golang golang locked and limited conversation to collaborators Mar 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@bcmills bcmills

Labels
FrozenDueToAge mobile Android, iOS, and x/mobile NeedsFix The path to resolution is known, but the work has not been done. OS-Android Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations) Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
3 participants
@dmitshur @bcmills @gopherbot