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/api: run api check on all ports #53205

Open
prattmic opened this issue Jun 2, 2022 · 5 comments
Open

cmd/api: run api check on all ports #53205

prattmic opened this issue Jun 2, 2022 · 5 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Backlog

Comments

@prattmic
Copy link
Member

prattmic commented Jun 2, 2022 •
edited

cmd/api runs only on a fixed set of ports. The list is strange, excluding some first-class ports (linux/arm64, darwin/arm64), but including several secondary ports. Any ports excluded from this list get no checking at all.

Inspired by a suggestion from @bcmills, I think we should adjust behavior:

  • cmd/api should always include the current GOOS/GOARCH(/CGO_ENABLED).
  • The default context set should include all first-class ports.
  • cmd/dist should perhaps tell cmd/api to only check the current GOOS/GOARCH, to avoid duplicate work in builders.

These changes should make all first-class and secondary ports covered by api checks, in each of their respective builders (pre-submit or post-submit depending on which ports are included in TryBots, as usual).

cc @golang/release
@prattmic prattmic added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jun 2, 2022
@prattmic prattmic added this to the Backlog milestone Jun 2, 2022
@prattmic
Copy link
Member Author

prattmic commented Jun 2, 2022

  • cmd/dist should perhaps tell cmd/api to only check the current GOOS/GOARCH, to avoid duplicate work in builders.

dist skips the API test on android, ios, js, and plan9 because they are too slow. Those could perhaps be covered in one of the misc-compile builders? Or maybe a single api builder just runs every port?

@bcmills
Copy link
Contributor

bcmills commented Jun 2, 2022

We have a distinction between short and long tests. Maybe the short test (the default) should run the current GOOS/GOARCH, and the long test should also run every first-class GOOS/GOARCH?

(That would carve out the android, ios, js, and plan9 builders incidentally, because none of those are configured as -longtest.)

@bcmills
Copy link
Contributor

bcmills commented Jun 2, 2022

compare #14892 / CL 322309 (CC @iwdgo)

@bcmills
Copy link
Contributor

bcmills commented Jun 14, 2022

Those could perhaps be covered in one of the misc-compile builders? Or maybe a single api builder just runs every port?

Running them on the misc-compile builders sounds appropriate to me. IIRC the misc-compile builders also run go vet on their port, and the API check is conceptually very similar to a vet check.

@bcmills
Copy link
Contributor

bcmills commented Jun 15, 2022

For reference, cmd/coordinator currently has a special case for the api test:
https://cs.opensource.google/go/x/build/+/master:dashboard/builders.go;l=1217-1226;drc=59e7a6bb02dba3fc4a32b23dc98b96d04453d1e1

Probably a good first step would be to move that logic into the api test itself (having it skip itself based on GO_BUILDER_NAME) and removing that special case from the coordinator so that we can iterate on it without redeploying the coordinator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
2 participants
@prattmic @bcmills