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

Jump to bottom

cmd/gofmt: honor build tags, especially release tags #21337

Closed
dvyukov opened this issue Aug 8, 2017 · 12 comments
Closed

cmd/gofmt: honor build tags, especially release tags #21337

dvyukov opened this issue Aug 8, 2017 · 12 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Go1.12

Comments

@dvyukov
Copy link
Member

dvyukov commented Aug 8, 2017

go version go1.8.3 linux/amd64

I've pulled latest versions of all deps, including golang.org/x/net/context. Now 1.8 go fmt ./... fails with:

vendor/golang.org/x/net/context/go19.go:15:14: expected type, found '='
vendor/golang.org/x/net/context/go19.go:20:17: expected type, found '='

https://travis-ci.org/google/syzkaller/builds/262119828

Not sure what exactly part is guilty. But I guess it will affect a bunch of users.
Go1.9 is not released yet, and I am not sure when it will appear on travis-ci.org.
go fmt ignores build tags, which is reasonable.
1.8 go does not ignore vendor dirs.
I cannot not update packages because our cloud APIs continue to break public interfaces.

Any way we can resolve this?
@odeke-em odeke-em mentioned this issue Aug 8, 2017
Merged
@odeke-em
Copy link
Member

odeke-em commented Aug 8, 2017

@dvyukov for now, I've added for you a PR at google/syzkaller#329, please test it out on Travis CI and it should be able to run the tests against Go1.9rc1 as well as tip, instead of against 1.8.1 on which aliases are gonna trip out, and when Go1.9 is released then we can update from 1.9rc1 to 1.9.

odeke-em added a commit to orijtech/syzkaller that referenced this issue Aug 8, 2017
@odeke-em
.travis.yml: use go1.9rc1 since aliases trip gofmt for vendored code
3932809 
For golang/go#21337.

Since the introduction of aliases is in Go1.9 but Go1.9 hasn't
yet been officially released, let's use go1.9rc1 which is supported
on Travis CI by their Go version getter gimme
https://github.com/travis-ci/gimme
instead of against go1.8.1. This solves the problem on which
our vendored code is updated using Go1.9* syntax but is running
against Go1.8* in Travis CI tests.
dvyukov pushed a commit to google/syzkaller that referenced this issue Aug 8, 2017
@odeke-em @dvyukov
.travis.yml: use go1.9rc1 since aliases trip gofmt for vendored code
ec649f0 
For golang/go#21337.

Since the introduction of aliases is in Go1.9 but Go1.9 hasn't
yet been officially released, let's use go1.9rc1 which is supported
on Travis CI by their Go version getter gimme
https://github.com/travis-ci/gimme
instead of against go1.8.1. This solves the problem on which
our vendored code is updated using Go1.9* syntax but is running
against Go1.8* in Travis CI tests.
@bradfitz
Copy link
Contributor

bradfitz commented Aug 8, 2017

That's unfortunate. Even if we did a Go 1.8.x release for this, user also wouldn't get that.

So really the answer is for people to move to Go 1.9 if they use deps using Go 1.9 (even +build conditional Go 1.9) features I guess.

This is something to keep in mind for rolling out any future language changes.

/cc @ianlancetaylor @griesemer @robpike @broady @rsc

@dvyukov
Copy link
Member Author

dvyukov commented Aug 8, 2017

It seems that x/net/context added type aliases too early. I mean it's a widely used package and 1.9 is not even released.

@bradfitz
Copy link
Contributor

bradfitz commented Aug 8, 2017

I don't think so.

The whole point of +build tags is so we can do just that.

I think the bug here is that gofmt should respect +build go1.N tags at least, or at least least respect them enough to ignore them if the parse fails.

That is, if I'm running go1.14 and it's formatting a file with +build go1.16, it should try its best to format that file, unless it gets a parse error, in which case it should silently do nothing and not fail.

@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Aug 8, 2017
@griesemer
Copy link
Contributor

gofmt doesn't touch a file if it contains syntax errors, but I'm not convinced that it shouldn't complain. A script could trivially discard any error output. Or, perhaps for manual use, maybe there could be a -silent flag.

@dvyukov
Copy link
Member Author

dvyukov commented Aug 10, 2017

