Use Bazel to build the entire backend and perform API code generation #609

neuromage · 2019-01-03T04:59:24Z

Using Bazel means we'll get reproducible builds that are not dependent on the tools installed by each dev. This change adds BUILD files for the backend that builds both binaries, as well as the API protocol buffers and swagger definitions.

In order to let go build and go test work as before, the Makefile is replaced with a script that uses Bazel to first generate the API code, and then copy them back into the source tree.

Most of the BUILD files were generated automatically using Gazelle.

Apologies for the large PR but this is hopefully a one time change. From here on, running bazel //:gazelle should update the BUILD files automatically.

We may also want to consider not checking in generated code altogether. This may be a better option going forward.

This change is

This also uses Bazel to generate code from the API definition in the proto files. The Makefile is replaced with a script that uses Bazel to first generate the code, and then copy them back into the source tree. Most of the BUILD files were generated automatically using Gazelle.

neuromage · 2019-01-03T05:00:33Z

Note: the changes in the generated files are due to using more up to date versions of the code generation tools.

neuromage · 2019-01-03T05:01:09Z

/cc @vicaire
/cc @yebrahim
/cc @IronPan

yebrahim

It would be great to close on checking-in auto-generated code. If bazel simplifies the build and we have all of our dependencies pinned down, I'm in favor of maintaining the fewest sources of truth. (To quickly follow this up, if a tool can generate the BUILD files, why are we checking them in?)

My opinion: if we have good reasons for checking in auto-generated files, let's discuss them here. Let's remove them otherwise.

yebrahim · 2019-01-03T07:11:46Z

backend/api/go_client/parameter.pb.gw.go

@@ -0,0 +1,3 @@
+// +build ignore


Why are these files needed?

There are auto-generated. I have no idea how this works. It also varies depending on the version of protoc-gen-grpc that we use.

yebrahim · 2019-01-03T07:14:04Z

backend/api/generate_api.sh

+  name=$(basename ${f})
+  cp $f go_client/${name}
+  chmod 766 $f
+  ${AUTOGEN_CMD} -i --no-tlc -c "Google LLC" -l apache go_client/${name}


It seems like this line didn't work in the checked in sources, they're missing the license headers.

Sorry, something must have gone wrong when I was merging with master. I re-ran the script and generated the licenses.

yebrahim · 2019-01-03T07:23:28Z

WORKSPACE

+    tag = "v1.1.1",
+)
+
+# go_repository(


Was this generated this way? Why would the tool comment this out?

Sorry I commented that out. Removed it, as we don't need it. The proto types are built into the rules now.

yebrahim · 2019-01-03T07:28:31Z

Can you also add a backend/README file that explains how to build the backend, and what the build toolset is? Is it just bazel? What version.. etc?

IronPan

could you use wildcard for all src[] section of the bazel file? so that it doesn't need to be manually maintained?

IronPan · 2019-01-03T16:50:53Z

backend/api/experiment.proto

@@ -20,6 +20,7 @@ import "google/api/annotations.proto";
 import "google/protobuf/empty.proto";
 import "google/protobuf/timestamp.proto";
 import "protoc-gen-swagger/options/annotations.proto";
+import "backend/api/error.proto";


is this change required or by accident?

This is required. Note the ".api.Status" which is the default response below. When generating swagger.json files, the error.proto file is what contains the Status message, and it must be visible to this file so that protoc_gen_swagger BUILD rule can generate the right swagger definitions. Otherwise, it's an error. This took me forever to figure out! :-)

thanks for confirm.

IronPan · 2019-01-03T16:53:21Z

backend/api/go_client/BUILD.bazel

