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

Bazel CI breaks frequently #817

Closed
drigz opened this issue Nov 19, 2018 · 12 comments
Closed

Bazel CI breaks frequently #817

drigz opened this issue Nov 19, 2018 · 12 comments

Comments

@drigz
Copy link
Contributor

drigz commented Nov 19, 2018

Adding Gazelle to the CI (#802) has resulted in additional work for contributors (#812, #816).

@johanbrandhorst: how often will unrelated user PRs be failing like this? Is there some way we can just update these manually and not have it fail the build?

This breakage could be quite frequent if it's required for any change that affects the dependencies of the individual Golang packages. I see two options:

  • streamline it by adding the docker run command to the error message in the CI, and possibly by adding instructions to the pull request template
    • the risk is that we annoy contributors: the docker run command involves a ~1GB download as well as a build takes several minutes on a laptop
  • make the CI check optional (or remove it) and accept that the Bazel build will be mostly broken
    • you could add it to the release checklist, so that numbered releases have working Bazel builds
    • the risk is that a change breaks Gazelle/rules_go, and the release creator has to work out which change it was

I don't have a preference as our project doesn't need to stay on the bleeding edge of grpc-gateway. Does the Bazel build have any other users?

@johanbrandhorst
Copy link
Collaborator

I don't know about users of the Bazel build, but if the gazelle_fix command updates Go dependencies in the background we risk running out of sync with our Gopkg.toml (if we haven't already), which would be unfortunate. Is that correct?

I heard from @achew22 that bazel stuff will sometime soon be generated from go.mod files (or something to this effect) so we should be able to maintain a single source of truth (a new go.mod file). Until such time, I think I'd like this to be a manual step (option 2), if my understanding above is correct.

@drigz
Copy link
Contributor Author

drigz commented Nov 19, 2018

if the gazelle_fix command updates Go dependencies in the background we risk running out of sync with our Gopkg.toml

It doesn't, so that shouldn't be a problem. Whether using go.mod or not, it will still be manual, although using go.mod could streamline the manual process.

:gazelle_fix updates the edges in the dependency graph, as well as adding new rules within this repository, but it won't change the versions of external dependencies. I don't know exactly how go.mod will be used by Gazelle, but I suspect it'll be a different command to the one used by :gazelle_fix.

@johanbrandhorst
Copy link
Collaborator

johanbrandhorst commented Nov 19, 2018

Ok, that's not so bad then. So is it correct that this might still be useful sometimes because user submitted changes may legitimately break something? To me it looks like:

  1. Sometimes cause friction for contributors, but occasionally highlight genuine problems.
  2. Sometimes cause friction for maintainers whenever we come to manually running :gazelle_fix.

Is that a fair summary? If so, I don't think option 1 is so bad for now, if we find it's causing a lot of friction more often than we'd like we can move to option 2.

@johanbrandhorst
Copy link
Collaborator

As it turned out the failure in #816 was correct - I was wrong about it being unrelated to the change.

@achew22
Copy link
Collaborator

achew22 commented Nov 19, 2018

I am having a hard time construction the situation where you need to run docker to invoke gazelle. Gazelle is used to bootstrap into bazel and doesn't have any hard dependencies on it.

go get -u github.com/bazelbuild/bazel-gazelle/cmd/gazelle

@johanbrandhorst
Copy link
Collaborator

Do we still need bazel for the :buildifier job? Is the buildifier necessary to put in the contribution notes? It would be nice if we could drop the bazel docker image in favor of just the go get.

@achew22
Copy link
Collaborator

achew22 commented Nov 19, 2018

It should just be standard go code and work fine without needing bazel, but I'm less certain about it (since it's not designed to be the on ramp).

Not at a computer for a while but I would guess something like

go get -u github.com/bazelbuild/buildtools/buildifier

@drigz
Copy link
Contributor Author

drigz commented Nov 19, 2018

I honestly didn't think about using go get to pull gazelle and buildifier. Is there a way to pin the versions to make sure contributors are running the same versions as in CI?

@johanbrandhorst
Copy link
Collaborator

If they're both linters I'm happy enough to always use tip tbh. Otherwise we can add it to Gopkg.toml and store the versions in Gopkg.lock: https://github.com/golang/dep/blob/master/docs/Gopkg.toml.md#required

@drigz
Copy link
Contributor Author

drigz commented Nov 19, 2018

It's unfortunately not safe to use tip for Gazelle, as its generated code is tied to the version of rules_go in WORKSPACE.

IIUC, if it's required in Gopkg.toml, it'll need to be installed with go install - would you put that in Makefile?

@johanbrandhorst
Copy link
Collaborator

Nah we could just put that in the contribution notes: dep ensure --vendor-only && go install ./vendor/github.com/bazelbuild/buildtools/buildifier. We're still only talking about the cases where CI breaks for contributors, it won't be very often hopefully.

@johanbrandhorst
Copy link
Collaborator

This has turned out not to be such a big problem so I'm happy to close this.

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

No branches or pull requests

3 participants