You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2017/08/23 05:46:53 UTC
[kudu-CR] [cmake] introduce 'pb-gen' and 'krpc-gen' target
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7777
Change subject: [cmake] introduce 'pb-gen' and 'krpc-gen' target
......................................................................
[cmake] introduce 'pb-gen' and 'krpc-gen' target
Introduced a new target to generate protobuf stubs and KRPC service and
proxy files.
The 'iwyu' target now depends on the newly introduced 'pb-gen' and
'krpc-gen' targets. That's because IWYU needs to have all protobuf
and KRPC headers available when running.
Change-Id: I48833098553ef7d3f4fae8b88079cb354b52bea1
---
M CMakeLists.txt
M src/kudu/cfile/CMakeLists.txt
M src/kudu/client/CMakeLists.txt
M src/kudu/common/CMakeLists.txt
M src/kudu/consensus/CMakeLists.txt
M src/kudu/fs/CMakeLists.txt
M src/kudu/master/CMakeLists.txt
M src/kudu/rpc/CMakeLists.txt
M src/kudu/security/CMakeLists.txt
M src/kudu/server/CMakeLists.txt
M src/kudu/tablet/CMakeLists.txt
M src/kudu/tserver/CMakeLists.txt
M src/kudu/util/CMakeLists.txt
13 files changed, 99 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/7777/1
--
To view, visit http://gerrit.cloudera.org:8080/7777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I48833098553ef7d3f4fae8b88079cb354b52bea1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [cmake] introduce 'pb-gen' and 'krpc-gen' targets
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7777
to look at the new patch set (#3).
Change subject: [cmake] introduce 'pb-gen' and 'krpc-gen' targets
......................................................................
[cmake] introduce 'pb-gen' and 'krpc-gen' targets
Introduced new targets to generate protobuf stubs and KRPC service/proxy
files.
The 'iwyu' target now depends on the newly introduced 'pb-gen' and
'krpc-gen' targets. That's because IWYU needs to have all protobuf
and KRPC headers available when running.
Change-Id: I48833098553ef7d3f4fae8b88079cb354b52bea1
---
M CMakeLists.txt
M cmake_modules/FindKRPC.cmake
M cmake_modules/FindProtobuf.cmake
M src/kudu/cfile/CMakeLists.txt
M src/kudu/client/CMakeLists.txt
M src/kudu/common/CMakeLists.txt
M src/kudu/consensus/CMakeLists.txt
M src/kudu/fs/CMakeLists.txt
M src/kudu/master/CMakeLists.txt
M src/kudu/rpc/CMakeLists.txt
M src/kudu/security/CMakeLists.txt
M src/kudu/server/CMakeLists.txt
M src/kudu/tablet/CMakeLists.txt
M src/kudu/tserver/CMakeLists.txt
M src/kudu/util/CMakeLists.txt
15 files changed, 85 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/7777/3
--
To view, visit http://gerrit.cloudera.org:8080/7777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I48833098553ef7d3f4fae8b88079cb354b52bea1
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [cmake] introduce 'pb-gen' and 'krpc-gen' targets
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [cmake] introduce 'pb-gen' and 'krpc-gen' targets
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7777/2/CMakeLists.txt
File CMakeLists.txt:
PS2, Line 634: source&header
Nit: source/header, or "source and header"
Line 637: function(KUDU_PROTOBUF_GENERATE_CPP SRCS HDRS TGTS)
FWIW, FindProtobuf.cmake is already a heavily-modified form of cmake's original file. Meaning, I don't think you necessarily need to isolate these changes from PROTOBUF_GENERATE_CPP; I think it'd be fine to modify PROTOBUF_GENERATE_CPP directly.
Same goes for KRPC_GENERATE, which is definitely Kudu-specific.
PS2, Line 659: source&header
same
--
To view, visit http://gerrit.cloudera.org:8080/7777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I48833098553ef7d3f4fae8b88079cb354b52bea1
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [cmake] introduce 'pb-gen' and 'krpc-gen' targets
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [cmake] introduce 'pb-gen' and 'krpc-gen' targets
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7777/2/CMakeLists.txt
File CMakeLists.txt:
Line 637: function(KUDU_PROTOBUF_GENERATE_CPP SRCS HDRS TGTS)
> FWIW, FindProtobuf.cmake is already a heavily-modified form of cmake's orig
The point here is to automatically add dependencies to some top-level targets. I was not sure that would be a good style to use hard-coded targets in the cmake module file.
Otherwise, it would be necessary to specify the desired target to add the TGTS for dependency as a parameter in all the sites where PROTOBUF_GENERATE_CPP is used.
If updating the definition of the targets in the cmake module file, which option from the mentioned above one do you prefer?
--
To view, visit http://gerrit.cloudera.org:8080/7777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I48833098553ef7d3f4fae8b88079cb354b52bea1
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [cmake] introduce 'pb-gen' and 'krpc-gen' targets
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged.
Change subject: [cmake] introduce 'pb-gen' and 'krpc-gen' targets
......................................................................
[cmake] introduce 'pb-gen' and 'krpc-gen' targets
Introduced new targets to generate protobuf stubs and KRPC service/proxy
files.
The 'iwyu' target now depends on the newly introduced 'pb-gen' and
'krpc-gen' targets. That's because IWYU needs to have all protobuf
and KRPC headers available when running.
Change-Id: I48833098553ef7d3f4fae8b88079cb354b52bea1
Reviewed-on: http://gerrit.cloudera.org:8080/7777
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Alexey Serbin <as...@cloudera.com>
---
M CMakeLists.txt
M cmake_modules/FindKRPC.cmake
M cmake_modules/FindProtobuf.cmake
3 files changed, 23 insertions(+), 0 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Alexey Serbin: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I48833098553ef7d3f4fae8b88079cb354b52bea1
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [cmake] introduce 'pb-gen' and 'krpc-gen' targets
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7777
to look at the new patch set (#4).
Change subject: [cmake] introduce 'pb-gen' and 'krpc-gen' targets
......................................................................
[cmake] introduce 'pb-gen' and 'krpc-gen' targets
Introduced new targets to generate protobuf stubs and KRPC service/proxy
files.
The 'iwyu' target now depends on the newly introduced 'pb-gen' and
'krpc-gen' targets. That's because IWYU needs to have all protobuf
and KRPC headers available when running.
Change-Id: I48833098553ef7d3f4fae8b88079cb354b52bea1
---
M CMakeLists.txt
M cmake_modules/FindKRPC.cmake
M cmake_modules/FindProtobuf.cmake
3 files changed, 23 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/7777/4
--
To view, visit http://gerrit.cloudera.org:8080/7777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I48833098553ef7d3f4fae8b88079cb354b52bea1
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [cmake] introduce 'pb-gen' and 'krpc-gen' targets
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [cmake] introduce 'pb-gen' and 'krpc-gen' targets
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I48833098553ef7d3f4fae8b88079cb354b52bea1
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [cmake] introduce 'pb-gen' and 'krpc-gen' targets
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [cmake] introduce 'pb-gen' and 'krpc-gen' targets
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7777/2/CMakeLists.txt
File CMakeLists.txt:
Line 637: function(KUDU_PROTOBUF_GENERATE_CPP SRCS HDRS TGTS)
> I meant:
I don't think having those functions refer to a top-level target is such a big deal. If you add a comment explaining what the "magic" target is and where to find it, I'm sure that's enough information.
--
To view, visit http://gerrit.cloudera.org:8080/7777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I48833098553ef7d3f4fae8b88079cb354b52bea1
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [cmake] introduce 'pb-gen' and 'krpc-gen' targets
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has uploaded a new patch set (#2).
Change subject: [cmake] introduce 'pb-gen' and 'krpc-gen' targets
......................................................................
[cmake] introduce 'pb-gen' and 'krpc-gen' targets
Introduced a new target to generate protobuf stubs and KRPC service and
proxy files.
The 'iwyu' target now depends on the newly introduced 'pb-gen' and
'krpc-gen' targets. That's because IWYU needs to have all protobuf
and KRPC headers available when running.
Change-Id: I48833098553ef7d3f4fae8b88079cb354b52bea1
---
M CMakeLists.txt
M src/kudu/cfile/CMakeLists.txt
M src/kudu/client/CMakeLists.txt
M src/kudu/common/CMakeLists.txt
M src/kudu/consensus/CMakeLists.txt
M src/kudu/fs/CMakeLists.txt
M src/kudu/master/CMakeLists.txt
M src/kudu/rpc/CMakeLists.txt
M src/kudu/security/CMakeLists.txt
M src/kudu/server/CMakeLists.txt
M src/kudu/tablet/CMakeLists.txt
M src/kudu/tserver/CMakeLists.txt
M src/kudu/util/CMakeLists.txt
13 files changed, 99 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/7777/2
--
To view, visit http://gerrit.cloudera.org:8080/7777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I48833098553ef7d3f4fae8b88079cb354b52bea1
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [cmake] introduce 'pb-gen' and 'krpc-gen' targets
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [cmake] introduce 'pb-gen' and 'krpc-gen' targets
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7777/2/CMakeLists.txt
File CMakeLists.txt:
Line 637: function(KUDU_PROTOBUF_GENERATE_CPP SRCS HDRS TGTS)
> The point here is to automatically add dependencies to some top-level targe
I meant:
If updating the definition of the targets in the cmake module file as you suggested, which option from the mentioned above is the best in your opinion?
--
To view, visit http://gerrit.cloudera.org:8080/7777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I48833098553ef7d3f4fae8b88079cb354b52bea1
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [cmake] introduce 'pb-gen' and 'krpc-gen' targets
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [cmake] introduce 'pb-gen' and 'krpc-gen' targets
......................................................................
Patch Set 4: Verified+1
unrelated flake in
AdminCliTest.TestMoveTablet
--
To view, visit http://gerrit.cloudera.org:8080/7777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I48833098553ef7d3f4fae8b88079cb354b52bea1
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No