Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12847 )
Change subject: [KUDU-1344] Part 1: cmake install for executables ...................................................................... Patch Set 2: (11 comments) Could you provide a rendering of the new installation.adoc? http://gerrit.cloudera.org:8080/#/c/12847/2/docs/installation.adoc File docs/installation.adoc: PS2: Some of the new lines are too long; can you wrap? http://gerrit.cloudera.org:8080/#/c/12847/2/docs/installation.adoc@172 PS2, Line 172: * Kudu client library in `/usr/local/lib/` You sure this lands in lib and not lib64? http://gerrit.cloudera.org:8080/#/c/12847/2/docs/installation.adoc@295 PS2, Line 295: * Kudu client library in `/usr/local/lib/` You sure this lands in lib and not something with x86_64 in it? http://gerrit.cloudera.org:8080/#/c/12847/2/docs/installation.adoc@406 PS2, Line 406: * Kudu client library in `/usr/local/lib/` Likewise I thought this might end up in lib64. http://gerrit.cloudera.org:8080/#/c/12847/2/src/kudu/master/CMakeLists.txt File src/kudu/master/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/12847/2/src/kudu/master/CMakeLists.txt@101 PS2, Line 101: kudu-master Nit: "the Kudu Master executable" http://gerrit.cloudera.org:8080/#/c/12847/2/src/kudu/master/CMakeLists.txt@104 PS2, Line 104: else(KUDU_MASTER_INSTALL) You can omit KUDU_MASTER_INSTALL from the body of the else() and endif(). Elsewhere too. http://gerrit.cloudera.org:8080/#/c/12847/2/src/kudu/tools/CMakeLists.txt File src/kudu/tools/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/12847/2/src/kudu/tools/CMakeLists.txt@143 PS2, Line 143: option(KUDU_CLIENT_INSTALL "Whether to install the Kudu client executable" ON) It's not really the "client executable" as the "CLI" or "command line tool". http://gerrit.cloudera.org:8080/#/c/12847/2/src/kudu/tools/CMakeLists.txt@148 PS2, Line 148: endif(KUDU_CLIENT_INSTALL) Nit: separate from the next line with an empty line. http://gerrit.cloudera.org:8080/#/c/12847/2/src/kudu/tserver/CMakeLists.txt File src/kudu/tserver/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/12847/2/src/kudu/tserver/CMakeLists.txt@a142 PS2, Line 142: Could you restore this? I think it's nice to see these "headers" separate the actual logic with empty lines. http://gerrit.cloudera.org:8080/#/c/12847/2/src/kudu/tserver/CMakeLists.txt@149 PS2, Line 149: kudu-tserver "the Kudu Tablet Server executable" http://gerrit.cloudera.org:8080/#/c/12847/2/src/kudu/tserver/CMakeLists.txt@154 PS2, Line 154: endif(KUDU_TSERVER_INSTALL) Nit: separate with a blank line. -- To view, visit http://gerrit.cloudera.org:8080/12847 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic020941343efc9192bf615c0c8a30b3d9eee91d7 Gerrit-Change-Number: 12847 Gerrit-PatchSet: 2 Gerrit-Owner: Greg Solovyev <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Greg Solovyev <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 27 Mar 2019 03:31:44 +0000 Gerrit-HasComments: Yes
