Alexey Serbin has posted comments on this change.

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


Patch Set 8:

(6 comments)

Thank you for review!

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_
The find_package(Doxygen) outputs information on location of doxygen binary 
found along with its version.  Would be it enough?  Or it's better to output 
doxygen binary location every time the 'doxygen' target is built?


Line 989:     add_custom_target(doxy_install_client_alt_destdir
> What if doxy_install_client_alt_destdir depended on 'all'? Then could you d
The 'all' and 'install' steps are here is for kudu-client sub-dir, actually.  
But yes, it might depend on 'all' target for the top-level (however, it could 
fire back if 'all' had 'docs' as the dependency :)).  Actually, I was down that 
path already :)  However, when I tried to introduce such a dependency, cmake 
failed stating that 'all' target did not exist.

If you know how to deal with that issue, please let me know -- I will gladly 
update this part.


Line 990:       COMMAND ${CMAKE_COMMAND} -E remove_directory 
${DOXY_CLIENT_DESTDIR}
> Why do we need to remove the old directory first?
This is for cleaning some stale stuff if it's left around when make was 
interrupted in one of prior runs.  Since this is a phony target, no 
auto-removal is possible on interruption.  Does it make sense?


Line 992:       WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/src/kudu/client
> Why does the working directory need to be modified for this command?
Because we need only client-related parts to be installed into the alternative 
destdir, nothing else: we are not interested in populating the directory 
structure with something which is not relevant to the client API.


Line 997:       COMMAND ${CMAKE_COMMAND} -E remove_directory 
${DOXY_CLIENT_DESTDIR}
> Why this?
This is a clean-up of the temporary stuff that was created.  The original idea 
was to use mktemp to create a temporary dir and then get rid of it, but it went 
into too complex cmake function and I abandoned it.  So, it's just some remnant.

We can leave with having that around, that's OK.  Actually, after some 
consideration I see it could be useful, especially if the installed files were 
different from those we have in the source tree -- it would allow to refer to 
the actual data if resolving warning/errors coming from doxygen.

I'll change 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
This change allows avoid running extra-processes to check for regex patterns.

Yes, they can be reverted.  I just though not running extra-processes was a 
good change.  Do you insist on reverting these?


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