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.

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

Open
toothrot opened this issue Sep 5, 2019 · 4 comments

Comments

@toothrot
Copy link
Contributor

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: #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 label Sep 5, 2019

@toothrot toothrot added the NeedsFix label Sep 5, 2019

@toothrot toothrot self-assigned this Sep 5, 2019

@dmitshur

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

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

This comment has been minimized.

Copy link

commented Sep 12, 2019

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
app/appengine: automate the hiding of old release branches
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.