[kudu-CR] client.h: doxygen comments for C++ API
Alexey Serbin has posted comments on this change.
Change subject: client.h: doxygen comments for C++ API
..
Patch Set 6:
(5 comments)
Thanks for review! Will post update soon.
http://gerrit.cloudera.org:8080/#/c/3619/6/CMakeLists.txt
File CMakeLists.txt:
Line 1000: # "make doxydocs" target
> Sorry to be a buzz kill, but can we just make this target "doxygen" instead
Sure, why not. Will do.
Line 1015: COMMAND make all install DESTDIR=${DOXY_CLIENT_DESTDIR}
> Can this be made to work with Ninja was well? I think you just need to repl
That's what I wondering as well, but I could not find it. Thanks -- will
replace with CMAKE_MAKE_PROGRAM.
http://gerrit.cloudera.org:8080/#/c/3619/6/docs/.gitignore
File docs/.gitignore:
Line 19
> nit: spurious change
Done
http://gerrit.cloudera.org:8080/#/c/3619/6/src/kudu/client/client.h
File src/kudu/client/client.h:
Line 28: #include
> This change isn't required for this patch, is it?
You are right -- it's not required. Will rollback.
http://gerrit.cloudera.org:8080/#/c/3619/6/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
Line 262: if ! `which -s doxygen` && [ ! -d $DOXYGEN_DIR ]; then
> The -s command-line flag is not available on GNU systems. You can do:
This is going away, actually. I'll use the same approach as for the rest of
the tools because of uniformity reasons.
--
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: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dinesh Bhat
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-HasComments: Yes
[kudu-CR] client.h: doxygen comments for C++ API
Mike Percy has posted comments on this change.
Change subject: client.h: doxygen comments for C++ API
..
Patch Set 6:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/3619/6/CMakeLists.txt
File CMakeLists.txt:
Line 1000: # "make doxydocs" target
Sorry to be a buzz kill, but can we just make this target "doxygen" instead of
"doxydocs"?
Line 1015: COMMAND make all install DESTDIR=${DOXY_CLIENT_DESTDIR}
Can this be made to work with Ninja was well? I think you just need to replace
make with ${CMAKE_MAKE_PROGRAM}
http://gerrit.cloudera.org:8080/#/c/3619/6/docs/.gitignore
File docs/.gitignore:
Line 19
nit: spurious change
http://gerrit.cloudera.org:8080/#/c/3619/6/src/kudu/client/client.h
File src/kudu/client/client.h:
Line 28: #include
This change isn't required for this patch, is it?
http://gerrit.cloudera.org:8080/#/c/3619/6/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
Line 262: if ! `which -s doxygen` && [ ! -d $DOXYGEN_DIR ]; then
The -s command-line flag is not available on GNU systems. You can do:
if ! which doxygen >/dev/null && [ ! -d "$DOXYGEN_DIR" ]; then
--
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: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dinesh Bhat
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-HasComments: Yes
[kudu-CR] client.h: doxygen comments for C++ API
Kudu Jenkins has posted comments on this change. Change subject: client.h: doxygen comments for C++ API .. Patch Set 6: Build Started http://104.196.14.100/job/kudu-gerrit/2500/ -- 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: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: No
[kudu-CR] client.h: doxygen comments for C++ API
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3619 to look at the new patch set (#6). Change subject: client.h: doxygen comments for C++ API .. client.h: doxygen comments for C++ API To generate the Doxygen docs from client.h, run 'make doxydocs'. Open $REPO_ROOT/docs/doxygen/html/index.html in your favorite browser to see the generated documentation. The 'docs' target generates doxygen documentaion as well since it depends on the 'doxydocs' target. Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02 --- M CMakeLists.txt M docs/.gitignore A docs/support/doxygen/client_api.doxy.in M src/kudu/client/client.h M thirdparty/build-definitions.sh M thirdparty/build-thirdparty.sh M thirdparty/download-thirdparty.sh M thirdparty/vars.sh 8 files changed, 1,314 insertions(+), 718 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/3619/6 -- To view, visit http://gerrit.cloudera.org:8080/3619 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] client.h: doxygen comments for C++ API
Alexey Serbin has posted comments on this change. Change subject: client.h: doxygen comments for C++ API .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/3619/5/CMakeLists.txt File CMakeLists.txt: Line 963: find_package(Doxygen REQUIRED) > Sure, if doxygen is in the thirdparty tree then that's fine. Out of curiosi On my MacBookPro if running 'make -j8': real0m16.457s user1m8.855s sys 0m6.126s Plus a 8 seconds to generate GNU makefiles out of cmakefiles. -- 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: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] client.h: doxygen comments for C++ API
Adar Dembo has posted comments on this change. Change subject: client.h: doxygen comments for C++ API .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/3619/5/CMakeLists.txt File CMakeLists.txt: Line 963: find_package(Doxygen REQUIRED) > Yes, you are right -- cmake will require doxygen to be present. And yes, i Sure, if doxygen is in the thirdparty tree then that's fine. Out of curiosity, how long does it take to build? -- 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: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Anonymous Coward #206 Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] client.h: doxygen comments for C++ API
Alexey Serbin has posted comments on this change.
Change subject: client.h: doxygen comments for C++ API
..
Patch Set 5:
(9 comments)
Thank you for the review!
I took a look at vera++ as a candidate for syntax rule/style enforcement. It
will require to write a couple of scripts in TCL for that, but I haven't done
that part yet. However, I used vera++ for other projects and it worked fine
for me.
I haven't looked for other stylecheckers besides vera++.
As I understood, syntax/style checker is only the thing we want to have, since
we don't want to enforce documentation of every parameter and return type or
documenting every public member of public C++ interface.
Here the idea was to roll out current file, pushing auto-generated docs for C++
API to the kudu.apache.org site pretty soon. The style checkers could be added
in background (but yes, it's important to have them in the long run).
http://gerrit.cloudera.org:8080/#/c/3619/5/CMakeLists.txt
File CMakeLists.txt:
Line 963: find_package(Doxygen REQUIRED)
> Doesn't this mean that any invocation of cmake will require doxygen? That s
Yes, you are right -- cmake will require doxygen to be present. And yes, it
might be too harsh if requiring it as installed by default. However, my idea
is to build the doxygen as a third-party tool (like, say curl) and I have that
change locally but I haven't posted it yet. I'm awaiting access to be granted
to the Cloudfront repo to upload doxygen source tarball. Sorry for not posting
it yet.
Probably, it's also possible to search for doxygen and build it only upon
building the 'doxydocs' target. I'll take a look on that.
Line 966: WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/docs)
> Shouldn't this be CMAKE_CURRENT_BINARY_DIR? Otherwise we're polluting the s
Yep, that's a typo. Thanks -- will fix.
Line 977: DEPENDS doxydocs)
> Does this work? The cmake docs say that DEPENDS should reference files and
Yes, it works. In the result GNU makefile the docs rule runs doxydoc-related
stuff by dependency. But I'll re-do it anyways. Thanks for pointing at that.
http://gerrit.cloudera.org:8080/#/c/3619/5/cmake_modules/FindDoxygen.cmake
File cmake_modules/FindDoxygen.cmake:
> FindDoxygen.cmake is already in the cmake we distribute as part of thirdpar
Frankly, I didn't think about this -- I just followed the same scheme as for
GTest, ZLib and others. Ok, I will take a look to clarify on this.
http://gerrit.cloudera.org:8080/#/c/3619/5/docs/support/doxygen/client.doxy
File docs/support/doxygen/client.doxy:
Line 21: # Doxyfile 1.8.10
> What's the significance of the version? If there is some significance, perh
Originally, the file was generated as a template by doxygen, and it inserted
this version here. I was thinking about using this as a sort of hint for up to
which version of doxygen this file is valid and which parameter to be expected
as default/non-default, but it appears to be a bad idea after some
consideration.
Will remove this.
Line 26:
> These are all the parameters with non-default values, right? Could you add
Right, for that particular version of doxygen they are non-default. Yes, I'll
add those comments -- it makes sense.
Line 29: INPUT = ../src/kudu/client/client.h
> The client API surface is more than just this one file. It made sense to st
Yep, that makes sense -- it's good idea. Using mktemp, create a directory,
install those files there, copy the doxyfile into that dir and then run doxygen
from that dir. Excellent, will use that approach.
http://gerrit.cloudera.org:8080/#/c/3619/5/src/kudu/client/client.h
File src/kudu/client/client.h:
Line 124: /// Signal number to use for internal
> Gerrit has flagged the trailing whitespace here, and below in the file. Cou
Yep, will do. I forgot to update my .vimrc accordingly before editing this
file -- it should have shown cspace errors.
PS5, Line 197: is
> "returned." is omitted here.
Thanks! Will update.
--
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: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Anonymous Coward #206
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-HasComments: Yes
[kudu-CR] client.h: doxygen comments for C++ API
Anonymous Coward #206 has posted comments on this change. Change subject: client.h: doxygen comments for C++ API .. Patch Set 5: (1 comment) Hi Alexey, This is not a review, only testing my review subscription via this. Also pointing a typo I noticed in client.h. ~Dinesh. http://gerrit.cloudera.org:8080/#/c/3619/5/src/kudu/client/client.h File src/kudu/client/client.h: PS5, Line 197: is "returned." is omitted here. -- 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: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Anonymous Coward #206 Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] client.h: doxygen comments for C++ API
Adar Dembo has posted comments on this change.
Change subject: client.h: doxygen comments for C++ API
..
Patch Set 5:
(9 comments)
I didn't review the changes in client.h in detail yet, wanted to figure out the
infrastructure first.
Did you look into lint-style checkers that will warn (and, more importantly,
fail a precommit build) if the doxygen syntax is malformed in the whitelisted
files where it's expected? I think such a checker is important; without it, the
syntax will gradually bitrot as people make mistakes, or code reviewers will
have to look for syntactic mistakes.
http://gerrit.cloudera.org:8080/#/c/3619/5/CMakeLists.txt
File CMakeLists.txt:
Line 963: find_package(Doxygen REQUIRED)
Doesn't this mean that any invocation of cmake will require doxygen? That seems
unnecessarily harsh to developers who don't want to build the docs.
Line 966: WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/docs)
Shouldn't this be CMAKE_CURRENT_BINARY_DIR? Otherwise we're polluting the
source directory with "build output".
Line 977: DEPENDS doxydocs)
Does this work? The cmake docs say that DEPENDS should reference files and
outputs of add_custom_command(), which suggests that the right way to enforce
dependencies is via add_dependencies().
http://gerrit.cloudera.org:8080/#/c/3619/5/cmake_modules/FindDoxygen.cmake
File cmake_modules/FindDoxygen.cmake:
FindDoxygen.cmake is already in the cmake we distribute as part of thirdparty.
Why duplicate it here?
http://gerrit.cloudera.org:8080/#/c/3619/5/docs/.gitignore
File docs/.gitignore:
Line 19: /doxygen/
This file should no longer be necessary; we shouldn't be putting any doc output
in the source directory. It all goes into the build directory.
http://gerrit.cloudera.org:8080/#/c/3619/5/docs/support/doxygen/client.doxy
File docs/support/doxygen/client.doxy:
Line 21: # Doxyfile 1.8.10
What's the significance of the version? If there is some significance, perhaps
explain in a comment? Otherwise, remove this comment?
Line 26:
These are all the parameters with non-default values, right? Could you add a
comment to each justifying the non-default value choice?
Line 29: INPUT = ../src/kudu/client/client.h
The client API surface is more than just this one file. It made sense to start
with client.h to prove the concept, but having done that, I think we should
roll this out for the entire public-facing client API.
The file set is defined in src/kudu/client/CMakeLists.txt, in the 'install'
target. Ideally we wouldn't have to duplicate the list of files here; I was
thinking the doxygen target could run 'make install' to some temporary
directory, then invoke doxygen recursively on all of the headers within.
http://gerrit.cloudera.org:8080/#/c/3619/5/src/kudu/client/client.h
File src/kudu/client/client.h:
Line 124: /// Signal number to use for internal
Gerrit has flagged the trailing whitespace here, and below in the file. Could
you trim?
--
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: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-HasComments: Yes
[kudu-CR] client.h: doxygen comments for C++ API
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3619 to look at the new patch set (#5). Change subject: client.h: doxygen comments for C++ API .. client.h: doxygen comments for C++ API To generate the Doxygen docs from client.h, run 'make doxydocs'. Open $REPO_ROOT/docs/doxygen/html/index.html in your favorite browser to see the generated documentation. The 'docs' target generates doxygen documentaion as well since it depends on the 'doxydocs' target. Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02 --- M CMakeLists.txt A cmake_modules/FindDoxygen.cmake M docs/.gitignore A docs/support/doxygen/client.doxy M src/kudu/client/client.h 5 files changed, 1,410 insertions(+), 717 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/3619/5 -- To view, visit http://gerrit.cloudera.org:8080/3619 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] client.h: doxygen comments for C++ API
Kudu Jenkins has posted comments on this change. Change subject: client.h: doxygen comments for C++ API .. Patch Set 5: Build Started http://104.196.14.100/job/kudu-gerrit/2331/ -- 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: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: No
[kudu-CR] client.h: doxygen comments for C++ API
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3619 to look at the new patch set (#4). Change subject: client.h: doxygen comments for C++ API .. client.h: doxygen comments for C++ API To generate the Doxygen docs from client.h, run 'make doxydocs'. Open $REPO_ROOT/docs/doxygen/html/index.html in your favorite browser to see the generated documentation. The 'docs' target generates doxygen documentaion as well since it depends on the 'doxydocs' target. Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02 --- M CMakeLists.txt A cmake_modules/FindDoxygen.cmake M docs/.gitignore A docs/support/doxygen/client.doxy M src/kudu/client/client.h 5 files changed, 1,410 insertions(+), 717 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/3619/4 -- To view, visit http://gerrit.cloudera.org:8080/3619 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] client.h: doxygen comments for C++ API
Kudu Jenkins has posted comments on this change. Change subject: client.h: doxygen comments for C++ API .. Patch Set 4: Build Started http://104.196.14.100/job/kudu-gerrit/2330/ -- 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: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: No
[kudu-CR] client.h: doxygen comments for C++ API
Kudu Jenkins has posted comments on this change. Change subject: client.h: doxygen comments for C++ API .. Patch Set 3: Build Started http://104.196.14.100/job/kudu-gerrit/2329/ -- 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: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: No
[kudu-CR] client.h: doxygen comments for C++ API
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3619 to look at the new patch set (#3). Change subject: client.h: doxygen comments for C++ API .. client.h: doxygen comments for C++ API To generate the Doxygen docs from client.h, run 'make doxydocs'. Open $REPO_ROOT/docs/doxygen/html/index.html in your favorite browser to see the generated documentation. The 'docs' target generates doxygen documentaion as well since it depends on the 'doxydocs' target. Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02 --- M CMakeLists.txt A cmake_modules/FindDoxygen.cmake M docs/.gitignore A docs/support/doxygen/client.doxy M src/kudu/client/client.h 5 files changed, 1,410 insertions(+), 717 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/3619/3 -- To view, visit http://gerrit.cloudera.org:8080/3619 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] client.h: doxygen comments for C++ API
Kudu Jenkins has posted comments on this change. Change subject: client.h: doxygen comments for C++ API .. Patch Set 2: Build Started http://104.196.14.100/job/kudu-gerrit/2321/ -- 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: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: No
[kudu-CR] client.h: doxygen comments for C++ API
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3619 to look at the new patch set (#2). Change subject: client.h: doxygen comments for C++ API .. client.h: doxygen comments for C++ API To generate the Doxygen docs from client.h, run 'make doxydocs'. Open $REPO_ROOT/docs/doxygen/html/index.html in your favorite browser to see the generated documentation. The 'docs' target generates doxygen documentaion as well since it depends on the 'doxydocs' target. Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02 --- M CMakeLists.txt M docs/.gitignore A docs/support/doxygen/client.doxy M src/kudu/client/client.h 4 files changed, 1,239 insertions(+), 717 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/3619/2 -- To view, visit http://gerrit.cloudera.org:8080/3619 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] client.h: doxygen comments for C++ API
Mike Percy has posted comments on this change. Change subject: client.h: doxygen comments for C++ API .. Patch Set 1: (1 comment) > (3 comments) > > Thank you for review, Mike! > > The auto-generated docs are available at: > https://alexeyserbin.github.io/ I only looked briefly but noticed 2 things we probably want to change: 1. the Main page is blank 2. the includes are wrong. it says #include but should say #include http://gerrit.cloudera.org:8080/#/c/3619/1/docs/.gitignore File docs/.gitignore: Line 19: doxygen/** > The committed files are not covered by this pattern, actually. This patter Can we generate the doxygen files in a subdirectory of the same tree that we generate the other docs in? -- 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: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] client.h: doxygen comments for C++ API
Alexey Serbin has posted comments on this change. Change subject: client.h: doxygen comments for C++ API .. Patch Set 1: (3 comments) Thank you for review, Mike! The auto-generated docs are available at: https://alexeyserbin.github.io/ Will post the updated version soon. http://gerrit.cloudera.org:8080/#/c/3619/1/CMakeLists.txt File CMakeLists.txt: Line 972: add_custom_target(doxydocs > Can we just make this part of the "make docs" target? Make sense -- will do. http://gerrit.cloudera.org:8080/#/c/3619/1/docs/.gitignore File docs/.gitignore: Line 19: doxygen/** > I think this is too broad, in this same commit we are adding files in a pat The committed files are not covered by this pattern, actually. This pattern excludes all content below doxygen directory in the same dir as the .gitignore file. But /doxygen/ will do the job as well -- will update. http://gerrit.cloudera.org:8080/#/c/3619/1/docs/support/doxygen/client.doxy File docs/support/doxygen/client.doxy: Line 1: # Doxyfile 1.8.10 > Wow, do we really need this huge file? Can we just have a file that specifi This is the auto-generated config which doxygen outputs if running with certain options. Defaults/non-default might change from version to version, but highly unlikely. It's not needed as is -- you are right. Ok, will update to have only non-default settings here. -- 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: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] client.h: doxygen comments for C++ API
Mike Percy has posted comments on this change. Change subject: client.h: doxygen comments for C++ API .. Patch Set 1: One more thing, we need to document doxygen as a new dependency in the docs. The current docs are here: http://kudu.apache.org/docs/installation.html#_build_from_source -- 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: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: No
[kudu-CR] client.h: doxygen comments for C++ API
Mike Percy has posted comments on this change. Change subject: client.h: doxygen comments for C++ API .. Patch Set 1: (3 comments) Mind posting the generated Doxygen HTML somewhere so we can view the output? http://gerrit.cloudera.org:8080/#/c/3619/1/CMakeLists.txt File CMakeLists.txt: Line 972: add_custom_target(doxydocs Can we just make this part of the "make docs" target? http://gerrit.cloudera.org:8080/#/c/3619/1/docs/.gitignore File docs/.gitignore: Line 19: doxygen/** I think this is too broad, in this same commit we are adding files in a path that is covered by this pattern http://gerrit.cloudera.org:8080/#/c/3619/1/docs/support/doxygen/client.doxy File docs/support/doxygen/client.doxy: Line 1: # Doxyfile 1.8.10 Wow, do we really need this huge file? Can we just have a file that specifies the non-defaults? Also, what is the license for this file? I don't see a license header here. -- 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: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] client.h: doxygen comments for C++ API
Kudu Jenkins has posted comments on this change. Change subject: client.h: doxygen comments for C++ API .. Patch Set 1: Build Started http://104.196.14.100/job/kudu-gerrit/2311/ -- 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: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] client.h: doxygen comments for C++ API
Alexey Serbin has uploaded a new change for review. http://gerrit.cloudera.org:8080/3619 Change subject: client.h: doxygen comments for C++ API .. client.h: doxygen comments for C++ API To generate the Doxygen docs from client.h, run 'make doxydocs' target. Open $REPO_ROOT/docs/doxygen/html/index.html in your favorite browser to see the generated documentation. Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02 --- M CMakeLists.txt M docs/.gitignore A docs/support/doxygen/client.doxy M src/kudu/client/client.h 4 files changed, 3,581 insertions(+), 716 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/3619/1 -- To view, visit http://gerrit.cloudera.org:8080/3619 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin
