Use protobuild to build protobufs - #2337
Conversation
Signed-off-by: He Xiaoxi <tossmilestone@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #2337 +/- ##
==========================================
- Coverage 60.59% 60.27% -0.32%
==========================================
Files 128 128
Lines 26048 26168 +120
==========================================
- Hits 15783 15772 -11
- Misses 8878 8997 +119
- Partials 1387 1399 +12 |
Signed-off-by: He Xiaoxi <tossmilestone@gmail.com>
|
Please sign your commits following these rules: $ git clone -b "use-protobuild" git@github.com:tossmilestone/swarmkit.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842353964368
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -fAmending updates the existing PR. You DO NOT need to open a new one. |
Signed-off-by: He Xiaoxi <tossmilestone@gmail.com>
Signed-off-by: He Xiaoxi <tossmilestone@gmail.com>
4fcae4b to
79c9bf2
Compare
| [includes] | ||
| # Include paths that will be added before all others. Typically, you want to | ||
| # treat the root of the project as an include, but this may not be necessary. | ||
| before = [".", "./protobuf"] |
There was a problem hiding this comment.
Is ./protobuf necessary? Can update these paths?
There was a problem hiding this comment.
Include ./protobuf will let the proto files in swarmkit/api can import swarmkit/protobuf/plugin/plugin.proto using relative path plugin/plugin.proto.
If not include ./protobuf, we must change plugin/plugin.proto imports to full path. It's really not necessary but a choice. We can discuss which is better. ping @stevvooe
There was a problem hiding this comment.
Let's go ahead and use the full include path so we don't need these extra paths. plugin/plugin.proto doesn't provide as much information as github.com/docker/swarmkit/protobufplugin/plugin.proto.
| import "github.com/docker/swarmkit/api/objects.proto"; | ||
| import "github.com/docker/swarmkit/api/types.proto"; | ||
| import "gogoproto/gogo.proto"; | ||
| import "plugin/plugin.proto"; |
There was a problem hiding this comment.
This path probably needs to take on the full paths.
There was a problem hiding this comment.
With full path import, the ./protobuf include in Protobuild.toml can be removed. ping @stevvooe
|
Ok, this looks like a good start! I'm surprised that applying deepcopy everywhere just works! We might have to fix that later, but it seems to cause little harm here. From a generation perspective, everything looks correct here. Please checkout a few comments I have on the paths for |
| @echo "🐳 $@" | ||
| @PATH=${ROOTDIR}/bin:${PATH} go generate -x ${PACKAGES} | ||
|
|
||
| protos: bin/protoc-gen-gogoswarm ## generate protobuf |
There was a problem hiding this comment.
Unless protos is an actual file that gets generated, it should be added to the .PHONY target.
There was a problem hiding this comment.
Yes, the protos should be added to .PHONY.
Signed-off-by: He Xiaoxi <tossmilestone@gmail.com>
|
@stevvooe @aaronlehmann All problems addressed has been done, please help to review it again. |
|
LGTM I'll also be adding some API stability tools to protobuild that will benefit swarmkit! |
|
To use the API stability tool of protobuild, I want to generate descriptor files of package |
|
@tossmilestone I think that is a great addition. Please do so in a follow up PR. |
|
@stevvooe OK |
4 participants
This PR uses protobuild tool to build protobufs, replacing the "go generate" command.
Signed-off-by: He Xiaoxi tossmilestone@gmail.com