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

Make ros2doctor depend on ros_environment and fix platform.py bug on error. #538

Merged
merged 2 commits into from
Jun 23, 2020

Conversation

clalancette
Copy link
Contributor

@clalancette clalancette commented Jun 22, 2020

The ros2doctor package check requires ROS_DISTRO to be defined in the environment. If it is not, you get warnings like:

UserWarning: ERROR: ROS_DISTRO is not set

Since that environment variable comes from the ros_environment package, that means this package has an exec_depend on ros_environment. Add that here.

While we are in here, also notice that platform.py returns a "None" value if _check_platform_helper fails. However, it's contract states:

:return: Report object storing report content.

Thus, make sure to return an empty report on error, which fixes possible crashes and also conforms to what other reports do.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

Since it technically needs ROS_DISTRO defined for it to work,
that means it also has an exec_depend on ros2doctor.  Put that
in here.  Also make the code a little more robust to tests
that may fail and return None.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@nuclearsandwich
Copy link
Member

Since it technically needs ROS_DISTRO defined for it to work,

You mentioned it in an offline discussion but since the diff doesn't include any mention of the ROS_DISTRO variable a description of the failure mode without the ros_environment package (which is the canonical provider of the ROS_DISTRO environment variable) would be welcome in the PR description. To answer another offline question, there is no reason not to add a dependency on ros_environment and we've been recommending a build dependency on it for anyone whose build process wants to check the ROS_DISTRO, ROS_VERSION, or ROS_PYTHON_VERSION.

@clalancette
Copy link
Contributor Author

You mentioned it in an offline discussion but since the diff doesn't include any mention of the ROS_DISTRO variable a description of the failure mode without the ros_environment package (which is the canonical provider of the ROS_DISTRO environment variable) would be welcome in the PR description.

Good point, will add that.

To answer another offline question, there is no reason not to add a dependency on ros_environment and we've been recommending a build dependency on it for anyone whose build process wants to check the ROS_DISTRO, ROS_VERSION, or ROS_PYTHON_VERSION.

👍

That is, it should return the empty ros_report, not "None".

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@dirk-thomas
Copy link
Member

(Maybe update the title to mention the additional change happening in the patch.)

@clalancette clalancette changed the title Make ros2doctor depend on ros_environment. Make ros2doctor depend on ros_environment and fix platform.py bug on error. Jun 23, 2020
Copy link
Contributor

@claireyywang claireyywang left a comment

Choose a reason for hiding this comment

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

LGTM with green CI!

@clalancette
Copy link
Contributor Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor Author

@ros-pull-request-builder retest this please

@clalancette
Copy link
Contributor Author

PR tests are flakey, so going ahead and merging this one.

@clalancette clalancette merged commit ec1ce86 into master Jun 23, 2020
@delete-merged-branch delete-merged-branch bot deleted the doctor-fix-deps branch June 23, 2020 21:02
craigh92 pushed a commit to craigh92/ros2cli that referenced this pull request Jul 17, 2020
…error. (ros2#538)

* Make ros2doctor depend on ros_environment.

Since it technically needs ROS_DISTRO defined for it to work,
that means it also has an exec_depend on ros2doctor.  Put that
in here.  Also make sure RosDistroReport returns a valid report on error.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Craig Hickman <craig.hickman@ukaea.uk>
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.

4 participants