Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support non-default python layouts #123

Merged

Conversation

rkent
Copy link
Contributor

@rkent rkent commented May 24, 2024

Fixes #122

The changes made here are:

  • a failure with the python source directory generates a warning instead of aborting
  • if python source directory is not specified, then we search for it using flat layout and source layout. We prefer to find a python package with the same name as the ros package. But if not found by that name, we search for a single python package, and use that.

Tests are included for a variety of cases.

With these changes, the following packages that were not generating output before, now generate a proper output:

mavros_extras
message_filters
microstrain_inertial_rqt
nmea_navsat_driver
nodl_python
ntrip_client
python_orocos_kdl_vendor
rmf_traffic_editor_assets
rosbag2_storage_mcap
rosbag2_storage_sqlite3
rqt, plus many related packages rqt_*
system_fingerprint
webots_ros2_tiago

These packages that previously failed, now generate html but do not fully document their python:

rmf_building_map_tools: Succeeds, but has many subdirectories that are packages that it does not parse
teleop_twist_keyboard: Succeeds, but does not show python API for main file at ./ level.
urdfdom_py: Succeeds, but error in apidoc gives incomplete python API
vision_msgs_rviz_plugins: Succeeds, there is a samples/ python directory that is not parsed

One python package still fails for another reason:

camera_calibration: now has Could not import extension sphinx.ext.pngmath (exception: No module named 'sphinx.ext.pngmath'). Fails

Many of these could have been made to work previously with the correct specification in rosdoc2.yaml. But philosophically, I think we should try to succeed (with warnings) rather than just abort if the user fails to follow the conventions we expect.

@rkent rkent requested review from audrow and tfoote as code owners May 24, 2024 05:09
@rkent
Copy link
Contributor Author

rkent commented May 30, 2024

@tfoote are you going to be able to review this?

Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on paternity leave until mid July so will have high latency and minimal bandwidth available. I put some thoughts in on this, I'd like to understand a bit more about whether we should be adding complexity to support automatically discovering the non-standard layouts/naming conventions vs providing a mechanism to just set this in rosdoc2.yaml etc.

@@ -49,7 +49,7 @@
## be assumed for 'sphinx-apidoc' invocation. The user can provide the path
## (relative to the 'package.xml' file) where the Python modules defined by this
## package are located.
python_source: '{package_name}'
# python_source: '{package_name}'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the above cases not be covered by setting this non-standard location in their config files?

Implementing custom discovery logic seem fragile and like it will end up with a lot of maintenance in the longer term versus setting the non-standard location in the configuration for those handful of packages. And we can make sure that we provide a useful error/warning such that developers can know to set this setting in their config file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear, the reason for the change in the line that you quoted is because {package_name} is the default. I would expect that a sample config file shows what the default is, but does not set it unless setting it is required. It is not.

@rkent
Copy link
Contributor Author

rkent commented Jun 4, 2024

Concerning "whether we should be adding complexity to support automatically discovering the non-standard layouts/naming conventions vs providing a mechanism to just set this in rosdoc2.yaml":

I feel strongly about:

  • rosdoc2 should not crash (fail to generate any documentation) if the python source cannot be found.
  • we should support by default both flat layout (as recommended) as well as src layout. (See the whole rqt_ universe of packages, like rqt_py_common for examples of src layout that I think we should support by default without a rosdoc2.yaml)

I don't feel strongly about fixing deviations from those two standards. One example is urdfdom_py, with the source in src/urdf_parser_py (that is, a slightly different name than the actual package name.)

But generally, I don't have a lot of confidence that package authors are going to add rosdoc2.yaml entries to fix minor differences in order to make rosdoc2 happy. I would rather have things "just work", and I am not convinced this adds much complexity to rosdoc2.

But I'll do what you want. If you want me to eliminate the search for misnamed subdirectories then I will.

@rkent rkent mentioned this pull request Jul 11, 2024
@nuclearsandwich
Copy link
Contributor

  • rosdoc2 should not crash (fail to generate any documentation) if the python source cannot be found.

I agree with this first bullet. Expected error cases should be handled with error messages addressing the user rather than a raw stacktrace.

  • we should support by default both flat layout (as recommended) as well as src layout.

I have less of an opinion about this bullet but I'm not opposed to it either.

But generally, I don't have a lot of confidence that package authors are going to add rosdoc2.yaml entries to fix minor differences in order to make rosdoc2 happy. I would rather have things "just work", and I am not convinced this adds much complexity to rosdoc2.

There's a pattern that I've seen for situations like this where a program is guessing about intended behavior which is to nag / warn that the program is guessing, then do it anyway, but suggest that a configuration be set to make what the program is guessing explicit. I think it's reasonable to push packages that don't adhere to standards to declare what they want instead in order to prevent the heuristics, which may be simple today, from growning unbounded without warning people that they might break under future versions without a config.

I'll do what you want. If you want me to eliminate the search for misnamed subdirectories then I will.

I can't speak for @tfoote but splitting this PR in half, with the first adding support for the two agreed upon cases (flat and src) and providing a follow-up for others would allow me to feel comfortable reviewing and merging the former without waiting for @tfoote.

@rkent rkent force-pushed the allow-source-python-layout branch from 72ecb9e to 950982b Compare July 11, 2024 22:29
@rkent
Copy link
Contributor Author

rkent commented Jul 11, 2024

This is the 'first half' that supports src layout. Also does not crash if there is an issue.

The earlier pushes had some git merge complications, this version should be a squash that just shows the current changes.

@nuclearsandwich nuclearsandwich merged commit c37f417 into ros-infrastructure:main Jul 12, 2024
4 checks passed
@rkent rkent deleted the allow-source-python-layout branch July 12, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Packages that claim python (ament_python or ament_cmake_python) fail if python non-standard and not specified
3 participants