Alexey Serbin has posted comments on this change.

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


Patch Set 8:

(4 comments)

Please see the responses in-line.  Once the questions are cleared, I'll post 
the next version.

Thanks!

http://gerrit.cloudera.org:8080/#/c/3619/8/CMakeLists.txt
File CMakeLists.txt:

Line 989:     add_custom_target(doxy_install_client_alt_destdir
> Maybe there's no real 'all' target after all. What about depending on 'kudu
Yesterday I tried dependency on 'kudu_client_exported' as well :)  The problem 
is that 'kudu_client_exported' does not have proper dependencies and if 
depending on this target, sometimes the following error appeared:

/Users/aserbin/Projects/kudu/src/kudu/util/version_info.cc:22:10: fatal error: 
      'kudu/generated/version_defines.h' file not found

Instead of fixing that 'kudu_client_exported' problem (I thought it might be a 
feature) I decided to go with calling the 'all' target explicitly since I'm 
calling the 'install' target anyways.

Please let me know if you think it's still worth continuing exploring this path 
and fixing those issues down the road.


Line 990:       COMMAND ${CMAKE_COMMAND} -E remove_directory 
${DOXY_CLIENT_DESTDIR}
> Hmm, OK. I was hoping that a second "make install" would automatically clea
'make install' does not clean old contents in the target DESTDIR.  At least as 
I checked it, if some extra file from previous 'make install' is left, the new 
'make install' leaves the extra file in place, overwriting only files which are 
common in old and new runs.


Line 992:       WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/src/kudu/client
> You don't have to worry about that, though; "make install" only installs th
Yes, but why to depend on that at all and spend extra-time fixing that if 
something changes in future in the top-level 'install' target?  Why not to use 
the client-specific target if it already exists and does exactly what is 
needed?  I think possibility of affecting changes in the top-level 'install' 
target's behavior is higher compared with possibility of changes in the 
client-specific target. 

I'm hesitant to change that due to the reasons above, but I might miss 
something.  Please let me know if you still think it's worth changing to run 
'make install' at the top-level.


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
> OK, I see. Let's keep them, then.
All right, I see.  But if there is additional vote from another reviewer to 
keep the original, I'll revert these changes.


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