add tags and i options to go build

[drewwells]

Adds some arguments to go build fixes #503

go build supported flags:

• tags: The -i flag installs the packages that are dependencies of the target.
• i: a list of build tags to consider satisfied during the build

Unsupported

• race: enable data race detection.
• v: print the names of packages as they are compiled.
• x: print the commands.

These are useful, but highly project specific. It would be difficult to make these flags useful in a general case.

• ldflags
• asmflags

[drewwells]

This needs some work, the syntax is not being sent to go build correctly

[swsnr]

The argument is go-build, not go here.

[swsnr]

@drewwells I'm not sure that I understand -i correctly. Does it really cause go build to download and install all dependencies?

[drewwells]

Say you're dependent on a package that takes 10mins to build. -i installs this package to $GOPATH/pkg. If you depend on this package, the compiled package is used and is linked instantly rather than taking 10mins to build. For instance,

a depends on b

go build -i will rebuild a from source, and in the process install b into $GOPATH/pkg

-i is named after go install which installs a top level package (cli tool or just library) into $GOPATH/pkg. As you can imagine, this is really awesome for local development.

[swsnr]

@drewwells Sorry, but that doesn't really answer my question. So again, does go build -i download and build dependencies if they are missing, instead of just failing?

[drewwells]

No, go build -i compiles local packages and puts them in $GOPATH/pkg. It does not download source from the internet, go get and/or go install download packages from the internet.

[drewwells]

any tips on fixing this? The expected output is go build -i -tags "tag1 tag2 tag3". It doesn't appear to be creating the cli arguments correctly.

[swsnr]

@drewwells I still don't understand what we need -i for. How does it help Flycheck that the current package is being installed to GOPATH?

[drewwells]

Building packages can take a long time. This installs dependent package so they do not need to be rebuilt. It automatically tracks changes and invalidates cache intelligently as well. Since a lot of useful Go packages depend on C libs, this can dramatically speed up build times.

[swsnr]

@drewwells If you want to pass all tags as a single argument you need to use option, not option-list. Please read the documentation for each of these symbols in flycheck-substitute-argument. Take a look at go-vet for an example of concatenating a list into a single argument.

As for -i: If I use go build -i on a package foo, the package foo is installed to GOPATH, isn't it?

[drewwells]

yes, it will install the top level package too. go build -i and go install are equivalent for the top level package.

[swsnr]

Why did you change this comment?

[drewwells]

Updated for the new location of this issue.

[swsnr]

@drewwells I have no idea what the “top level package” is supposed to be, but regardless I strongly dislike the idea of having an automatic background syntax check changing the global state of GOPATH. Flycheck should never alter anything in your development and build environment. That's neither its job nor its purpose and it's bound to cause confusion.

I'll merge the tags part of this PR, but I'm very skeptical about the -i flag. Please remove the -i part from this one, so that I can merge the tags stuff, and—if you insist—open a separate PR for -i, on which we can continue the discussion about that flag. At least, I'd collect feedback from all those that have hitherto contributed to the go checker before merging a flag which I'm not convinced of.

[drewwells]

When you are building code, the top level packages is the one being built. The underlying dependencies are dependencies (not top level packages).

From the go build documentation: https://golang.org/cmd/go/

The -i flag installs the packages that are dependencies of the target.

Let's break it down

package a

import "b"

Dev is editing package a. Package a depends on package b. Package b takes 10mins to build. Package a takes milliseconds to build itself. However, to build package a, go must build package b first. So to build package a, it takes 10mins + milliseconds.

To alleviate this very common problem, go has the ability to compile and install a dependent package into a cache folder at $GOPATH/pkg. If for any reason, a or b change their cache is automatically invalidated and rebuilt the next time this package is referenced. So there are no side effects to installing a package. This process happens automatically anytime you go install a package, thus the flag -i. For instance, go install b would install b to $GOPATH/pkg.

-i|install does not modify GOPATH, it does not install random executable on the user's machine. It generates b.a files in the pkg folder for linking to other go packages. Since the flycheck-go-install-deps would default to off, I don't see the issue with adding the ability to enable install deps -i. There are however a lot of useful reasons to enable this option.

