search

ros-infrastructure / rosdoc_lite

A light-weight version of rosdoc that does not rely on ROS infrastructure for crawling packages.
10 stars 31 forks source link

Consider changing this to be a non-catkin independent package? #66

Open mikepurvis opened 8 years ago

mikepurvis commented 8 years ago

The only catkin-y thing this package has is a dependence on genmsg, which it uses to generate message documentation, in this one file:

https://github.com/ros-infrastructure/rosdoc_lite/blob/d6f63f94c62b3b73f5d3350a7773ffb7478b353a/src/rosdoc_lite/msgenator.py

It feels awkward to me that rosdoc_lite has to be "built" in a catkin workspace, in order to document another catkin workspace— seems like it would be cleaner for it to be a fully external affair, etc, sudo apt-get install python-rosdoc; rosdoc path/to/my/package

I don't have a ton of bandwidth to look into this further, but I'm curious to have others' thoughts on it.

jack-oquin commented 8 years ago

That makes sense to me, Mike. While I "maintain" this package in the minimal sense of merging pull requests and making new releases when needed, I don't normally do any development on it myself.

Becoming catkin-independent might be easier to do and make more sense in the context of a ROS 2 version of the tool. Would that help any? I guess you'd still want it to work with ROS 1 messages, right?

mikepurvis commented 7 years ago

@tfoote @dirk-thomas Can I get your thoughts on this? Given the awkwardness of how the doc jobs currently install rosdoc_lite, I think there would be some advantage in moving it to be a independent package.

I think there would be three possible ways forward:

Would you be in support of any of these paths?

dirk-thomas commented 7 years ago

I don't consider cloning the default branch of rosdoc_lite to be awkward. There is no real difference to use a released Python package from PyPI. It is just not necessary to run the install step since all code is usable from a cloned working copy. We have also considered to use a tagged version number before but since this repo is only rarely changed at all we didn't see a benefit in doing so.

Regarding the message generation: I don't think that rosdoc_lite should be involved in this process at all. The doc builds are imo currently badly designed. The CMake file of a package gets replaced by a simple generic one which tries to look for messages and generate them. Simply the assumption that this is a reasonable approach breaks several doc jobs (ros-infrastructure/ros_buildfarm#292). You just can't assume anything about what a package does in its CMake code. It could generate .msg files, it might not even use .msg files present in the file system, some packages even fetch additional code at configure time, etc.

