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

.travis.yml: Bump to Go 1.11, return to 'gofmt -s', and update the format #92

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@wking

wking commented Nov 7, 2018

In three commits:

  1. 459ce58, bumping the Go versions, which I wrote by hand.
  2. 118c0b1, reverting be6978e (#40).
  3. 9f73bc1, with the gofmt changes.

Details on each in the commit messages.

wking added some commits Nov 7, 2018

.travis.yml: Bump to modern Go (1.11)
Go 1.11 came out in August [1], which means Go 1.9 and earlier are no
longer supported [2].  Just test on supported versions.

[1]: https://golang.org/doc/devel/release.html#go1.11
[2]: https://golang.org/doc/devel/release.html#policy
Revert "Remove simplify-code option, for now"
This reverts commit be6978e (Remove
simplify-code option, for now, 2016-03-12, #40).  I expect this was
due to a simplification change between Go versions (the commit message
doesn't say), but none of the versions which we were using back then
are supported any more.

wking added some commits Nov 7, 2018

httpcache: Update gofmt
Generated with:

  $ gofmt -s -w *.go */*.go

using:

  $ go version
  go version go1.11.2 linux/amd64
.travis.yml: Only run 'gofmt' on 1.11.x
The indent heuristic changed with Go 1.11 [1], so there's no single
form that satisfies gofmt for both Go 1.10 and Go 1.11.  With this
commit, we now only run gofmt on 1.11.

Docs for the 'include' syntax are in [2].

Also replace the double-diff with a 'gofmt -w ...' to write the
changes and a 'git diff ...' call to display the changes and exit
non-zero if there were any.

[1]: https://golang.org/doc/go1.11#gofmt
[2]: https://docs.travis-ci.com/user/customizing-the-build/#explicitly-including-jobs
@wking

This comment has been minimized.

wking commented Nov 7, 2018

I've pushed a new d5cd581 to only run gofmt on 1.11.x (and I've updated our format to match the 1.11.x suggestions). See here for example output of the git diff call from before I updated our format.

wking added a commit to wking/httpcache that referenced this pull request Nov 7, 2018

Merge branch 'travis-go-bump'
gregjones#92

* travis-go-bump:
  .travis.yml: Only run 'gofmt' on 1.11.x
  httpcache: Update gofmt
  Revert "Remove simplify-code option, for now"
  .travis.yml: Bump to modern Go (1.11)
@dmitshur

Thanks for the PR. I have one question for now.

install:
- # Do nothing. This is needed to prevent default install action "go get -t -v ./..." from happening here (we want it to happen inside script step).
script:
- go get -t -v ./...
- diff -u <(echo -n) <(gofmt -d .)
- if test -n "${GOFMT}"; then gofmt -w -s . && git diff --exit-code; fi

This comment has been minimized.

@dmitshur

dmitshur Nov 8, 2018

Collaborator

What is the motivation for changing diff -u <(echo -n) <(gofmt -d -s .) to gofmt -w -s . && git diff --exit-code?

This comment has been minimized.

@wking

wking Nov 8, 2018

What is the motivation for changing diff -u <(echo -n) <(gofmt -d -s .) to gofmt -w -s . && git diff --exit-code?

The old approach created a doubled diff, and the round of leading + it put before everything was distracting. Compare this and this.

This comment has been minimized.

@dmitshur

dmitshur Nov 9, 2018

Collaborator

Thanks for explaining, that makes sense.

I'd like to look a way to continue to rely on gofmt -d to display a gofmt diff, rather than gofmt -w followed by git diff, if possible. The motivation is that:

  1. It doesn't modify the source code. Even though gofmt -w should not change behavior, it's still better to avoid modifying source code in the general case (and trying to revert changes can make the command verbose).
  2. It's not fallible to missing gofmt issues due to .gitignore or other git configuration details.

An idea is to use -n flag to diff rather than -u, since it produces a cleaner diff without the leading +s. I prototyped it in commit eedefab. You can see its Travis output at https://travis-ci.org/gregjones/httpcache/jobs/452715773.

How do you feel about going with that approach?

This comment has been minimized.

@wking

wking Nov 9, 2018

I'm not worried about either of your cases ;). If someone submits an ignored file, I expext we'd reject that regardless of its Go formatting. And if gofmt makes any change to an unignored file, the tests will fail regardless of whether the change is functional. Who cares what's on disk when the failed container gets torn down?

But your output looks fine too, so I'm fine with you landing your approach instead.

This comment has been minimized.

@dmitshur

dmitshur Nov 11, 2018

Collaborator

Who cares what's on disk when the failed container gets torn down?

I don't mind what's on disk after the container is being shutdown. It's just that gofmt -w writes to disk first, and then the rest of the tests run on potentially modified .go files second.

I know that for this repo it's extremely unlikely it'd ever make a difference. However, I'd like to find a set of commands as general and simple as possible, in order to keep the Travis scripts used in many repositories in sync.

But your output looks fine too, so I'm fine with you landing your approach instead.

Great! If you don't mind, please update this PR to use it. I think the rest of the changes look good so far.

Thanks!

This comment has been minimized.

@wking

wking Nov 11, 2018

It's just that gofmt -w writes to disk first, and then the rest of the tests run on potentially modified .go files second.

But the tests as a whole are going to fail anyway (because git diff will notice the changes). So again, I don't see an issue. But searching for a happy medium, how about I move the gofmt call to the end of the script block? Would that address your concerns?

However, I'd like to find a set of commands as general and simple as possible, in order to keep the Travis scripts used in many repositories in sync.

That's a good goal, but I don't see a lot of complexity or generality space between gofmt -w -s . && git diff --exit-code and diff -n <(echo -n) <(gofmt -d .).

But your output looks fine too, so I'm fine with you landing your approach instead.

Great! If you don't mind, please update this PR to use it.

I'm fine with a parallel PR landing instead of this one. But as long as we're working on this one, I'd rather keep hunting until we find an approach that we both like. I'm ok with the nested-diff approach (whether it uses -u or -n), but I don't like it ;).

This comment has been minimized.

@dmitshur

dmitshur Nov 11, 2018

Collaborator

But as long as we're working on this one, I'd rather keep hunting until we find an approach that we both like. I'm ok with the nested-diff approach (whether it uses -u or -n), but I don't like it ;).

Understandable. If at any point you decide you'd rather just land this sooner, let me know. Until then, here are some more thoughts on this.

But the tests as a whole are going to fail anyway (because git diff will notice the changes).

I was referring to the situation where gofmt -w writes first, git diff reports no diff (likely because nothing changed), and then tests run and succeed.

But searching for a happy medium, how about I move the gofmt call to the end of the script block?

I would not prefer that more. I think running a gofmt check first is a good idea, since it's fast and a more basic check than all the tests. It's just that I want to find a way of running it without writing to disk.

That's a good goal, but I don't see a lot of complexity or generality space between gofmt -w -s . && git diff --exit-code and diff -n <(echo -n) <(gofmt -d .).

The main difference is that the former writes to disk (before running tests), the latter does not. The idea of writing the .go files before testing them kinda bugs me.

gofmt itself isn't so bad; but I've seen other repos where the similar approach of "writing to repo and then using git diff" was used to check that go generate doesn't produce a diff. go generate may produce a diff, and I wouldn't want that to influence the rest of the tests. But trying to revert a potential diff means more git commands... Which is what motivates me to look for a solution that finds out whether there's a diff without writing to disk. That may not be possible in the case of go generate though.

This comment has been minimized.

@wking

wking Nov 12, 2018

It's just that gofmt -w writes to disk first, and then the rest of the tests run on potentially modified .go files second.

But the tests as a whole are going to fail anyway (because git diff will notice the changes).

I was referring to the situation where gofmt -w writes first, git diff reports no diff (likely because nothing changed), and then tests run and succeed.

Wait, that's what we want to happen for a successful PR, right? If gofmt is happy with the originals, that's great, right? I thought you were concerned about cases where the originals would have failed the unit tests (or some other later test), but the gofmt -w call adjusts the originals to get them to pass the later tests instead and git diff fails to notice the change vs. the originals. I tried to rebut that situation here, based on the unlikeliness of a gitignored .go file. Are you still worried about gitignored .go files? If so, they probably merit an explict CI check, because we should be rejecting them regardless of whether they are properly formatted Go ;).

The idea of writing the .go files before testing them kinda bugs me.

But if we write something different, the test run as a whole will fail. In the very unlikely case that the gofmt output passes a unit test that the originals would have failed, the gofmt failure will still keep the broken originals from landing.

go generate may produce a diff, and I wouldn't want that to influence the rest of the tests.

Again, why does a pure test matter? You need everything green before the code lands, so cross-talk in a run that's going to fail anyway doesn't seem like a big deal to me.

@wking wking referenced this pull request Nov 9, 2018

Open

Streaming caches #93

wking added a commit to wking/httpcache that referenced this pull request Nov 9, 2018

Merge branch 'travis-go-bump'
gregjones#92

* travis-go-bump:
  .travis.yml: Only run 'gofmt' on 1.11.x
  httpcache: Update gofmt
  Revert "Remove simplify-code option, for now"
  .travis.yml: Bump to modern Go (1.11)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment