rosbuild/Reviews/2010-01-12 Doc Review
Reviewer:
Instructions for doing a doc review
See DocReviewProcess for more instructions
- Does the documentation define the Users of your Package, i.e. for the expected usages of your Stack, which APIs will users engage with?
- Are all of these APIs documented?
- Do relevant usages have associated tutorials? (you can ignore this if a Stack-level tutorial covers the relevant usage), and are the indexed in the right places?
- If there are hardware dependencies of the Package, are these documented?
- Is it clear to an outside user what the roadmap is for the Package?
- Is it clear to an outside user what the stability is for the Package?
- Are concepts introduced by the Package well illustrated?
- Is the research related to the Package referenced properly? i.e. can users easily get to relevant papers?
- Are any mathematical formulas in the Package not covered by papers properly documented?
For each launch file in a Package
- Is it clear how to run that launch file?
- Does the launch file start up with no errors when run correctly?
- Do the Nodes in that launch file correctly use ROS_ERROR/ROS_WARN/ROS_INFO logging levels?
Concerns / issues
Vijay
rosbuild mainpage
manifest typo: rosbuild contains scripts for managing the CMake-based build system for ROS
- Fixed on trunk.
Customizing the build - I'm not sure if pasting all of rosconfig.cmake in a verbatim block provides much value. Users are not going to be modifying this file, and you've also told them where to find it.
Replaced the paste-in with a bulleted list of the variables that are set in rosconfig.cmake, along with valid and default values.
Minor Edits http://wiki/rosbuild?action=diff&rev2=26&rev1=25
- Thanks.
CMakeLists
When defining function names, the style varies between Bold() and BoldMonospace(). Probably should be consistent
- A bit of search and replace should have fixed it; can't promise 100%.
I'd suggest adding a line at the top of Test Macros, explaining more about testing, and also link to unittest
- Done.
Section rosbuild_add_gtest_labeled is a little strange, since there's no command of that name. Maybe it should be called something like rosbuild_add_gtest (labeled)
- Hmm, there is a macro by that name, and section 2.4.2 (rosbuild_add_gtest_labeled) gives example usage and documentation.
[Vijay] I think I was looking at 2.4.9 when I wrote the original comment. Refer to Section 2.4.9. I think the macro name in the verbatim block should be rosbuild_add_rostest_labeled.
rosbuild_genmsg/gensrv: Not sure where you're trying to link genmsg and gensrv to. Do we need to add a page documenting these scripts?
- Fixed.
Minor Edits http://wiki/rosbuild/CMakeLists?action=diff&rev2=128&rev1=126
- Thanks.
Jeremy
- Somewhere on the rosbuild main page it seems like it would be useful to have an overview explanation of the typical problem rosbuild solves. That is, getting your package to include the correct include/link flags for all of its dependencies, getting it to only rebuild when dependencies rebuild, etc.
Fixed.
On the CMakeLists page, it is hard to find the detailed documentation and the TOC is just out of control. It took me a while to figure out that the detailed documentation even existed. At the very least, it needs a much more clear link somewhere. I would also consider breaking the CMakeLists page into 2 pages: CommonTasks, and RosbuildAPI.
- Moved examples from CMakeLists to CMakeLists/Examples, and changed "Detailed documentation" to "API documentation".
In section "Checking for a function", would be good to mention that things like CheckFunctionExists.cmake are provided by cmake in /usr/share/cmake-2.6/Modules/. Same for CheckIncludeFiles, etc. Providing appropriate links to the real cmake documentation on these includes would be great. CheckIncludeFile for example.
- Added a paragraph at the top of CMakeLists/Examples explaining the use of standard CMake modules. I didn't deep link to the docs for any particular module, because that's apparently supported only for >=2.6, and I want to keep people looking at the 2.4.6 docs.
- Maybe cover adding compile/link flags before checking system capabilities, since the former is probably the more common use case.
- Done.
- In linking against an exernal library, it's unclear why wavefront_standalone is being linked against. This feels like something that would normally be coming from another ROS package. This example would probably be better served by using some library from a more common system dependency.
- Suggested different ordering for section on linking:
- Reordered as requested.
- Linking against libraries built by other ROS packages
- Linking against an external library
- Linking against a library in your package
- Using pkg-conf to find an external configuration
- Compiling and linking with profiling
- Removing default compile/link flags
- A rosbuild_add_gtest reference in the Examples section would be helpful.
A more in-depth discussion of what is happening with rosbuild_add_boost_directories / rosbuild_link_boost would be helpful. When should/shouldn't this be used. Does it need to be used if you are using the same boost functionality already used by roscpp? Appropriate documentation for using these (or at least a link) probably belongs on the boost page.
- Added an example: rosbuild/CMakeLists/Examples#Using_Boost, and a bit more text on the API page: rosbuild/CMakeLists#rosbuild_add_boost_directories, rosbuild/CMakeLists#rosbuild_link_boost,
- I don't understand why rosbuild_add_gtest_labeled declares the test if ROS_BUILD_TEST_LABEL is unset. What is the value of this?
- Because the labeling mechanism is really being used to exclude certain tests when somebody (e.g., Hudson) has set a particular label. If you don't set anything (which is what most users do), then all tests run, which is what we want.
- Should we move Deprecated documentation (rosbuild_add_rostest_graphical) to the end of the list?
- Removed it, because now all deprecated macros have been removed from the code.
rosbuild_add_generated_msgs doesn't seem useful without a description of how one might go about using rosbuild to generate messages automatically. This should at least point to a package that might be used as a reference.
- Said that it's an advanced topic and pointed to actionlib.