Skip to content
This repository has been archived by the owner on Jan 26, 2018. It is now read-only.

Only build common protos with --grpc option for LRO. #109

Merged
merged 5 commits into from
Sep 12, 2016
Merged

Only build common protos with --grpc option for LRO. #109

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1755918
Only build common protos with --grpc option for LRO.
Sep 7, 2016
7c1ba99
Increase test coverage
Sep 9, 2016
6d0e042
Latest released version of googleapis-common-protos is 1.3.4
Sep 9, 2016
b571542
Add bytestream.proto per request from @jmuk
Sep 9, 2016
cc1f8bc
Fix a nit
Sep 12, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion config/common_protos.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,4 @@ packages:
version: ''

# semver is the semantic version of the common protos package.
semver: 1.3.2
semver: 1.3.3
2 changes: 1 addition & 1 deletion config/dependencies.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ protobuf:

googleapis_common_protos:
python:
version: '1.3.2'
version: '1.3.3'
ruby:
version: '1.2.0'

Expand Down
16 changes: 11 additions & 5 deletions lib/api_repo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ var DEFAULT_PROTO_COMPILER = 'protoc';
// Python builds using grpcio-tools.
var NO_PROTOC_PLUGIN = ['nodejs', 'java', 'python'];

// common protos for which we build gRPC service methods
var COMMON_GRPC_PROTOS = ['operations.proto'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding bytestream.proto as well? It's not in the common protos right now, but might be worth adding here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I don't have the context on that, but I don't object.


// the default include path, where install the protobuf runtime installs its
// protos.
var DEFAULT_INCLUDE_PATH = Object.freeze(['/usr/local/include']);
Expand Down Expand Up @@ -1018,11 +1021,14 @@ ApiRepo.prototype._makeProtocFunc = function _makeProtocFunc(opts, language) {
pluginOption = '--plugin=';
} else {
args.push('--' + language + '_out=' + outDir);
if (language === 'python') {
// required per https://github.com/grpc/grpc/issues/7857
args.push('--grpc_python_out=' + outDir);
} else {
args.push('--grpc_out=' + outDir);
if (!opts.buildCommonProtos ||
_.includes(COMMON_GRPC_PROTOS, protoPath)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this the path from the proto's rootdir (i.e. 'google/longrunning/operations.proto')?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's just the last element. pkg has the path.

if (language === 'python') {
// required per https://github.com/grpc/grpc/issues/7857
args.push('--grpc_python_out=' + outDir);
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we decided not to do this in order to support GAE?

My understanding was that this required a more complex change to generate both gRPC and gRPC-less modules.

Copy link
Contributor

@geigerj geigerj Sep 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

After offline discussion, I understand that this is actually an improvement over the current state of master, where every common proto is being built with the gRPC plugin (and hence is potentially GAE-incompatible).

} else {
args.push('--grpc_out=' + outDir);
}
}
}
if (!_.includes(NO_PROTOC_PLUGIN, language)) {
Expand Down