@griesemer you mean always running go fmt as: go fmt ./... 2>/dev/null || true?

@griesemer
Copy link
Contributor

@dvyukov Not always, but perhaps in situations where we don't care about errors.

dvyukov added a commit to dvyukov/syzkaller that referenced this issue Aug 10, 2017
@dvyukov
vendor/golang.org/x/net/context/: fix fmt with Go 1.8
b9a20ed 
See golang/go#21337
@dvyukov dvyukov mentioned this issue Aug 10, 2017
Merged
dvyukov added a commit to google/syzkaller that referenced this issue Aug 10, 2017
@dvyukov
vendor/golang.org/x/net/context/: fix fmt with Go 1.8
4bd2619 
See golang/go#21337
@sh0umik
Copy link

sh0umik commented Nov 14, 2017

Still exists in

go version go1.8 linux/amd64

Found this bug while generating swagger doc for my api

2017/11/14 12:20:03 Generate swagger docs....
2017/11/14 12:20:03 Generate general API Info
2017/11/14 12:20:03 ParseFile panic:vendor/golang.org/x/net/context/go19.go:15:14: expected type, found '=' (and 1 more errors)
panic: ParseFile panic:vendor/golang.org/x/net/context/go19.go:15:14: expected type, found '=' (and 1 more errors)

goroutine 1 [running]:
log.Panicf(0x7d359e, 0x13, 0xc421104fb8, 0x1, 0x1)
        /usr/local/go/src/log/log.go:329 +0xda
github.com/swaggo/swag.(*Parser).getAllGoFileInfo.func1(0xc4262c5b30, 0x27, 0x962040, 0xc4262bd6c0, 0x0, 0x0, 0x0, 0x0)
        /home/fahim/go/src/github.com/swaggo/swag/parser.go:228 +0x1a2
path/filepath.walk(0xc4262c5b30, 0x27, 0x962040, 0xc4262bd6c0, 0xc42025c220, 0x0, 0x0)
        /usr/local/go/src/path/filepath/path.go:351 +0x81
path/filepath.walk(0xc42625dea0, 0x1f, 0x962040, 0xc421c22680, 0xc42025c220, 0x0, 0x0)
        /usr/local/go/src/path/filepath/path.go:376 +0x414
path/filepath.walk(0xc42614d680, 0x17, 0x962040, 0xc421a86ea0, 0xc42025c220, 0x0, 0x0)
        /usr/local/go/src/path/filepath/path.go:376 +0x414
path/filepath.walk(0xc424a09c00, 0x13, 0x962040, 0xc4227cc820, 0xc42025c220, 0x0, 0x0)
        /usr/local/go/src/path/filepath/path.go:376 +0x414
path/filepath.walk(0xc424a09b60, 0x11, 0x962040, 0xc4227cc750, 0xc42025c220, 0x0, 0x0)
        /usr/local/go/src/path/filepath/path.go:376 +0x414
path/filepath.walk(0xc42017452a, 0x6, 0x962040, 0xc4202a7860, 0xc42025c220, 0x0, 0x0)
        /usr/local/go/src/path/filepath/path.go:376 +0x414
path/filepath.walk(0x7ce199, 0x2, 0x962040, 0xc4201204e0, 0xc42025c220, 0x0, 0x10)
        /usr/local/go/src/path/filepath/path.go:376 +0x414
path/filepath.Walk(0x7ce199, 0x2, 0xc42025c220, 0x1a, 0x0)
        /usr/local/go/src/path/filepath/path.go:398 +0x14c
github.com/swaggo/swag.(*Parser).getAllGoFileInfo(0xc42020c500, 0x7ce199, 0x2)
        /home/fahim/go/src/github.com/swaggo/swag/parser.go:234 +0x7b
github.com/swaggo/swag.(*Parser).ParseApi(0xc42020c500, 0x7ce199, 0x2, 0x7cfbfe, 0x9)
        /home/fahim/go/src/github.com/swaggo/swag/parser.go:59 +0xd2
github.com/swaggo/swag/gen.(*Gen).Build(0xc4201b1950, 0x7ce199, 0x2, 0x7cfbfe, 0x9, 0x0, 0x0)
        /home/fahim/go/src/github.com/swaggo/swag/gen/gen.go:27 +0x42a
