You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Henry Robinson (Code Review)" <ge...@cloudera.org> on 2017/01/10 01:40:17 UTC
[Impala-ASF-CR] Add Kudu cmake utilities
Henry Robinson has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5656
Change subject: Add Kudu cmake utilities
......................................................................
Add Kudu cmake utilities
This commit imports some CMake utility methods from Kudu, in preparation
for adding KRPC and its dependencies to Impala's build.
The methods are unused in this patch, but will be used both by
thirdparty dependencies (e.g. Protobuf) and by the Kudu libraries
themselves.
Some methods are stubbed out to make it easier to import Kudu's
CMakeLists.txt files without adding extra test targets etc. to Impala's
build.
Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
---
M CMakeLists.txt
A cmake_modules/kudu_cmake_fns.txt
2 files changed, 177 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/5656/1
--
To view, visit http://gerrit.cloudera.org:8080/5656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] Add Kudu cmake utilities
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: Add Kudu cmake utilities
......................................................................
Patch Set 2: Code-Review+1
Will let MJ +2 it
--
To view, visit http://gerrit.cloudera.org:8080/5656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] Add Kudu cmake utilities
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: Add Kudu cmake utilities
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5656/1/cmake_modules/kudu_cmake_fns.txt
File cmake_modules/kudu_cmake_fns.txt:
Line 18: # Require cmake that can build LLVM [1].
> This comment doesn't seem relevant to us.
It would also be good to document the provenance of this file in case someone needs to update it.
--
To view, visit http://gerrit.cloudera.org:8080/5656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Add Kudu cmake utilities
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: Add Kudu cmake utilities
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5656/1/cmake_modules/kudu_cmake_fns.txt
File cmake_modules/kudu_cmake_fns.txt:
Line 18: # Require cmake that can build LLVM [1].
This comment doesn't seem relevant to us.
--
To view, visit http://gerrit.cloudera.org:8080/5656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Add Kudu cmake utilities
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: Add Kudu cmake utilities
......................................................................
Patch Set 3:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/170/
--
To view, visit http://gerrit.cloudera.org:8080/5656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] Add Kudu cmake utilities
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5656
to look at the new patch set (#3).
Change subject: Add Kudu cmake utilities
......................................................................
Add Kudu cmake utilities
This commit imports some CMake utility methods from Kudu, in preparation
for adding KRPC and its dependencies to Impala's build.
The methods are unused in this patch, but will be used both by
thirdparty dependencies (e.g. Protobuf) and by the Kudu libraries
themselves.
Some methods are stubbed out to make it easier to import Kudu's
CMakeLists.txt files without adding extra test targets etc. to Impala's
build.
Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
---
M CMakeLists.txt
A cmake_modules/kudu_cmake_fns.txt
2 files changed, 160 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/5656/3
--
To view, visit http://gerrit.cloudera.org:8080/5656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] Add Kudu cmake utilities
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: Add Kudu cmake utilities
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5656/2/cmake_modules/kudu_cmake_fns.txt
File cmake_modules/kudu_cmake_fns.txt:
PS2, Line 30: # There are two different kinds of exported libraries: internal and leaf.
: # Internal libraries are static archives while leaf libraries are shared
: # objects built from internal libraries.
> Why do we need this? Maybe I'm missing context- Is there a doc or readme ex
These are needed to so that we can import kudu's CMakeLists.txt without significant alteration.
See https://github.com/henryr/Impala/blob/0ef5ce0a18eb734dc8caedfca2752dfd4f513ba2/be/src/kudu/util/CMakeLists.txt for the main case where they're used.
--
To view, visit http://gerrit.cloudera.org:8080/5656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Add Kudu cmake utilities
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: Add Kudu cmake utilities
......................................................................
Patch Set 1:
(3 comments)
docker images/dev setup puppet script may need to be updated to use the new cmake
http://gerrit.cloudera.org:8080/#/c/5656/1/CMakeLists.txt
File CMakeLists.txt:
PS1, Line 17:
: cmake_minimum_required(VERSION 3.2.3)
is this installed on our build machines?
http://gerrit.cloudera.org:8080/#/c/5656/1/cmake_modules/kudu_cmake_fns.txt
File cmake_modules/kudu_cmake_fns.txt:
PS1, Line 19:
: # Note: cmake in thirdparty/ will always meet this minimum.
this comment doesn't seem to make sense for us either
PS1, Line 27: exported Kudu C++ client
remove client reference
--
To view, visit http://gerrit.cloudera.org:8080/5656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Add Kudu cmake utilities
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: Add Kudu cmake utilities
......................................................................
Patch Set 3: Code-Review+2
I think this makes more sense now, thanks.
--
To view, visit http://gerrit.cloudera.org:8080/5656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] Add Kudu cmake utilities
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: Add Kudu cmake utilities
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/5656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] Add Kudu cmake utilities
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: Add Kudu cmake utilities
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5656/2/cmake_modules/kudu_cmake_fns.txt
File cmake_modules/kudu_cmake_fns.txt:
PS2, Line 26: # add_library() wrapper that adds a second variant of the library, suffixed with
: # "_exported" and which is compiled with special visibility flags to hide all symbols
: # except those that are part of the public ABI.
this doesn't seem to be true; the code differs from the kudu impl
PS2, Line 30: # There are two different kinds of exported libraries: internal and leaf.
: # Internal libraries are static archives while leaf libraries are shared
: # objects built from internal libraries.
Why do we need this? Maybe I'm missing context- Is there a doc or readme explaining how the kudu libraries in impala will be included/linked/work?
--
To view, visit http://gerrit.cloudera.org:8080/5656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Add Kudu cmake utilities
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has uploaded a new patch set (#2).
Change subject: Add Kudu cmake utilities
......................................................................
Add Kudu cmake utilities
This commit imports some CMake utility methods from Kudu, in preparation
for adding KRPC and its dependencies to Impala's build.
The methods are unused in this patch, but will be used both by
thirdparty dependencies (e.g. Protobuf) and by the Kudu libraries
themselves.
Some methods are stubbed out to make it easier to import Kudu's
CMakeLists.txt files without adding extra test targets etc. to Impala's
build.
Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
---
M CMakeLists.txt
A cmake_modules/kudu_cmake_fns.txt
2 files changed, 175 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/56/5656/2
--
To view, visit http://gerrit.cloudera.org:8080/5656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] Add Kudu cmake utilities
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: Add Kudu cmake utilities
......................................................................
Add Kudu cmake utilities
This commit imports some CMake utility methods from Kudu, in preparation
for adding KRPC and its dependencies to Impala's build.
The methods are unused in this patch, but will be used both by
thirdparty dependencies (e.g. Protobuf) and by the Kudu libraries
themselves.
Some methods are stubbed out to make it easier to import Kudu's
CMakeLists.txt files without adding extra test targets etc. to Impala's
build.
Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
Reviewed-on: http://gerrit.cloudera.org:8080/5656
Reviewed-by: Matthew Jacobs <mj...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M CMakeLists.txt
A cmake_modules/kudu_cmake_fns.txt
2 files changed, 160 insertions(+), 1 deletion(-)
Approvals:
Impala Public Jenkins: Verified
Matthew Jacobs: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/5656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] Add Kudu cmake utilities
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: Add Kudu cmake utilities
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5656/1/CMakeLists.txt
File CMakeLists.txt:
PS1, Line 17:
: cmake_minimum_required(VERSION 3.2.3)
> is this installed on our build machines?
The toolchain has cmake 3.2.3.
http://gerrit.cloudera.org:8080/#/c/5656/1/cmake_modules/kudu_cmake_fns.txt
File cmake_modules/kudu_cmake_fns.txt:
Line 18: # Require cmake that can build LLVM [1].
> It would also be good to document the provenance of this file in case someo
Done
PS1, Line 19:
: # Note: cmake in thirdparty/ will always meet this minimum.
> this comment doesn't seem to make sense for us either
Done
PS1, Line 27: exported Kudu C++ client
> remove client reference
Done
--
To view, visit http://gerrit.cloudera.org:8080/5656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Add Kudu cmake utilities
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: Add Kudu cmake utilities
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5656/2/cmake_modules/kudu_cmake_fns.txt
File cmake_modules/kudu_cmake_fns.txt:
PS2, Line 26: # add_library() wrapper that adds a second variant of the library, suffixed with
: # "_exported" and which is compiled with special visibility flags to hide all symbols
: # except those that are part of the public ABI.
> this doesn't seem to be true; the code differs from the kudu impl
Doh, you're right; I've had this file in my branch so long I misremembered its provenance slightly. ADD_EXPORTED_LIBRARY is mostly just a wrapper for add_library() now. I updated the comments to that effect.
--
To view, visit http://gerrit.cloudera.org:8080/5656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes