davecgh / go-spew

Implements a deep pretty printer for Go data structures to aid in debugging
ISC License
5.97k stars 361 forks source link

Google App Engine #35

Closed pjebs closed 9 years ago

pjebs commented 9 years ago

Is it possible to remove the unsafe package so that your package works with Google App Engine:

parser: bad import "unsafe" in github.com/davecgh/go-spew/spew/common.go

pjebs commented 9 years ago

Or if it is absolutely essential to have unsafe then put "// +build !appengine" at the top of the files and have some minimal code (just enough) so projects still compile when deployed to GAE even if functionally they don't do anything.

Your library works in local development mode in GAE. Just not in Deployment. Your library is very useful when in local development mode.

pjebs commented 9 years ago

Thanks in advance

dmitshur commented 9 years ago

Hi @pjebs,

Unsafe is definitely needed, but the library can operate in a limited fashion without it (on environments where unsafe is not available).

I had a similar need in my fork of go-spew, I wanted it to run in an environment without unsafe (it's not for Google App Enginge, but rather for compiling the Go code to JavaScript with GopherJS), so I've done this there. See https://github.com/shurcooL/go-goon/commit/0cea3a99a941622e45ada7254af0a0802b2b2d24.

A very similar change can be done to go-spew to support GAE.

dmitshur commented 9 years ago

I can make a PR to apply that kind of change to go-spew for @davecgh. Done so in #36. Feel free to use it if you like and find it helpful, or close it otherwise.

dmitshur commented 9 years ago

By the way, while unsafe is definitely used to a significant degree, I can't guarantee that it's impossible to implement unsafeReflectValue without it. It might be possible in another way. But I'll leave that for @davecgh.

davecgh commented 9 years ago

FYI. I was on vacation, so I'm not ignoring this issue, I just didn't have net access. I'll take a look at it over the next couple of days as I catch up on everything.

davecgh commented 9 years ago

It should be possible to make a version which works without unsafe, albeit in a limited fashion. There are several features which require the unsafe requirement. However, it should still be quite useful without it even if some of the more advanced features wouldn't work.

pjebs commented 9 years ago

Not sure for others, but in my case your package is not needed in deployment so there is no need to even make a 'limited version'. It's only needed in development, in which case unsafe works.

It's just currently annoying having to comment out every line that uses your package before deployment and then uncomment them again for development.

dmitshur commented 9 years ago

It's just currently annoying having to comment out every line that uses your package before deployment and then uncomment them again for development.

I don't think there's any way around that from go-spew's side.

Similarly, if you had a bunch of debugging fmt.Println statements in your development code, unless you comment it out, that code would run in production.

I suppose with fmt.Println you could put it in a helper func that acts normally in development but returns early in production, but you can't do that with go-spew since it will fail to compile in production as soon as it's imported.

davecgh commented 9 years ago

Agreed. That is why I think the best way to handle this is to have a limited version that compiles fine under GAE, just without the extra features that require unsafe.

pjebs commented 9 years ago

It won't fail To compile if you put "// +build !appengine" on all the files and have a brand new file with "// +build app engine" with all the currently exported functions etc which just return immediately and do nothing functionally.

davecgh commented 9 years ago

@pjebs: Yes, however, that would cause run-time panics because the code relies on the fact that the reflect values can be addressed and interfaced after calling those functions. If they are simply stubbed out, that would no longer be the case.

jrick commented 9 years ago

You could stub them out in your GAE project:

spew_appengine.go:

// +build appengine

func spewDump(args ...interface{}) {
}

spew_noappengine.go

// +build !appengine

func spewDump(args ...interface{}) {
        spew.Dump(args...)
}

Then use spewDump instead of spew.Dump in your project. Or throw them into your own spew package so you can just change the spew import path.

davecgh commented 9 years ago

As an update, I did most of the work last night to provide support for a limited mode which allow spew to build and operate without access to the unsafe package. There are still a couple of sections related to hex dumping that need to be completed and the tests need to be fixed to handle the different modes.

The approach I'm using is to set a constant at build time which specifies whether or not unsafe is available and moving all unsafe specific code into a separate file which is controlled by build tags. When unsafe is not available, the features which rely on it will be disabled (i.e the pointer method invocation bits will simply work as if the DisablePointerMethods flag is set, method invocation against unexported fields will work as if DisableMethods is set, hexdump conversion optimizations will be avoided, etc).

By default, unsafe will be disabled on Goggle App Engine, and I've also provided a disableunsafe build tag to manually control it.

davecgh commented 9 years ago

@pjebs: I don't have access to GAE at the moment to try it, but can you try with pr #38 and confirm it works?

pjebs commented 9 years ago

I'll take a look at it soon

pjebs commented 9 years ago

Tested and safe to merge

davecgh commented 9 years ago

Implemented by #38.

pjebs commented 9 years ago

I wonder if there is a way to enable 'unsafe' for GAE local development despite the !appengine build tag unfortunately taking precedence.

davecgh commented 9 years ago

Do you know if there is a build tag for local development mode?

pjebs commented 9 years ago

https://cloud.google.com/appengine/docs/go/reference#IsDevAppServer

davecgh commented 9 years ago

Thanks, but unfortunately that's just an API call so it can't be used at build time. The issue here is it needs to happen at build time.

pjebs commented 9 years ago

Perhaps there is a creative way of implementing it at run time?