You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Joe McDonnell (Code Review)" <ge...@cloudera.org> on 2020/05/28 18:01:50 UTC

[Impala-ASF-CR] IMPALA-9760: Add NATIVE PACKAGES HOME to prepare for GCC7

Joe McDonnell has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15991


Change subject: IMPALA-9760: Add NATIVE_PACKAGES_HOME to prepare for GCC7
......................................................................

IMPALA-9760: Add NATIVE_PACKAGES_HOME to prepare for GCC7

The locations for native packages in IMPALA_TOOLCHAIN currently
do not include the compiler version. This means that the
toolchain can't distinguish between native packages built
with gcc 4.9.2 versus gcc 7.5.0. The collisions can cause
issues when switching back and forth between branches.

This introduces the NATIVE_PACKAGES_HOME environment variable,
which is a location inside IMPALA_TOOLCHAIN that would hold
native packages. Currently, it is set to the same as
IMPALA_TOOLCHAIN, so there is no difference in behavior.
This lays the groundwork to add the compiler version to this
path when switching to GCC7.

Testing:
 - Built with NATIVE_PACKAGES_HOME=$IMPALA_TOOLCHAIN/test
   and verified that it will work with a path other than
   $IMPALA_TOOLCHAIN.
 - Core tests

Change-Id: I1ff641e503b2161baf415355452f86b6c8bfb15b
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/service/CMakeLists.txt
M bin/bootstrap_toolchain.py
M bin/distcc/distcc.sh
M bin/dump_breakpad_symbols.py
M bin/impala-config.sh
M bin/impala-shell.sh
M bin/jenkins/finalize.sh
M bin/run-backend-tests.sh
M bin/run-binary.sh
M bin/run-jvm-binary.sh
M bin/run_clang_tidy.sh
M bin/set-ld-library-path.sh
M bin/set-pythonpath.sh
M cmake_modules/clang_toolchain.cmake
M cmake_modules/toolchain.cmake
M docker/setup_build_context.py
M fe/pom.xml
M infra/python/bootstrap_virtualenv.py
M shell/make_shell_tarball.sh
M shell/packaging/make_python_package.sh
M testdata/datasets/tpcds/preload
M testdata/datasets/tpch/preload
24 files changed, 90 insertions(+), 77 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/15991/1
-- 
To view, visit http://gerrit.cloudera.org:8080/15991
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1ff641e503b2161baf415355452f86b6c8bfb15b
Gerrit-Change-Number: 15991
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-9760: Add NATIVE PACKAGES HOME to prepare for GCC7

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15991 )

Change subject: IMPALA-9760: Add NATIVE_PACKAGES_HOME to prepare for GCC7
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15991/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/15991/2//COMMIT_MSG@7
PS2, Line 7: IMPALA-9760: Add NATIVE_PACKAGES_HOME to prepare for GCC7
It's a little unfortunate that we decided to nest cd*_components underneath the toolchain directory, when it's not really part of the toolchain. It makes the naming here a little weird, since IMPALA_TOOLCHAIN is now a root directory for the actual toolchain, plus other stuff that is not really the toolchain, and we need a new name to describe the toolchain.

I think the overall strategy is fine and makes more sense than trying to change the meaning of IMPALA_TOOLCHAIN and potentially breaking people's configs.

Could also consider calling it IMPALA_TOOLCHAIN_PACKAGES_HOME or something like that to make it clearer.



-- 
To view, visit http://gerrit.cloudera.org:8080/15991
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ff641e503b2161baf415355452f86b6c8bfb15b
Gerrit-Change-Number: 15991
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 May 2020 20:51:21 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9760: Add IMPALA TOOLCHAIN PACKAGES HOME to prepare for GCC7

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Hello Tim Armstrong, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15991

