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

ci: install all packages first #9981

Merged
merged 1 commit into from
Oct 14, 2016
Merged

Conversation

tbg
Copy link
Member

@tbg tbg commented Oct 14, 2016

All packages need to be installed before we can run (some) of the checks and
code generators reliably. More precisely, anything that using x/tools/go/loader
is fragile (this includes stringer, vet and others).

The blocking issue is golang/go#14120; see
golang/go#10249 for some more concrete discussion on
stringer and golang/go#16086 for vet.


This change is Reviewable

All packages need to be installed before we can run (some) of the checks and
code generators reliably. More precisely, anything that using x/tools/go/loader
is fragile (this includes stringer, vet and others).

The blocking issue is golang/go#14120; see
golang/go#10249 for some more concrete discussion on
`stringer` and golang/go#16086 for `vet`.
build/builder.sh make check 2>&1 | go-test-teamcity
# All packages need to be installed before we can run (some) of the checks
# and code generators reliably. More precisely, anything that using
# x/tools/go/loader is fragile (this includes stringer, vet and others).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this imply we should add gotestdashi as a dependency of make check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does imply that, but we'd still need it here for stability (no reason we wouldn't reorder make check and go generate). I like @dt's idea though.

@dt
Copy link
Member

dt commented Oct 14, 2016

Could we do these steps inside style_check.go, wrapped in a if !testing.Short(), so they could be skipped easily when only running pure text manipulation linter?

@tbg
Copy link
Member Author

tbg commented Oct 14, 2016

@dt That's a good idea. Would you mind doing that? You can poach this PR for the commentary or I can merge it, your call (or whoever is up for putting in the elbow grease).

@tbg
Copy link
Member Author

tbg commented Oct 14, 2016

One problem with the diff check though is that it mutates the repo state (running go generate).
I think that's generally awkward. What if it fails? You've just messed with a bunch of files. There could be uncommited files even before you run make check. We certainly shouldn't expect that to be run from a clean repo.

@dt
Copy link
Member

dt commented Oct 14, 2016

@tschottdorf I probably won't do that today, so don't wait for me -- if you want to merge for now and file an issue to follow up later?

@tbg
Copy link
Member Author

tbg commented Oct 14, 2016

SGTM, will do once LGTSO

@tamird
Copy link
Contributor

tamird commented Oct 14, 2016

Thanks for tracking down the upstream issues. I agree with @petermattis that this should be hooked into the relevant stuff globally, and not just in TC.

@tbg
Copy link
Member Author

tbg commented Oct 14, 2016

Yes, but how do you hook it into go generate ./...? We'd have to put it one line before each stringer invocation and hope for the best?

@tbg
Copy link
Member Author

tbg commented Oct 14, 2016

Generate processes packages in the order given on the command line,
one at a time. If the command line lists .go files, they are treated
as a single package. Within a package, generate processes the
source files in a package in file name order, one at a time. Within
a source file, generate runs generators in the order they appear
in the file, one at a time.

Hmm, we could make a file that shows up first and make that run go build -i ./...? But then it'll still fail if you just generate a subpkg explicitly.

@tbg
Copy link
Member Author

tbg commented Oct 14, 2016

Filed #9983 for the remainder of the discussion. I'll merge this as per @dt's suggestion.

@tbg tbg merged commit 09ef4da into cockroachdb:master Oct 14, 2016
@tbg tbg deleted the check-build-first branch October 14, 2016 14:58
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