Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9629 )
Change subject: IMPALA-3307: Build CCTZ lib in toolchain ...................................................................... Patch Set 1: (5 comments) Had a few comments about understandability and maintainability. http://gerrit.cloudera.org:8080/#/c/9629/1/source/cctz/build.sh File source/cctz/build.sh: http://gerrit.cloudera.org:8080/#/c/9629/1/source/cctz/build.sh@31 PS1, Line 31: # build Comment is not very helpful :). Do we need to create a separate build subdirectory or is it just to keep things cleaner. It's worth leaving a short comment, since I wasn't sure the motivation when reading it. http://gerrit.cloudera.org:8080/#/c/9629/1/source/cctz/build.sh@32 PS1, Line 32: mkdir build || true Use "mkdir -p" and avoid the "|| true" if you want it succeed if the directory is already present but fail if creating the directory fails. http://gerrit.cloudera.org:8080/#/c/9629/1/source/cctz/build.sh@38 PS1, Line 38: wrap cmake --build . --config Release It seems like "cmake --build" is an alternative way to call out to "make". Why is this line necessary when we're already calling make below? http://gerrit.cloudera.org:8080/#/c/9629/1/source/cctz/build.sh@39 PS1, Line 39: -C . Does "-C ." do anything? It seems like changing to the current directory is a no-op. http://gerrit.cloudera.org:8080/#/c/9629/1/source/cctz/cctz-2.2-patches/0001-Build-static-shared-libraries.patch File source/cctz/cctz-2.2-patches/0001-Build-static-shared-libraries.patch: PS1: Can we upstream this patch or avoid it? I'd like to avoid maintaining it ourselves, since we'll need to update it to resolve conflicts if the upstream CMakeLists.txt changes. It looks like this patch would already have conflicts because of this change: https://github.com/google/cctz/commit/8768b6d02283f6226527c1a7fb39c382ddfb4cec#diff-af3b638bc2a3e6c650974192a53c7291 Worst case it seems like we can set CFLAGS/CXXFLAGS to include -fPIC and build CCTZ twice to get the two libraries. It's a pretty small library so the overhead of doing that seems ok. -- To view, visit http://gerrit.cloudera.org:8080/9629 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I12127e7862b76822c15d65d1dd6baf2b74202dd2 Gerrit-Change-Number: 9629 Gerrit-PatchSet: 1 Gerrit-Owner: Attila Jeges <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 14 Mar 2018 19:10:12 +0000 Gerrit-HasComments: Yes
