build: pass TAGS to test target

[yaojingguo]

pass TAGS to test target in Makefile

This change is

[tamird]

LGTM but this should go to master and you should at least fix testrace as
well while you're here, and it would be good to get stress and stressrace
too.

On Sep 16, 2016 02:16, "Jingguo Yao" notifications@github.com wrote:

---

You can view, comment on, or merge this pull request online at:

#9421
Commit Summary

• build: pass TAGS to test target

File Changes

• M Makefile
  https://github.com/cockroachdb/cockroach/pull/9421/files#diff-0 (4)

Patch Links:

• https://github.com/cockroachdb/cockroach/pull/9421.patch
• https://github.com/cockroachdb/cockroach/pull/9421.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#9421, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdsPKSvJ-3uRn_AMOynBxTZDtd1Vm_lks5qqjQ-gaJpZM4J-oDt
.

[yaojingguo]

Pass TAGS to all go test related targets and pass it to go tool vet in build/check-style.sh.

[tamird]

LGTM cc @bdarnell

[bdarnell]

LGTM. If we support passing tags into build and install we should definitely support it for the rest of the commands too. I think we should also be passing -installsuffix whenever -tags are used, but since make build and make install are already using -tags without -installsuffix that's no reason to block this change.

[bdarnell]

This quoting doesn't look right. Why can't it just be '$(TAGS)` like the rest?

[yaojingguo]

Yes, your are right. Fixed in the commit.

[bdarnell]

And why isn't this ${1:-}? You're quoting the expansion below so extra quotes shouldn't be needed.

[yaojingguo]

Fixed in the new commit.

[yaojingguo]

It is worthwhile to mention that go build, go test and go tool vet handle tag list in different ways. I have opened an issue golang/go#17148 for it. Here are our make targets corresponding to these go commands:

• make build TAGS=... invokes go build.
• make test TAGS=... invokes go test.
• make check TAGS=... invokes go tool vet.

So we need to use space-separated tag list for make build TAGS=... and make test TAGS=.... And for make check TAGS=..., we need to use comma-separated tags list. How should we deal with this inconsistency? Document it somewhere? Or make TAGS use a consistent format?

[bdarnell]

Why do we need to pass tags to vet? I think it's OK if vet only looks at files in the default build configuration (especially since none of the other linters support tags). We should just use the go build format for now and we can add tags to vet when it is updated to be consistent with the other tools.

[yaojingguo]

@bdarnell OK. So what we are going to do? Is this PR OK for merge? Or should I remove the code which passes TAGS from Makefile to build/check-style.sh?

Now the CircleCI build fails with such a message. I think that it is not caused by my PR:

Your build ran 1151 tests with 1 failure
 TestParallelCreateConflictingTables - github.com/cockroachdb/cockroach/sql
  moreI160918 15:50:44.995102 3449 storage/engine/rocksdb.go:356  opening in memory rocksdb instance

[bdarnell]

Yes, remove the part related to check-style and this is ready to merge. The test failure isn't your fault.

[yaojingguo]

• Remove changes made to build/check-style.sh.
• Remove the code passing TAGS to @build/check-style.sh in check target. But leave TAGS for $(GO) test ... in check target.

[bdarnell]

Thanks!

[yaojingguo]

@bdarnell Is it possible for you or some member of the stability team to cherry-pick this PR into the develop branch? My another PR #8867 depends on this PR.