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/build/app/appengine: local development server panics on pageload #34116

Closed
toothrot opened this issue Sep 5, 2019 · 6 comments
Closed

x/build/app/appengine: local development server panics on pageload #34116

toothrot opened this issue Sep 5, 2019 · 6 comments
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@toothrot
Copy link
Contributor

toothrot commented Sep 5, 2019

Issue

Running dev_appserver.py locally causes the following issue:

$ dev_appserver.py  app.yaml
...
panic: open app/appengine/perf_graph.html: no such file or directory

goroutine 1 [running]:
html/template.Must(0x0, 0xbbcd80, 0xc0002ce060, 0x1)
	/home/rakoczy/.goenv/versions/1.13rc1/src/html/template/template.go:372 +0x54
main.init()
	/home/rakoczy/development/build/app/appengine/perf_graph.go:240 +0x6c4
panic: open app/appengine/perf_graph.html: no such file or directory

Cause

This was caused by a remediation for go111 module mode in app engine, which calculates repository paths on upload based on the location of the go.mod file in the repository.

See: golang/build@c073844

Suggested fix

To workaround dev_appserver.py's differences with App Engine, we could detect if the file exists in app/appengine before falling back to a local file (for example, app/appengine/perf_graph.html and perf_graph.html as the fallback).

Ideally this is temporary until either we can easily create a nested module with AppEngine (see alternative 1 below), or until a fix for dev_appserver.py is completed.

Alternatives

  1. Create a nested module in app/appengine/go.mod
    This would involve all the complications of migrating to a nested module: cmd/go: setting up a multi-module single repo is difficult #27056 (comment).
    I've explored this a bit. The biggest issue is that AppEngine doesn't currently respect rewrite rules at upload time, which means that adding the local dependency for golang.org/x/build requires a potentially non-buildable commit to be merged, followed by a correction commit.

  2. Move app/appengine/app.yaml to the root of x/build, and supply a main directive that points to app/appengine.
    Making this concession seems at odds with the organization of the x/build repo.

  3. Move to Cloud Run and Docker.

See #34107, which discusses deployment time issues with GOPATH vs module mode.

/cc @dmitshur @andybons
@gopherbot gopherbot added this to the Unreleased milestone Sep 5, 2019
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Sep 5, 2019
@toothrot toothrot added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 5, 2019
@toothrot toothrot self-assigned this Sep 5, 2019
@dmitshur
Copy link
Contributor

dmitshur commented Sep 5, 2019 •
edited

Thanks for describing the issue in detail.

The suggested fix sounds very reasonable to me. It seems better compared to alternatives 1 and 2. Alternative 3 is also a good one.

I want to offer another alternative for us to consider (either as a solution now, or a long term direction to move in):

Another way to resolve this problem is to move away from using dev_appserver.py for local development, and instead build and run the Go program normally (e.g., go run golang.org/x/build/app/appengine).

In the "Migrating your App Engine app from Go 1.9 to Go 1.11" document, one of the described changes is that it's possible to write an App Engine app that's more like non-App Engine apps, meaning it has a package main with func main() in it. (See https://cloud.google.com/appengine/docs/standard/go111/go-differences#package-main.) It just needs to os.Getenv("PORT") and use that for its HTTP server, but otherwise it can behave like a normal Go command.

We already do this for some projects that need to be deployed to App Engine but also run locally. For example, the tour. See CL 165537.

Some disadvantages of this approach:

  • it might be more work compared to the suggested fix
  • I'm not very familiar with the benefits that being able to use dev_appserver.py offer, but if there are some, we'd lose them

It's worth considering our longer-term goal of merging the build dashboard code into cmd/coordinator, as that work would make this problem obsolete.

@toothrot
Copy link
Contributor Author

toothrot commented Sep 5, 2019

Great point, I forgot about that option! I think that moving off dev_appserver.py using that guide could be the best fix, and I'll look into it first. If it's too much work, it might be worth doing the longer-term goal of merging the build dashboard into cmd/coordinator.

@dmitshur
Copy link
Contributor

To workaround dev_appserver.py's differences with App Engine, we could detect if the file exists in app/appengine before falling back to a local file (for example, app/appengine/perf_graph.html and perf_graph.html as the fallback).

It seems CL 194643 implements this, just with an inverse order of the lookups. /cc @bradfitz

@gopherbot
Copy link

Change https://golang.org/cl/194643 mentions this issue: app/appengine: automate the hiding of old release branches

gopherbot pushed a commit to golang/build that referenced this issue Sep 12, 2019
@bradfitz
app/appengine: automate the hiding of old release branches
9c46470 
Also, because this is the first CL in this package to add tests, add a
helper to return the right template filename depending on the
environment (go test vs prod) so tests don't panic in init.

Fixes golang/go#34097
Updates golang/go#34116

Change-Id: I4b3e83c2417611cfbdc32e27941dbb90687eb509
Reviewed-on: https://go-review.googlesource.com/c/build/+/194643
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@dmitshur
Copy link
Contributor

I believe this issue is resolved, see #34116 (comment). @toothrot Do you concur?

@toothrot
Copy link
Contributor Author

I think we can consider this resolved. I'll file a new bug to move off dev_appserver.py, so we can run this as a normal go server.

codebien pushed a commit to codebien/build that referenced this issue Nov 13, 2019
@bradfitz @codebien
app/appengine: automate the hiding of old release branches
476dac9 
Also, because this is the first CL in this package to add tests, add a
helper to return the right template filename depending on the
environment (go test vs prod) so tests don't panic in init.

Fixes golang/go#34097
Updates golang/go#34116

Change-Id: I4b3e83c2417611cfbdc32e27941dbb90687eb509
Reviewed-on: https://go-review.googlesource.com/c/build/+/194643
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@golang golang locked and limited conversation to collaborators Sep 25, 2020
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.
Projects
None yet
Milestone
Unreleased
Development

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