In the very early design phase of the new build farm I proposed to let the doc jobs do the following instead (ros-infrastructure/jenkins_scripts#58):

My proposal was not considered to reduce the effort for transitioning from the old to the new build farm. I am still convinced that the best path forward would be to change the doc jobs to perform actual builds and then document the "result" (meaning the installed headers) as well as only the exported messages.

mikepurvis commented 7 years ago

Oh wow, so I'd seen the build workspace in isolation and install, but didn't realise the whole rewritten CMakelists thing. That's... quite a hack. Based on my own experiments, the epydoc generator is very dependent on having some kind of built workspace, since it has to be able to import the modules it is documenting.

In any case, once you're only documenting the "exported" messages, then it matters a lot less how exactly you do it and you might as well just document from the python results rather than have to re-parse the msg files.

I'm unsure if I'm on board with only documenting exported headers— it's reasonable to want to co-locate docs with the implementations in the cpp files (eg), and those doc blocks would be invisible to such a generator.

The rosdoc_lite package is not a large or complicated affair; perhaps I'm overcomplicating things by trying to work within it, and it would make more sense to just start over and build something which does exactly what I want; there are a number of constraints that it and the buildfarm are subject to which I am not.

jack-oquin commented 7 years ago

I do consider cloning the default branch of rosdoc_lite to be awkward, because it makes the build farm pick up un-released code sometimes, making it harder to maintain this package. But, that's not as bad as the other hacks Dirk mentions, and nobody seems willing to fix them, including me.

I agree with Mike's notion of building his own documentation scripts. That's generally what people do when they need some feature that does not easily fit into the current implementation.

mikepurvis commented 7 years ago

I am willing to make fixups to rosdoc_lite to make it better suited to building docs on Clearpath's infrastructure; I believe the suggested improvements would allow ros_buildfarm's doc jobs to be simplified and improved, as well as improving the story for developers generating docs locally on their laptops.

However, if the recommendation from upstream is that I implement these things on a proprietary fork, I can do so, and rosdoc_lite and ros_buildfarm will remain as they are.

dirk-thomas commented 7 years ago

Nobody is suggesting to create a fork.

I will comment with my opinion on the previous mentioned bullets:

Duplicate the msgenator functionality/file into rosdoc_lite

Copying code into another location is imo not a good idea in general. If the existing code is not suitable to be reused in should be refactored to enable that. Otherwise if the msgenator ever changes rosdoc_lite needs to be updated too.

Split out the message parsing part of genmsg into its own independent package, akin to catkin_pkg (eg, ros_msg_parser or similar), so that rosdoc_lite could depend on that without having to be a catkin package itself.

ROS distro agnostic packages are very problematic since they must continue to be supported for every ROS distribution. Even the ones which have been EOLed before since some users are still building them from source. Therefore as few code as possible is released that way. It would also make changing the code significantly more effort since it needs to maintain backward compatibility. That is the reason why currently msgenator is part of a distro specific package.

Rework rosdoc_lite to generate message docs from the built python rather than by re-parsing the .msg files directly.

Imo the documentation generated from the .msg files directly is very valuable (e.g. http://docs.ros.org/kinetic/api/geometry_msgs/html/msg/). It allows the user to easily see which version is available in a specific ROS distro and lets them navigate the recursive types. The generated Python code as well as the generated C++ code should be documented in the same way as manually written Python and C++ code. rosdoc_lite shouldn't need to implement anything special here.

I do consider cloning the default branch of rosdoc_lite to be awkward, because it makes the build farm pick up un-released code sometimes, making it harder to maintain this package.

For a repository with active development I agree that tagging versions makes a lot of sense. I just doubt the benefit in this case (at least based on the past) since there is almost no activity in this repo. Only very few bug fixes are being applied and they are usually immediately "released" / rolled out to be used. If someone wants to tag this repo with a version number it would be trivial to update the build farm to use a specific tag instead of master. If I remember correctly we discussed this before and since there was no active development nothing was changed since it basically didn't make a real difference.

If "enough" people raise their voice about the doc jobs (and their pseudo builds) it is more likely that I can bring up the problem in our team meeting again and get time allocated to work on it. Without I just can't spend any time on it due to other projects having priority.

mikepurvis commented 7 years ago

The generated Python messages contain the entire contents of the original msg file— including comments. so nothing is really lost by going that route.

Absent the issue with msgenator, are you opposed to the idea of the ros documentation tool being a pypi package rather than a distro package?

dirk-thomas commented 7 years ago

The generated Python messages contain the entire contents of the original msg file— including comments. so nothing is really lost by going that route.

The sphinx documentation of the generated Python code contains the content of the .msg files. But only withing a docblock, right? Wouldn't that be a step back in terms of documentation? Maybe I am thinking about something different. Can you please post a link to docs of the generated Python code.

I am fine with it being a pure Python package. I am just not sure how you want to address the dependencies on doxygen and genmsg.

mikepurvis commented 7 years ago

There is confusion— I am not meaning to propose that the messages would be documented by sphinx or epydoc. There would still be a bespoke generator for messages, but it would use as its source the generated python modules (eg, to discover the fields, types, dependencies, etc). This breaks the dependency on the parsing logic in genmsg.

dirk-thomas commented 7 years ago

I think I don't understand how the proposed process would look like. The generated code contains the __slots__ and __slot_types__ as well as the _full_text. In order to show a similar documentation as currently wouldn't this require to parse the full text (in order to keep showing the comments)? Also wouldn't parsing the generated Python code be more fragile than reading the message specification?

mikepurvis commented 7 years ago

Taking Imu for example, the generated doc has three parts:

The only piece missing is a reference to the exact filename in the package, though we don't really have that today either (it would be cool if it linked right back to the file on github, at the appropriate branch).

Perhaps this is marginally more fragile than using the original parser, but as has been pointed out, neither has changed in years, and although the fields are "private" in the sense that they're prefixed with underscores, in reality they're part of the stable API and used all over the place, eg:

https://github.com/ros/ros_comm/search?q=_slot_types+OR+_full_text

To further simplify, another way could be to do as catkin itself does and generate rst pages for Sphinx to process— that gets rosdoc_lite completely out of the business of slinging HTML around.

As far as the doxygen dependency, I don't have a good answer for that— I think the simplest thing would be for (the proposed pypi-based) rosdoc_lite to simply check for doxygen with a trivial invocation, and when missing inform the user that doxygen invocations are skipped until it is installed. The stdeb config that produces python-rosdoc-lite would insert the dependency.

dirk-thomas commented 7 years ago

Thanks for clarifying. The proposed process sounds reasonable to me.

(In my personal opinion using the ROS msg files instead of the generated Python code sounds better but I am ok with rosdoc_lite being updated the way you described it since it addresses your use cases.)