[swsnr]

@drewwells If it's that useful I wonder why none else needed this option hitherto, even though the go build has been in Flycheck for a long time. Would you not normally install dependencies in advance, before working on a package?

Inviting @dougm, @gfrey, @ptrv, @yasuyk and @robert-zaremba for their feedback, as they have contributed to Go support in Flycheck.

[drewwells]

Well it has been so far magical how my code compiles in Go. And a
non-trivial amount of effort to see how to change it. For most work, people
use makefiles which makes the emacs compiling just icing on the cake or a
continual series of long running gcc task runs...

On Thu, Jul 2, 2015 at 1:50 AM Sebastian Wiesner notifications@github.com
wrote:

@drewwells https://github.com/drewwells If it's that useful I wonder
why none else needed this option hitherto, even though the go build has
been in Flycheck for a long time. Would you not normally install
dependencies in advance, before working on a package?

Inviting @dougm https://github.com/dougm, @gfrey
https://github.com/gfrey, @ptrv https://github.com/ptrv, @yasuyk
https://github.com/yasuyk and @robert-zaremba
https://github.com/robert-zaremba for their feedback, as they have
contributed to Go support in Flycheck.

—
Reply to this email directly or view it on GitHub
#674 (comment).

[drewwells]

Okay, it's creating the correct string for tags go build -tags\=one\ two which does work... The test I wrote fails, because I have no idea what I'm doing in the test runner. There's dozens of tests failing locally, so it's hard to say what I'm doing wrong there.

[swsnr]

@drewwells It's hard for me, too, if you don't show me any error messages or test runner output.

[swsnr]

@drewwells Are you still interested in this PR? I'd like to merge it, but there are still issues to be addressed.

[drewwells]

I've been using this branch for the last month. It works great so close if
you desire
On Sat, Aug 1, 2015 at 5:19 AM Sebastian Wiesner notifications@github.com
wrote:

@drewwells https://github.com/drewwells Are you still interested in
this PR? I'd like to merge it, but there are still issues to be addressed.

—
Reply to this email directly or view it on GitHub
#674 (comment).

[swsnr]

@drewwells I would not like to close it, but there are still some minor issues that need to be corrected in the definitions of the options. Would you mind to fix them?

[drewwells]

What errors?
On Sat, Aug 1, 2015 at 9:45 AM Sebastian Wiesner notifications@github.com
wrote:

@drewwells https://github.com/drewwells I would not like to close it,
but there are still some minor issues that need to be corrected in the
definitions of the options. Would you mind to fix them?

—
Reply to this email directly or view it on GitHub
#674 (comment).

[drewwells]

The tests are broken, I haven't figured out how those work and it's probably best to just delete the bad tests I wrote.

The defintions match the description tags and i go build options were added. You didn't like -i, but we disagreed on that point. As a go developer, I use -i all day long and do not see a point in having any build parameters without including -i. It's a proper go build parameter and necessary for a go developer to do their work.

I understand it is your project, so do what you like. Though, I won't contribute code that I don't agree with, so I have no intention of removing -i from this PR. If that doesn't fly, then just close this.

[swsnr]

@drewwells We did not disagree on -i, and there's no need to turn this into a matter of principle. I just wanted to understand why we need that option and asked questions. I still don't see the need, but then again, I don't use Go, and thus I'll trust your judgement about the necessity of this option more than I trust mine.

I asked you to remove -i from this PR and move it into a separate PR, because it's easier to discuss two distinct options in two distinct PRs, but if that's too hard, I'm fine with this one too.

But there are still minor issues in the implementation. Please look at my comments in the diff, the arguments given to flycheck-def-option-var still need to be fixed. And I forgot about documentation: The need options need to be added to doc/languages.texi as well.

With regards to the test cases, we can fix them, but then I need to see the actual test errors.

[drewwells]

The test failures are in the CI https://travis-ci.org/flycheck/flycheck/builds/69250708

I tried refactoring def-option-var but it didn't work for what I needed. I'll take a look at that again.

[swsnr]

