Adar Dembo has posted comments on this change.

Change subject: doxygen for C++ client API

Patch Set 8:

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 
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 
Why this?
File thirdparty/

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Dinesh Bhat <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <>
Gerrit-HasComments: Yes

Reply via email to