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

Doxygen builder: added support of PREDEFINED tag #46

Closed fspindle closed 10 years ago

fspindle commented 10 years ago

Follows discussion http://answers.ros.org/question/154676/how-to-introduce-the-doxygen-predefined-tag-in-rosdoc_lite/?answer=154841#post-id-154841

Fabien

jack-oquin commented 10 years ago

@fspindle: Thanks for providing this enhancement. The PREDEFINED tag (b8b71fc) looks good. I presume it works with your program?

While I personally favor uninstall support, I am reluctant to add 06797f8 because the catkin developers seem opposed to the idea, and I have no confidence that it interacts well with catkin and catkin workspaces.

@dirk-thomas, @wjwwood: do you have comments about that commit?

fspindle commented 10 years ago

@jack-oquin Yes it works with my catkin package.

Shall I do a new PR without cmake changes (only PREDEFINED) ?

jack-oquin commented 10 years ago

That would be cleaner. The other patch is really a separate issue.

I like uninstall, but feel I should wait to hear from Dirk and William before accepting a patch like that into a low-level ROS repository. If it were my own code, I would gladly try it.

I am just filling in as a maintainer of rosdoc_lite. I didn't write it and don't know it very well. But, I do know how to release ROS packages and fix simple problems.

fspindle commented 10 years ago

Here is the PR with changes concerning only PREDEFINED https://github.com/ros-infrastructure/rosdoc_lite/pull/48

jack-oquin commented 10 years ago

Merged #48, closing this one.

dirk-thomas commented 10 years ago

For the rosdoc_lite package itself the uninstall might not make much sense.

On a global scale having the option for all catkin packages would indeed be useful. If someone would like to work on that, contribute code and make it a solid feature there is no reason not to include such a feature. But I haven't seen a "solution" for this which really worked well and therefore the feature does not exist in catkin yet.

wjwwood commented 10 years ago

There is nothing wrong with having an uninstall option, but in my experience they tend to rely on having an install manifest installed. CMake generates one, but it lives in the build space, so after the build space is removed uninstall no longer works.

I've done uninstall targets before:

https://github.com/segwayrmp/libsegwayrmp/blob/master/cmake/segwayrmp_targets.cmake#L54-68

But that is unix specific and only works as long as the build space is available. I don't oppose having an uninstall target in general, I just never saw a suggestion of how to do it robustly for all catkin packages.

jack-oquin commented 10 years ago

I agree that it should only be done correctly if at all.