@drewwells I'm not sure what your problem with flycheck-def-option-var is. You just need to change the argument for the syntax checker, because it's name is go-build and not just go. That's not really important for functionality, but it does matter for the syntax checker documentation.

As for the tests, I just noticed that they ran on the old Travis CI setup, which was broken. Please just rebase on master and force push, that should fix the Travis failures.

Just a last question, what is test/resources/checkers/go/src/tags/tags for? Github tells me it's a binary file?

[drewwells]

Something is wrong in drewwells@8f3890b. I must have misinterpreted your comments. We did get a successful build finally!

[swsnr]

Please remove this blank line.

[swsnr]

Please remove the redundant blank line as by my comment, and squash all commits. I'll merge this PR then.

[drewwells]

As mentioned here: #674 (comment)

There's an issue with the PR comments. go-build causes the PR to fail

[swsnr]

@drewwells That is expected. The PR lacks documentation in doc/languages.texi, which is why the unit tests for the manual fail. But that's fine as far as I'm concerned, because I can add the docs myself as well.

[swsnr]

@drewwells But proper style and a squashed commit is a must for a clean merge, which is why I need to ask you to address these issues.

[drewwells]

Ahh I see, okay I squashed the commits. I'm getting errors with make texinfo, so maybe it's better I don't mess with those.

make texinfo
makeinfo  doc/flycheck.texi -o doc/flycheck.info
doc/flycheck.texi:6: warning: unrecognized encoding name `UTF-8'.
doc/flycheck.texi:311: warning: @strong{Note...} produces a spurious cross-reference in Info; reword to avoid that.
doc/flycheck.texi:342: warning: @strong{Note...} produces a spurious cross-reference in Info; reword to avoid that.
doc/flycheck.texi:404: warning: @strong{Note...} produces a spurious cross-reference in Info; reword to avoid that.
doc/flycheck.texi:650: warning: @strong{Note...} produces a spurious cross-reference in Info; reword to avoid that.
doc/flycheck.texi:971: warning: @strong{Note...} produces a spurious cross-reference in Info; reword to avoid that.
/Users/drew/.emacs.d/personal/preload/flycheck/doc//languages.texi:197: Unknown command `smallindentedblock'.
/Users/drew/.emacs.d/personal/preload/flycheck/doc//languages.texi:202: Unmatched `@end'.
/Users/drew/.emacs.d/personal/preload/flycheck/doc//languages.texi:213: Unknown command `smallindentedblock'.
/Users/drew/.emacs.d/personal/preload/flycheck/doc//languages.texi:218: Unmatched `@end'.
/Users/drew/.emacs.d/personal/preload/flycheck/doc//languages.texi:381: Unknown command `smallindentedblock'.
/Users/drew/.emacs.d/personal/preload/flycheck/doc//languages.texi:389: Unmatched `@end'.
/Users/drew/.emacs.d/personal/preload/flycheck/doc//languages.texi:572: Unknown command `smallindentedblock'.
/Users/drew/.emacs.d/personal/preload/flycheck/doc//languages.texi:577: Unmatched `@end'.
/Users/drew/.emacs.d/personal/preload/flycheck/doc//languages.texi:710: Unknown command `smallindentedblock'.
/Users/drew/.emacs.d/personal/preload/flycheck/doc//languages.texi:715: Unmatched `@end'.

[drewwells]

Here's my guess at what needs to be added to the texi file:

@flycoption flycheck-go-build-install-deps
Install dependencies of the target package being built. Installing
dependencies can dramatically speed up builds that have slow
building dependent packages.
@end table

@flycoption flycheck-go-build-tags
Tags are a list of build tags to consider satisified during a build.
@end table

[drewwells]

@lunaryorn I rebased and put some docs in for the options. Looks like travis finally likes this branch

[swsnr]

@drewwells Thank you, cherry-pick onto master 👍 😍

[swsnr]

And sorry for the long delay!

[drewwells]

No problem glad this out now!!

On Thu, Oct 22, 2015, 1:48 PM Sebastian Wiesner notifications@github.com
wrote:

And sorry for the long delay!

—
Reply to this email directly or view it on GitHub
#674 (comment).