to look at the new patch set (#4).

Change subject: IMPALA-9760: Add IMPALA_TOOLCHAIN_PACKAGES_HOME to prepare for GCC7
......................................................................

IMPALA-9760: Add IMPALA_TOOLCHAIN_PACKAGES_HOME to prepare for GCC7

The locations for native-toolchain packages in IMPALA_TOOLCHAIN
currently do not include the compiler version. This means that
the toolchain can't distinguish between native-toolchain packages
built with gcc 4.9.2 versus gcc 7.5.0. The collisions can cause
issues when switching back and forth between branches.

This introduces the IMPALA_TOOLCHAIN_PACKAGES_HOME environment
variable, which is a location inside IMPALA_TOOLCHAIN that would
hold native-toolchain packages. Currently, it is set to the same
as IMPALA_TOOLCHAIN, so there is no difference in behavior.
This lays the groundwork to add the compiler version to this
path when switching to GCC7.

Testing:
 - The only impediment to building with
   IMPALA_TOOLCHAIN_PACKAGES_HOME=$IMPALA_TOOLCHAIN/test is
   Impala-lzo. All other code works, and Impala-lzo can either be
   modified to work or be removed.
 - Core tests

Change-Id: I1ff641e503b2161baf415355452f86b6c8bfb15b
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/service/CMakeLists.txt
M bin/bootstrap_toolchain.py
M bin/distcc/distcc.sh
M bin/dump_breakpad_symbols.py
M bin/impala-config.sh
M bin/impala-shell.sh
M bin/jenkins/finalize.sh
M bin/run-backend-tests.sh
M bin/run-binary.sh
M bin/run-jvm-binary.sh
M bin/run_clang_tidy.sh
M bin/set-ld-library-path.sh
M bin/set-pythonpath.sh
M cmake_modules/clang_toolchain.cmake
M cmake_modules/toolchain.cmake
M docker/setup_build_context.py
M fe/pom.xml
M infra/python/bootstrap_virtualenv.py
M shell/make_shell_tarball.sh
M shell/packaging/make_python_package.sh
M testdata/datasets/tpcds/preload
M testdata/datasets/tpch/preload
24 files changed, 104 insertions(+), 77 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/15991/4
-- 
To view, visit http://gerrit.cloudera.org:8080/15991
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1ff641e503b2161baf415355452f86b6c8bfb15b
Gerrit-Change-Number: 15991
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-9760: Add IMPALA TOOLCHAIN PACKAGES HOME to prepare for GCC7

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Hello Tim Armstrong, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15991

to look at the new patch set (#5).

Change subject: IMPALA-9760: Add IMPALA_TOOLCHAIN_PACKAGES_HOME to prepare for GCC7
......................................................................

IMPALA-9760: Add IMPALA_TOOLCHAIN_PACKAGES_HOME to prepare for GCC7

The locations for native-toolchain packages in IMPALA_TOOLCHAIN
currently do not include the compiler version. This means that
the toolchain can't distinguish between native-toolchain packages
built with gcc 4.9.2 versus gcc 7.5.0. The collisions can cause
issues when switching back and forth between branches.

This introduces the IMPALA_TOOLCHAIN_PACKAGES_HOME environment
variable, which is a location inside IMPALA_TOOLCHAIN that would
hold native-toolchain packages. Currently, it is set to the same
as IMPALA_TOOLCHAIN, so there is no difference in behavior.
This lays the groundwork to add the compiler version to this
path when switching to GCC7.

Testing:
 - The only impediment to building with
   IMPALA_TOOLCHAIN_PACKAGES_HOME=$IMPALA_TOOLCHAIN/test is
   Impala-lzo. With a custom Impala-lzo, compilation succeeds.
   Either Impala-lzo will be fixed or it will be removed.
 - Core tests

Change-Id: I1ff641e503b2161baf415355452f86b6c8bfb15b
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/service/CMakeLists.txt
M bin/bootstrap_toolchain.py
M bin/distcc/distcc.sh
M bin/dump_breakpad_symbols.py
M bin/impala-config.sh
M bin/impala-shell.sh
M bin/jenkins/finalize.sh
M bin/run-backend-tests.sh
M bin/run-binary.sh
M bin/run-jvm-binary.sh
M bin/run_clang_tidy.sh
M bin/set-ld-library-path.sh
M bin/set-pythonpath.sh
M cmake_modules/clang_toolchain.cmake
M cmake_modules/toolchain.cmake
M docker/setup_build_context.py
M fe/pom.xml
M infra/python/bootstrap_virtualenv.py
M shell/make_shell_tarball.sh
M shell/packaging/make_python_package.sh
M testdata/datasets/tpcds/preload
M testdata/datasets/tpch/preload
24 files changed, 105 insertions(+), 78 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/15991/5
-- 
To view, visit http://gerrit.cloudera.org:8080/15991
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1ff641e503b2161baf415355452f86b6c8bfb15b
Gerrit-Change-Number: 15991
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-9760: Add IMPALA TOOLCHAIN PACKAGES HOME to prepare for GCC7

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15991 )

Change subject: IMPALA-9760: Add IMPALA_TOOLCHAIN_PACKAGES_HOME to prepare for GCC7
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/15991/3/bin/impala-config.sh
File bin/impala-config.sh:

http://gerrit.cloudera.org:8080/#/c/15991/3/bin/impala-config.sh@677
PS3, Line 677: export ASAN_SYMBOLIZER_PATH="${IMPALA_TOOLCHAIN_PACKAGES_HOME}/llvm-${IMPALA_LLVM_ASAN_VERSION}/bin/llvm-symbolizer"
line too long (116 > 90)


http://gerrit.cloudera.org:8080/#/c/15991/3/bin/jenkins/finalize.sh
File bin/jenkins/finalize.sh:

http://gerrit.cloudera.org:8080/#/c/15991/3/bin/jenkins/finalize.sh@49
PS3, Line 49:     $IMPALA_TOOLCHAIN_PACKAGES_HOME/breakpad-$IMPALA_BREAKPAD_VERSION/bin/minidump_stackwalk \
line too long (94 > 90)



-- 
To view, visit http://gerrit.cloudera.org:8080/15991
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ff641e503b2161baf415355452f86b6c8bfb15b
Gerrit-Change-Number: 15991
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 May 2020 01:06:22 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9760: Add IMPALA TOOLCHAIN PACKAGES HOME to prepare for GCC7

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15991 )

Change subject: IMPALA-9760: Add IMPALA_TOOLCHAIN_PACKAGES_HOME to prepare for GCC7
......................................................................


Patch Set 3: Code-Review+2

LGTM once you've added those comments, thanks for taking this on


-- 
To view, visit http://gerrit.cloudera.org:8080/15991
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ff641e503b2161baf415355452f86b6c8bfb15b
Gerrit-Change-Number: 15991
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 May 2020 19:26:02 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9760: Add IMPALA TOOLCHAIN PACKAGES HOME to prepare for GCC7

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15991 )

Change subject: IMPALA-9760: Add IMPALA_TOOLCHAIN_PACKAGES_HOME to prepare for GCC7
......................................................................


Patch Set 4:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/6173/ : 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/15991
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ff641e503b2161baf415355452f86b6c8bfb15b
Gerrit-Change-Number: 15991
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 30 May 2020 03:20:57 +0000
Gerrit-HasComments: No