[Impala-ASF-CR] IMPALA-8147: part 1/2: make make *.sh redundant.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12368 ) Change subject: IMPALA-8147: part 1/2: make make_*.sh redundant. .. IMPALA-8147: part 1/2: make make_*.sh redundant. The logic in that file, which is mostly about constructing argument lists for CMake and make, is moved to functions in buildall.sh. A new option -release_and_debug is added to buildall.sh to build both the debug and release builds. This is convenient for building a binary Impala for distribution because you want to have both sets of binaries available. make*.sh are not yet removed in order to make the transition easier. Testing: Ran buildall.sh locally with -release_and_debug, confirmed that all of the right binaries were generated. Change-Id: I70e4f65712166348ca006bc68e1a1e18e853d3a0 Reviewed-on: http://gerrit.cloudera.org:8080/12368 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/common/CMakeLists.txt M bin/distcc/README.md M bin/make_impala.sh M buildall.sh 4 files changed, 90 insertions(+), 26 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/12368 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I70e4f65712166348ca006bc68e1a1e18e853d3a0 Gerrit-Change-Number: 12368 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8147: part 1/2: make make *.sh redundant.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12368 ) Change subject: IMPALA-8147: part 1/2: make make_*.sh redundant. .. Patch Set 3: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12368 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I70e4f65712166348ca006bc68e1a1e18e853d3a0 Gerrit-Change-Number: 12368 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 08 Feb 2019 03:54:39 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8147: part 1/2: make make *.sh redundant.
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/12368 )
Change subject: IMPALA-8147: part 1/2: make make_*.sh redundant.
..
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12368/2/buildall.sh
File buildall.sh:
http://gerrit.cloudera.org:8080/#/c/12368/2/buildall.sh@389
PS2, Line 389: generate_cmake_files $build_type
> Are we trying to make repeated invocations of buildall faster in any way? T
I wasn't specifically trying to optimise buildall.sh time with this patch, just
make it comparable to the previous buildall.sh, then make_impala.sh workflow.
On my system with ninja rerunning buildall.sh doesn't rebuild all the C++, so
that's good.
buildall.sh does churn away a bit each time generating CMake files though (and
checking toolchain stuff if you don't have SKIP_TOOLCHAIN_BOOTSTRAP=true).
http://gerrit.cloudera.org:8080/#/c/12368/2/buildall.sh@427
PS2, Line 427: if [[ ("$build_type" == "ADDRESS_SANITIZER") \
: || ("$build_type" == "TIDY") \
: || ("$build_type" == "UBSAN") \
: || ("$build_type" == "UBSAN_FULL") \
: || ("$build_type" == "TSAN") ]]; then
:
CMAKE_ARGS+=(-DCMAKE_TOOLCHAIN_FILE=$IMPALA_HOME/cmake_modules/clang_toolchain.cmake)
: else
:
CMAKE_ARGS+=(-DCMAKE_TOOLCHAIN_FILE=$IMPALA_HOME/cmake_modules/toolchain.cmake)
: fi
> It's tempting to push this into CMake itself. Most of this file is passing
This doesn't seem like a bad idea. CMake's FAQ seems to discourage setting the
compiler in the main CMakeLists.txt directly, which I assume also means to
avoid including those toolchain files directly. But I don't know if it really
makes a difference for us since we don't really aim to support building with
arbitrary compilers without changes in the CMake files.
https://gitlab.kitware.com/cmake/community/wikis/FAQ#method-3-avoid-use-set
--
To view, visit http://gerrit.cloudera.org:8080/12368
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I70e4f65712166348ca006bc68e1a1e18e853d3a0
Gerrit-Change-Number: 12368
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Thu, 07 Feb 2019 23:45:06 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8147: part 1/2: make make *.sh redundant.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12368 ) Change subject: IMPALA-8147: part 1/2: make make_*.sh redundant. .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12368 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I70e4f65712166348ca006bc68e1a1e18e853d3a0 Gerrit-Change-Number: 12368 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 07 Feb 2019 23:45:26 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8147: part 1/2: make make *.sh redundant.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12368 ) Change subject: IMPALA-8147: part 1/2: make make_*.sh redundant. .. Patch Set 3: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3738/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/12368 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I70e4f65712166348ca006bc68e1a1e18e853d3a0 Gerrit-Change-Number: 12368 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 07 Feb 2019 23:45:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8147: part 1/2: make make *.sh redundant.
Philip Zeyliger has posted comments on this change. (
http://gerrit.cloudera.org:8080/12368 )
Change subject: IMPALA-8147: part 1/2: make make_*.sh redundant.
..
Patch Set 2: Code-Review+2
(2 comments)
Looks good to me. I took a look at bin/jenkins/build-all-flag-combinations.sh
and it's not obvious that we should add things in there.
http://gerrit.cloudera.org:8080/#/c/12368/2/buildall.sh
File buildall.sh:
http://gerrit.cloudera.org:8080/#/c/12368/2/buildall.sh@389
PS2, Line 389: generate_cmake_files $build_type
Are we trying to make repeated invocations of buildall faster in any way? This
re-creates all the cmake stuff, which maybe makes everything invalid? But maybe
that was already happening before...
http://gerrit.cloudera.org:8080/#/c/12368/2/buildall.sh@427
PS2, Line 427: if [[ ("$build_type" == "ADDRESS_SANITIZER") \
: || ("$build_type" == "TIDY") \
: || ("$build_type" == "UBSAN") \
: || ("$build_type" == "UBSAN_FULL") \
: || ("$build_type" == "TSAN") ]]; then
:
CMAKE_ARGS+=(-DCMAKE_TOOLCHAIN_FILE=$IMPALA_HOME/cmake_modules/clang_toolchain.cmake)
: else
:
CMAKE_ARGS+=(-DCMAKE_TOOLCHAIN_FILE=$IMPALA_HOME/cmake_modules/toolchain.cmake)
: fi
It's tempting to push this into CMake itself. Most of this file is passing
arguments around, but this is really logic. When I worked on one of these
sanitizers, I got super super confused that make_impala had this logic.
Anyway, fine not to do, but kibitzing.
--
To view, visit http://gerrit.cloudera.org:8080/12368
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I70e4f65712166348ca006bc68e1a1e18e853d3a0
Gerrit-Change-Number: 12368
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Thu, 07 Feb 2019 23:22:08 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8147: part 1/2: make make *.sh redundant.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12368 ) Change subject: IMPALA-8147: part 1/2: make make_*.sh redundant. .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2027/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12368 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I70e4f65712166348ca006bc68e1a1e18e853d3a0 Gerrit-Change-Number: 12368 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 07 Feb 2019 22:50:57 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8147: part 1/2: make make *.sh redundant.
Tim Armstrong has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/12368 ) Change subject: IMPALA-8147: part 1/2: make make_*.sh redundant. .. IMPALA-8147: part 1/2: make make_*.sh redundant. The logic in that file, which is mostly about constructing argument lists for CMake and make, is moved to functions in buildall.sh. A new option -release_and_debug is added to buildall.sh to build both the debug and release builds. This is convenient for building a binary Impala for distribution because you want to have both sets of binaries available. make*.sh are not yet removed in order to make the transition easier. Testing: Ran buildall.sh locally with -release_and_debug, confirmed that all of the right binaries were generated. Change-Id: I70e4f65712166348ca006bc68e1a1e18e853d3a0 --- M be/src/common/CMakeLists.txt M bin/distcc/README.md M bin/make_impala.sh M buildall.sh 4 files changed, 90 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/12368/2 -- To view, visit http://gerrit.cloudera.org:8080/12368 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I70e4f65712166348ca006bc68e1a1e18e853d3a0 Gerrit-Change-Number: 12368 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Tim Armstrong