main.main.func1(0xc420088840, 0x0, 0xc420088840)
        /home/fahim/go/src/github.com/swaggo/swag/cmd/swag/main.go:23 +0x55
github.com/urfave/cli.HandleAction(0x7557e0, 0x7e1f60, 0xc420088840, 0xc420270100, 0x0)
        /home/fahim/go/src/github.com/urfave/cli/app.go:502 +0xd4
github.com/urfave/cli.Command.Run(0x7ce7c4, 0x4, 0x0, 0x0, 0xc42025c100, 0x1, 0x1, 0x7d1bd5, 0xe, 0x0, ...)
        /home/fahim/go/src/github.com/urfave/cli/command.go:210 +0xb87
github.com/urfave/cli.(*App).Run(0xc420388000, 0xc42000c5c0, 0x2, 0x2, 0x0, 0x0)
        /home/fahim/go/src/github.com/urfave/cli/app.go:259 +0x7b7
main.main()
        /home/fahim/go/src/github.com/swaggo/swag/cmd/swag/main.go:29 +0x196

Also Duting go fmt

gateway/vendor/golang.org/x/net/context/go19.go:15:14: expected type, found '='
gateway/vendor/golang.org/x/net/context/go19.go:20:17: expected type, found '='
exit status 2
gateway/vendor/golang.org/x/net/ipv4/batch.go:59:14: expected type, found '='
gateway/vendor/golang.org/x/net/ipv4/batch.go:78:2: expected declaration, found 'if'
exit status 2
gateway/vendor/golang.org/x/net/ipv6/batch.go:56:14: expected type, found '='
gateway/vendor/golang.org/x/net/ipv6/batch.go:69:2: expected declaration, found 'if'
exit status 2

here what go19.go file looks like

// Copyright 2017 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

// +build go1.9

package context

import "context" // standard library's context, as of Go 1.7

// A Context carries a deadline, a cancelation signal, and other values across
// API boundaries.
//
// Context's methods may be called by multiple goroutines simultaneously.
type Context = context.Context

// A CancelFunc tells an operation to abandon its work.
// A CancelFunc does not wait for the work to stop.
// After the first call, subsequent calls to a CancelFunc do nothing.
type CancelFunc = context.CancelFunc

@ianlancetaylor
Copy link
Contributor

@sh0umik Yes, that specific problem with gofmt is fixed in the 1.9 release.

This issue is about whether gofmt should somehow honor build tags to avoid this kind of crash in the future.

@ianlancetaylor ianlancetaylor added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 14, 2017
@ianlancetaylor ianlancetaylor modified the milestones: Go1.10, Go1.11 Nov 14, 2017
@ianlancetaylor ianlancetaylor changed the title cmd/gofmt: fails on type aliases cmd/gofmt: honor build tags, especially release tags Nov 14, 2017
@bradfitz bradfitz modified the milestones: Go1.11, Go1.12 Jun 20, 2018
@rsc
Copy link
Contributor

rsc commented Oct 3, 2018

I don't think we should try to make gofmt or "go fmt" detect "files from the future" by parsing build tags. Typically you run gofmt on your own code.

@rsc rsc closed this as completed Oct 3, 2018
@noaos
Copy link

noaos commented Nov 4, 2018

I got this error today during make generate. My code is rebased on 8bd6bd63 ('prog: allow escaping paths but don't generate them').
`vendor/golang.org/x/net/context/go19.go:15:14: expected type, found '='
vendor/golang.org/x/net/context/go19.go:20:17: expected type, found '='
exit status 2
vendor/golang.org/x/net/http2/transport.go

vendor/google.golang.org/grpc/status/status.go
vm/qemu/qemu.go
make[1]: *** [format_go] Error 1
`

Any suggestion how to move on?
Thanks.

@dvyukov
Copy link
Member Author

dvyukov commented Nov 4, 2018

Hi Noa,

We require Go 1.9+ because of this, please upgrade. Also if you are going to do any changes, then 1.9 or 1.10, but not 1.11 because of breaking changes in gofmt output.

@golang golang locked and limited conversation to collaborators Nov 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
9 participants
@bradfitz @rsc @dvyukov @ianlancetaylor @odeke-em @griesemer @gopherbot @sh0umik @noaos