+go_library(
+    name = "go_default_library",
+    srcs = [
+        "error.pb.go",


can this be wildcard *.pb.go *.pb.gw.go? otherwise this need to manually added in the future i guess?

There's actually no need to manually manage these. Just run bazel run //:gazelle to auto update the BUILD files. I added a README with these instructions.

IronPan · 2019-01-03T16:54:45Z

backend/api/go_client/error.pb.go

@@ -1,28 +1,12 @@
-// Copyright 2018 Google LLC


headers seems gone here

Sorry, re-ran the script and added them back. Think something went wrong when I was merging.

Also fix the missing licenses in the generated proto files.

neuromage · 2019-01-03T17:35:16Z

I added a README with instructions on how to build/test with bazel. The BUILD files don't need to be manually maintained, for the most part, running bazel run //:gazelle will auto-update them.

As for whether we should keep checking generated code in, here are the pros and cons as I see them:

Pros:

go build and go test continue to work as before. No need to change build/release process as yet.
IDE/Editors see the generated code, and don't need special Bazel plugins to auto-complete on them.

Cons;

Generated code changes are noisy during code reviews
Spurious merge conflicts to resolve when changes touch proto files.

If we do decide to remove them, my preference is to do that in a separate PR.

yebrahim · 2019-01-03T22:06:04Z

Thanks Ajay. Looks like there's still some generated files missing the license headers.
Is there value in checking in the BUILD files if they're all auto-generated? Can the gazelle build rule be run before running the rest of the bazel rules? This way we can just add them to .gitignore.

yebrahim · 2019-01-03T22:07:52Z

I guess it's a good idea to discuss maintaining auto-generated files in a separate issue so as not to block this PR on it, but it's good to make sure we really need any new generated files.

neuromage · 2019-01-03T22:26:10Z

Thanks for catching that! Added the missing license for files under go_http_client, and fixed the script so this won't happen again.

We will need to check in BUILD files. The files are only mostly autogenerated. I made some changes (see lines that have #keep in them) to make sure everything compiles and tests correctly. Also, auto generation only works for Go. Other languages will require more manual BUILD file work. It's standard to check these in in projects that use Bazel. I think we should do the same.

yebrahim · 2019-01-03T22:40:38Z

SG then.
/lgtm

IronPan · 2019-01-05T00:29:18Z

/lgtm
/approve

k8s-ci-robot · 2019-01-05T00:29:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [IronPan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2019-01-05T00:29:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [IronPan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

IronPan · 2019-01-05T00:29:31Z

Thanks for the change

)

k8s-ci-robot requested review from IronPan, vicaire and qimingj January 3, 2019 04:59

k8s-ci-robot added size/XXL labels Jan 3, 2019

k8s-ci-robot requested a review from yebrahim January 3, 2019 05:01

neuromage added 2 commits January 2, 2019 21:02

Fix indentation in generate_api.sh

cd15d5f

Clean up WORKSPACE

3aeb2f1

yebrahim reviewed Jan 3, 2019

View reviewed changes

IronPan reviewed Jan 3, 2019

View reviewed changes

Add README for building/testing backend.

dc5989b

Also fix the missing licenses in the generated proto files.

Add license to files under go_http_client

a89577b

k8s-ci-robot assigned yebrahim Jan 3, 2019

k8s-ci-robot added lgtm labels Jan 3, 2019

Merge with master

af08744

k8s-ci-robot removed the lgtm label Jan 4, 2019

k8s-ci-robot assigned IronPan Jan 5, 2019

k8s-ci-robot added lgtm labels Jan 5, 2019

k8s-ci-robot added the approved label Jan 5, 2019

k8s-ci-robot merged commit 163545b into kubeflow:master Jan 5, 2019

HumairAK pushed a commit to red-hat-data-services/data-science-pipelines that referenced this pull request Mar 11, 2024

fix(sdk): Fix nested recursion runafter and param mapping; (kubeflow#609

af9f03d

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Bazel to build the entire backend and perform API code generation #609

Use Bazel to build the entire backend and perform API code generation #609

neuromage commented Jan 3, 2019 •

edited by jlewi

Loading

neuromage commented Jan 3, 2019

neuromage commented Jan 3, 2019

yebrahim left a comment

yebrahim Jan 3, 2019

neuromage Jan 3, 2019

yebrahim Jan 3, 2019

neuromage Jan 3, 2019

yebrahim Jan 3, 2019

neuromage Jan 3, 2019

yebrahim commented Jan 3, 2019

IronPan left a comment

IronPan Jan 3, 2019

neuromage Jan 3, 2019

IronPan Jan 5, 2019

IronPan Jan 3, 2019

neuromage Jan 3, 2019

IronPan Jan 3, 2019

neuromage Jan 3, 2019

neuromage commented Jan 3, 2019

yebrahim commented Jan 3, 2019

yebrahim commented Jan 3, 2019

neuromage commented Jan 3, 2019

yebrahim commented Jan 3, 2019

IronPan commented Jan 5, 2019

k8s-ci-robot commented Jan 5, 2019

k8s-ci-robot commented Jan 5, 2019

IronPan commented Jan 5, 2019

Use Bazel to build the entire backend and perform API code generation #609

Use Bazel to build the entire backend and perform API code generation #609

Conversation

neuromage commented Jan 3, 2019 • edited by jlewi Loading

neuromage commented Jan 3, 2019

neuromage commented Jan 3, 2019

yebrahim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yebrahim commented Jan 3, 2019

IronPan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neuromage commented Jan 3, 2019

yebrahim commented Jan 3, 2019

yebrahim commented Jan 3, 2019

neuromage commented Jan 3, 2019

yebrahim commented Jan 3, 2019

IronPan commented Jan 5, 2019

k8s-ci-robot commented Jan 5, 2019

k8s-ci-robot commented Jan 5, 2019

IronPan commented Jan 5, 2019

neuromage commented Jan 3, 2019 •

edited by jlewi

Loading