Adar Dembo has posted comments on this change. Change subject: doxygen for C++ client API ......................................................................
Patch Set 8: (6 comments) http://gerrit.cloudera.org:8080/#/c/3619/8/CMakeLists.txt File CMakeLists.txt: Line 983: set(DOXY_SUBDIR ${CMAKE_CURRENT_BINARY_DIR}/docs/doxygen) Could you first log what the location of doxygen is? Maybe logging DOXYGEN_EXECUTABLE is sufficient, not sure. Line 989: add_custom_target(doxy_install_client_alt_destdir What if doxy_install_client_alt_destdir depended on 'all'? Then could you drop the explicit "make all" step? I think that would be better, as it'd feed more dependency information into the generated makefiles ("make all install" as a command doesn't do that). Also, I don't think you'd need the remove_directory commands then either. Line 990: COMMAND ${CMAKE_COMMAND} -E remove_directory ${DOXY_CLIENT_DESTDIR} Why do we need to remove the old directory first? Line 992: WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/src/kudu/client Why does the working directory need to be modified for this command? Line 997: COMMAND ${CMAKE_COMMAND} -E remove_directory ${DOXY_CLIENT_DESTDIR} Why this? http://gerrit.cloudera.org:8080/#/c/3619/8/thirdparty/download-thirdparty.sh File thirdparty/download-thirdparty.sh: PS8, Line 63: if [[ "$FILENAME" =~ \.zip$ ]]; then : if ! unzip -q "$FILENAME"; then : echo "Error unzipping $FILENAME, removing file" : rm "$FILENAME" : continue : fi : elif [[ "$FILENAME" =~ \.(tar\.gz|tgz)$ ]]; then Are these changes still useful? Or can they be reverted? They seem cosmetic to me, but maybe I'm missing something. -- To view, visit http://gerrit.cloudera.org:8080/3619 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-HasComments: Yes
