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

Guard unsafe imports behind !appengine #948

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Hexcles
Copy link

@Hexcles Hexcles commented Apr 5, 2019

"unsafe" (in this case, imported via "golang.org/x/sys/unix") cannot be
used on AppEngine, so we need to add the "!appengine" build constraint
to files that use it (namely a few terminal_check_*.go).

Fixes #947.

terminal_check_unix.go Show resolved Hide resolved
terminal_check_unix.go Show resolved Hide resolved
terminal_check_windows.go Outdated Show resolved Hide resolved
@Hexcles
Copy link
Author

Hexcles commented Apr 15, 2019

@dgsb what do you reckon?

@dgsb
Copy link
Collaborator

dgsb commented May 18, 2019

@Hexcles did you try my suggestion to only add the !appengine build constraint in the terminal_check_unix.go file ?
I would also like that you merge master in your branch so that you can benefit a full cross compilation for all target supported by go 1.12 compiler in travis.

"unsafe" (in this case, imported via "golang.org/x/sys/unix") cannot be
used on AppEngine, so we need to add the "!appengine" build constraint
to terminal_check_unix.go.

Fixes sirupsen#947.
@Hexcles
Copy link
Author

Hexcles commented May 22, 2019

@dgsb sorry this thread has been going to my personal email which I haven't been keeping close track of.

I tried your suggestion on both Linux and macOS, and it does work. Only terminal_check_unix.go needs to be modified. I thought terminal_check_bsd.go would also need to be modified when I deployed from macOS, but apparently that's not the case from my experiment. It seems like the Go AppEngine toolchain is more like a crossbuild setup targeting Unix/Linux.

I also rebased the change on top of the current master.

@Hexcles
Copy link
Author

Hexcles commented Sep 4, 2019

ping @dgsb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

terminal_check_solaris.go doesn't have a build tag and is breaking the build on AppEngine
2 participants