You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2019/05/08 21:38:00 UTC
[native-toolchain-CR] Update cmake to 3.14.3
Hello Tim Armstrong,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/13286
to review the following change.
Change subject: Update cmake to 3.14.3
......................................................................
Update cmake to 3.14.3
This updates to the latest cmake, which includes a few improvements that
may be helpful. In particular, the Ninja generator avoids generating
some false dependencies[1]
Instead of carrying over the patch to change one of the #defines in the
CMake source, this uses a newly-provided variable when building CMake
to accomplish the same.
I tested building Impala with this version of CMake on el7 and it worked
fine.
[1] https://gitlab.kitware.com/cmake/cmake/merge_requests/430
Change-Id: I1ad46a7b820bbb727ce47219f03f1d4a36301ab4
---
M init.sh
M source/cmake/build.sh
2 files changed, 4 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/86/13286/1
--
To view, visit http://gerrit.cloudera.org:8080/13286
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1ad46a7b820bbb727ce47219f03f1d4a36301ab4
Gerrit-Change-Number: 13286
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[native-toolchain-CR] Update cmake to 3.14.3
Posted by "Laszlo Gaal (Code Review)" <ge...@cloudera.org>.
Laszlo Gaal has posted comments on this change. ( http://gerrit.cloudera.org:8080/13286 )
Change subject: Update cmake to 3.14.3
......................................................................
Patch Set 1:
The cmake tweak symbol should be KWSYSPE_USE_SELECT instead of KWSYSPE_PROCESS_USE_SELECT. I have a build on private infra that passed with this change; can push a PS if you want.
--
To view, visit http://gerrit.cloudera.org:8080/13286
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ad46a7b820bbb727ce47219f03f1d4a36301ab4
Gerrit-Change-Number: 13286
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 13 May 2019 22:46:57 +0000
Gerrit-HasComments: No
[native-toolchain-CR] Update cmake to 3.14.3
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13286 )
Change subject: Update cmake to 3.14.3
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13286
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ad46a7b820bbb727ce47219f03f1d4a36301ab4
Gerrit-Change-Number: 13286
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 May 2019 00:11:26 +0000
Gerrit-HasComments: No
[native-toolchain-CR] Update cmake to 3.14.3
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13286 )
Change subject: Update cmake to 3.14.3
......................................................................
Patch Set 1:
For those watching at home, it looks like this hit a build failure when testing it:
[ 17%] Building C object Source/CursesDialog/form/CMakeFiles/cmForm.dir/fty_alnum.c.o
/mnt/source/cmake/cmake-3.14.3/Source/kwsys/ProcessUNIX.c:270:23: error: #if with no expression
#if KWSYSPE_USE_SELECT
--
To view, visit http://gerrit.cloudera.org:8080/13286
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ad46a7b820bbb727ce47219f03f1d4a36301ab4
Gerrit-Change-Number: 13286
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 13 May 2019 18:19:21 +0000
Gerrit-HasComments: No
[native-toolchain-CR] Update cmake to 3.14.3
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13286 )
Change subject: Update cmake to 3.14.3
......................................................................
Update cmake to 3.14.3
This updates to the latest cmake, which includes a few improvements that
may be helpful. In particular, the Ninja generator avoids generating
some false dependencies[1]
Instead of carrying over the patch to change one of the #defines in the
CMake source, this uses a newly-provided variable when building CMake
to accomplish the same.
I tested building Impala with this version of CMake on el7 and it worked
fine.
[1] https://gitlab.kitware.com/cmake/cmake/merge_requests/430
Change-Id: I1ad46a7b820bbb727ce47219f03f1d4a36301ab4
Reviewed-on: http://gerrit.cloudera.org:8080/13286
Reviewed-by: Laszlo Gaal <la...@cloudera.com>
Tested-by: Tim Armstrong <ta...@cloudera.com>
---
M init.sh
M source/cmake/build.sh
2 files changed, 7 insertions(+), 2 deletions(-)
Approvals:
Laszlo Gaal: Looks good to me, approved
Tim Armstrong: Verified
--
To view, visit http://gerrit.cloudera.org:8080/13286
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I1ad46a7b820bbb727ce47219f03f1d4a36301ab4
Gerrit-Change-Number: 13286
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[native-toolchain-CR] Update cmake to 3.14.3
Posted by "Laszlo Gaal (Code Review)" <ge...@cloudera.org>.
Laszlo Gaal has posted comments on this change. ( http://gerrit.cloudera.org:8080/13286 )
Change subject: Update cmake to 3.14.3
......................................................................
Patch Set 2: Code-Review+2
The last patch set passed our internal automated build.
The two (remarkably similar) symbol names look more like bad design than a bug, with the goal to distinguish two different roles:
- KWSYS_PROCESS_USE_SELECT gates whether you can pass in a preference for using select() calls at all.
- KWSYSPE_USE_SELECT is the actual preprocessor symbol that disables some part of the code.
But it doesn't really matter; at the end of the day you still need to define both (and both with a value) to get the desired result.
--
To view, visit http://gerrit.cloudera.org:8080/13286
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ad46a7b820bbb727ce47219f03f1d4a36301ab4
Gerrit-Change-Number: 13286
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 14 May 2019 20:11:00 +0000
Gerrit-HasComments: No
[native-toolchain-CR] Update cmake to 3.14.3
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13286 )
Change subject: Update cmake to 3.14.3
......................................................................
Patch Set 1:
Oh, looks like maybe it's a bug in the CMakeLists.txt. the if() statement uses KWSYS_PROCESS_USE_SELECT but the substitution is KWSYSPE_USE_SELECT, so I think we need to define both :(
--
To view, visit http://gerrit.cloudera.org:8080/13286
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ad46a7b820bbb727ce47219f03f1d4a36301ab4
Gerrit-Change-Number: 13286
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 13 May 2019 22:50:06 +0000
Gerrit-HasComments: No
[native-toolchain-CR] Update cmake to 3.14.3
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13286 )
Change subject: Update cmake to 3.14.3
......................................................................
Patch Set 1:
I don't think that's right. Looking at the CMakeLists.txt file, it has:
IF(DEFINED KWSYS_PROCESS_USE_SELECT)
GET_PROPERTY(ProcessUNIX_FLAGS SOURCE ProcessUNIX.c PROPERTY COMPILE_FLAGS)
SET_PROPERTY(SOURCE ProcessUNIX.c PROPERTY COMPILE_FLAGS "${ProcessUNIX_FLAGS} -DKWSYSPE_USE_SELECT=${KWSYSPE_USE_SELECT}")
ENDIF()
So the correct one is KWSYS_PROCESS_USE_SELECT. Not sure why the build failed with that one.
--
To view, visit http://gerrit.cloudera.org:8080/13286
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ad46a7b820bbb727ce47219f03f1d4a36301ab4
Gerrit-Change-Number: 13286
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 13 May 2019 22:48:59 +0000
Gerrit-HasComments: No
[native-toolchain-CR] Update cmake to 3.14.3
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Laszlo Gaal, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13286
to look at the new patch set (#2).
Change subject: Update cmake to 3.14.3
......................................................................
Update cmake to 3.14.3
This updates to the latest cmake, which includes a few improvements that
may be helpful. In particular, the Ninja generator avoids generating
some false dependencies[1]
Instead of carrying over the patch to change one of the #defines in the
CMake source, this uses a newly-provided variable when building CMake
to accomplish the same.
I tested building Impala with this version of CMake on el7 and it worked
fine.
[1] https://gitlab.kitware.com/cmake/cmake/merge_requests/430
Change-Id: I1ad46a7b820bbb727ce47219f03f1d4a36301ab4
---
M init.sh
M source/cmake/build.sh
2 files changed, 7 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/86/13286/2
--
To view, visit http://gerrit.cloudera.org:8080/13286
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1ad46a7b820bbb727ce47219f03f1d4a36301ab4
Gerrit-Change-Number: 13286
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[native-toolchain-CR] Update cmake to 3.14.3
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13286 )
Change subject: Update cmake to 3.14.3
......................................................................
Patch Set 2: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/13286
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ad46a7b820bbb727ce47219f03f1d4a36301ab4
Gerrit-Change-Number: 13286
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 14 May 2019 20:32:15 +0000
Gerrit-HasComments: No