Adar Dembo has posted comments on this change.

Change subject: doxygen for C++ client API
......................................................................


Patch Set 8:

(5 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)
> The find_package(Doxygen) outputs information on location of doxygen binary
Ah, great. I think that's sufficient.


Line 989:     add_custom_target(doxy_install_client_alt_destdir
> The 'all' and 'install' steps are here is for kudu-client sub-dir, actually
Maybe there's no real 'all' target after all. What about depending on 
'kudu_client_exported'? That's the target that builds the client library.


Line 990:       COMMAND ${CMAKE_COMMAND} -E remove_directory 
${DOXY_CLIENT_DESTDIR}
> This is for cleaning some stale stuff if it's left around when make was int
Hmm, OK. I was hoping that a second "make install" would automatically clear 
out the old state, but maybe it doesn't.


Line 992:       WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/src/kudu/client
> Because we need only client-related parts to be installed into the alternat
You don't have to worry about that, though; "make install" only installs the 
client library and headers.

If/when it installs more (especially if "more" includes "things that might 
confuse doxygen"), we can further restrict Doxygen to run in 
${DOXY_CLIENT_DESTDIR}/usr/include or some such.


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
> This change allows avoid running extra-processes to check for regex pattern
OK, I see. Let's keep them, then.

I just wanted to make sure it wasn't a change from one personal style to 
another; we discourage such changes due to the inherent "ping-ponging" effect 
they can elicit.


-- 
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

Reply via email to