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

Fix ginkgo run with Golang 1.10 snap archive on Ubuntu 18.04 #505

Merged
merged 1 commit into from
Sep 13, 2018

Conversation

joestringer
Copy link
Contributor

Since Golang 1.10, this option should no longer be required:

https://golang.org/doc/go1.10#build

The old advice to add the -i flag for speed, as in go build -i or go
test -i, is no longer necessary: builds run just as fast without -i. For
more details, see go help cache.

Furthermore, it creates issues like the below when running ginkgo
against a snap-installed version of Golang 1.10 on Ubuntu 18.04:

$ ginkgo build
Compiling test...
Failed to compile test:

go test runtime/cgo: open /snap/go/2130/pkg/linux_amd64/runtime/cgo.a: read-only file system

Fixes: #504

@nodo
Copy link
Collaborator

nodo commented Aug 1, 2018

Thanks @joestringer !

I am a bit concerned in removing the -i flag, since we are currently supporting Go 1.6. Also, I have tried to reproduce the steps and seem very different from yours. Could this be an issue with Snap?

@joestringer
Copy link
Contributor Author

Hi @nodo!

I can definitely appreciate that, Golang 1.10 is fairly new.

For a bit more information, the maintainer of the Snap package described part of the issue here (some quotes below):

golang/go#24674 (comment)

For the snap packaging, I do know why runtime/cgo appears stale. It's fairly obscure though: the snap packaging builds runtime/cgo with particular C compiler flags to ensure that the generated .o files can be handled by older compilers. The go tool now keeps track of these, so unless you happen to use exactly the same flags, the go tool considers runtime/cgo to be stale.

Some others on the same thread suggested that people should just move away from -i, which makes sense if you can guarantee Golang 1.10.

Perhaps a better alternative would be to be able to parameterize this option rather than hardcoding it? By default, it could always add -i, then for users on newer Golang already they set some config option somewhere to disable the -i, or alias something like ginkgo=ginkgo --build-opts-i=false?

@ueokande
Copy link

ueokande commented Sep 6, 2018

Same issues occurs when Go modules introduced in Go1.11 is enabled. I used official Go compiler.
Ginkgo should detect go version and remove -i flag when Go 1.10 or later version is used.

@joestringer
Copy link
Contributor Author

@ueokande Perhaps then there's a better way that uses something like https://golang.org/pkg/runtime/#Version to detect and inject this at runtime.

/cc @nodo thoughts?

@ymmt2005
Copy link

ymmt2005 commented Sep 6, 2018

It would be better to use build constraints:
https://golang.org/pkg/go/build/#hdr-Build_Constraints

To have different build args for Go <1.10 and >=1.10, create these two files:

build_args.go:

// +build go1.10

var buildArgs = []string{"test", "-c"}

build_args_old.go:

// +build !go1.10

var buildArgs = []string{"test", "-c", "-i"}

and reference buildArgs like this:

func (t *TestRunner) BuildArgs(path string) []string {
    args := make([]string, len(buildArgs), len(buildArgs)+3)
    copy(args, buildArgs)
    args = append(args, "-o", path, t.Suite.Path)
    ...
}

@joestringer
Copy link
Contributor Author

Thanks @ymmt2005, I used your approach.

@joestringer joestringer force-pushed the fix-golang-1.10 branch 4 times, most recently from fdd819d to 92f66eb Compare September 6, 2018 20:59
Since Golang 1.10, this option should no longer be required:

https://golang.org/doc/go1.10#build

> The old advice to add the -i flag for speed, as in go build -i or go
test -i, is no longer necessary: builds run just as fast without -i. For
more details, see go help cache.

Furthermore, it creates issues like the below when running ginkgo
against a snap-installed version of Golang 1.10 on Ubuntu 18.04:

  $ ginkgo build
  Compiling test...
  Failed to compile test:

  go test runtime/cgo: open /snap/go/2130/pkg/linux_amd64/runtime/cgo.a: read-only file system
@nodo
Copy link
Collaborator

nodo commented Sep 7, 2018

Thanks all, it makes sense to me! I will have a more close look later today.

@joestringer
Copy link
Contributor Author

@nodo did you get a chance to take a look?

@nodo
Copy link
Collaborator

nodo commented Sep 13, 2018

Hey @joestringer - looks good to me I have a few doubts about the testing, but I cannot think about a better solution for now 😄

Thanks a lot for contributing! 💯

@nodo nodo merged commit 46bbc26 into onsi:master Sep 13, 2018
@joestringer joestringer deleted the fix-golang-1.10 branch September 13, 2018 20:59
@joestringer
Copy link
Contributor Author

@nodo Thanks! I agree it's not the most elegant, but I couldn't think of something better.

Fortunately I think if the testing changes significantly and this breaks, it should err on the side of "breaking when it shouldn't" rather than "hiding a breakage when it should break".

@ymmt2005
Copy link

Thanks!

Just for your information, you can use buildArgs to construct the expected value in testing.

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

Successfully merging this pull request may close these issues.

4 participants