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

x/net/http2/h2demo: has started to cause x/net builders to fail #34361

Closed
dmitshur opened this issue Sep 18, 2019 · 5 comments
Closed

x/net/http2/h2demo: has started to cause x/net builders to fail #34361

dmitshur opened this issue Sep 18, 2019 · 5 comments
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. 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
Unreleased

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Sep 18, 2019 •
edited
Loading

Issue #32528 has been resolved and all inner modules in x/net are being tested now, which includes the golang.org/x/net/http2/h2demo module. It contains a single package, which is a command with an // +build h2demo build constraint.

This is currently causing the x/net builders to report a failure, because the go test command reports a non-zero exit code when given an import path pattern that matches no packages:

h2demo $ go test golang.org/x/net/http2/h2demo/...
go: warning: "golang.org/x/net/http2/h2demo/..." matched no packages
no packages to test
h2demo $ echo $?
1

This makes x/net builders fail:

image

We have to fix x/net builders, but need to decide how.

Possible Solutions

My first suggestion would be to remove the h2demo build constraint. If I understand correctly, it was used when h2demo was created in 2015, before modules, and it was a probably a way to make it not appear in users' $GOPATH/bin when they did go get -u golang.org/x/net/... and to avoid its dependencies from being fetched.

The h2demo command was carved out into a separate module in #30685 to resolve the issue of it contributing too many dependencies to x/net. I think the build constraint isn't needed anymore, but let me know what you think.

An alternative solution is to come up with a way to allow a module with zero packages to not cause a test failure. But this option doesn't seem very good.

/cc @bradfitz @jayconrod
@dmitshur dmitshur added Testing An issue that has been verified to require only test changes, not just a test failure. Builders x/build issues (builders, bots, dashboards) NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations) labels Sep 18, 2019
@dmitshur dmitshur self-assigned this Sep 18, 2019
@gopherbot gopherbot added this to the Unreleased milestone Sep 18, 2019
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/196036 mentions this issue: http2/h2demo: remove h2demo build constraint

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/196097 mentions this issue: dashboard: skip testing x/net/http2/h2demo in GOPATH mode

@dmitshur
Copy link
Contributor Author

I prototyped the idea of removing the h2demo build constraint in CL 196036. It passed all trybots except 1.12.x one, because that one ran tests in GOPATH mode, and failed since h2demo imports some third-party packages. But that can be solved via CL 196097.

So, that solution seems to be viable. This issue still needs decision; I just wanted to check that removing the build constraint would actually work to fix the issue.

@bradfitz
Copy link
Contributor

SGTM

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Sep 18, 2019
gopherbot pushed a commit to golang/build that referenced this issue Sep 18, 2019
@dmitshur
dashboard: skip testing x/net/http2/h2demo in GOPATH mode
9bc86d7 
The h2demo command is an internal command that we use to
powers the https://http2.golang.org server, it is not a
package that others import.

It was never tested before golang.org/issue/34361 because it
had a +build h2demo constraint. But that build constraint is
being removed, so explicitly skip testing it in GOPATH mode.

The package is supported only in module mode now, since
it requires third-party dependencies.

Updates golang/go#34361

Change-Id: Ia3870baa10688b83f8400b1a5e13824a1c1b3d3b
Reviewed-on: https://go-review.googlesource.com/c/build/+/196097
Reviewed-by: Brad Fitzpatrick <[email protected]>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/198917 mentions this issue: [release-branch.go1.13] http2/h2demo: remove h2demo build constraint

gopherbot pushed a commit to golang/net that referenced this issue Oct 4, 2019
@dmitshur
[release-branch.go1.13] http2/h2demo: remove h2demo build constraint
13f9640 
The build constraint is no longer useful. It doesn't prevent this
package contributing module requirements to x/net, that was already
resolved by carving h2demo into its own module in golang/go#30685.
Few people do go get -u golang.org/x/net/... in GOPATH mode by now,
so there's no need to optimize for avoiding polluting GOPATH/bin.

Removing the build constraint allows the package to be visible and
tested by trybots and builders. It's also simpler.

Updates golang/go#34361

Change-Id: I84b5d70aab210ca8e4f5494160ae4d9049ef08ad
Reviewed-on: https://go-review.googlesource.com/c/net/+/196036
Run-TryBot: Dmitri Shuralyov <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Bryan C. Mills <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>
(cherry picked from commit a8b05e9)
Reviewed-on: https://go-review.googlesource.com/c/net/+/198917
codebien pushed a commit to codebien/build that referenced this issue Nov 13, 2019
@dmitshur @codebien
dashboard: skip testing x/net/http2/h2demo in GOPATH mode
937d175 
The h2demo command is an internal command that we use to
powers the https://http2.golang.org server, it is not a
package that others import.

It was never tested before golang.org/issue/34361 because it
had a +build h2demo constraint. But that build constraint is
being removed, so explicitly skip testing it in GOPATH mode.

The package is supported only in module mode now, since
it requires third-party dependencies.

Updates golang/go#34361

Change-Id: Ia3870baa10688b83f8400b1a5e13824a1c1b3d3b
Reviewed-on: https://go-review.googlesource.com/c/build/+/196097
Reviewed-by: Brad Fitzpatrick <[email protected]>
@golang golang locked and limited conversation to collaborators Oct 3, 2020
dteh pushed a commit to dteh/fhttp that referenced this issue Jun 22, 2022
@dmitshur
http2/h2demo: remove h2demo build constraint
b3b7a1a 
The build constraint is no longer useful. It doesn't prevent this
package contributing module requirements to x/net, that was already
resolved by carving h2demo into its own module in golang/go#30685.
Few people do go get -u golang.org/x/net/... in GOPATH mode by now,
so there's no need to optimize for avoiding polluting GOPATH/bin.

Removing the build constraint allows the package to be visible and
tested by trybots and builders. It's also simpler.

Fixes golang/go#34361

Change-Id: I84b5d70aab210ca8e4f5494160ae4d9049ef08ad
Reviewed-on: https://go-review.googlesource.com/c/net/+/196036
Run-TryBot: Dmitri Shuralyov <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Bryan C. Mills <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. 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
Unreleased
Development

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