Check/Report package versions #356
Conversation
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
…2/ros2cli into claire/check-package-version
Signed-off-by: claireyywang <[email protected]>
claireyywang
changed the title
claireyywang
changed the title
ros2doctor/package.xml
Outdated
| @@ -9,6 +9,7 @@ | |||
|
|
|||
| <depend>ros2cli</depend> | |||
|
|
|||
| <exec_depend>ament_index</exec_depend> | |||
There was a problem hiding this comment.
There is no package with that name. I guess you need ament_index_python.
ros2doctor/ros2doctor/api/package.py
Outdated
| return | ||
| xml_file = os.path.join(prefix, 'share', package_name, 'package.xml') | ||
| try: | ||
| root = ET.parse(xml_file).getroot() |
There was a problem hiding this comment.
Instead of parsing the manifest file yourself please use the API provided by the Python package catkin_pkg (rosdep key python3-catkin-pkg-modules) instead.
| if distro_ver[:3] != local_ver[:3]: # 0.8.0 vs. 0.8.0-1 | ||
| result.add_warning('`%s` has different local version %s from \ | ||
| required version %s' % (package_name, distro_ver, local_ver)) | ||
| return result |
There was a problem hiding this comment.
Instead of getting all packages and versions from rosdistro and then checking if they exist on the system I would suggest the opposite: collect all packages found and their versions and then compare them to the information from rosdistro.
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
|
|
||
| :return: a dictionary contains package name, release, source and status | ||
| """ | ||
| distro_name = os.environ.get('ROS_DISTRO') |
There was a problem hiding this comment.
It looks like this will raise RuntimeError if ROS_DISTRO is unset. What should PackageCheck and PackageReport do in that case?
>>> import rosdistro
>>> url = rosdistro.get_index_url()
>>> i = rosdistro.get_index(url)
>>> data = rosdistro.get_distribution(i, None).get_data()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/usr/lib/python3/dist-packages/rosdistro/__init__.py", line 114, in get_distribution
dist_file = get_distribution_file(index, dist_name)
File "/usr/lib/python3/dist-packages/rosdistro/__init__.py", line 119, in get_distribution_file
data = _get_dist_file_data(index, dist_name, 'distribution')
File "/usr/lib/python3/dist-packages/rosdistro/__init__.py", line 186, in _get_dist_file_data
raise RuntimeError("Unknown release: '{0}'. Valid release names are: {1}".format(dist_name, ', '.join(sorted(index.distributions.keys()))))
RuntimeError: Unknown release: 'None'. Valid release names are: ardent, bouncy, crystal, dashing, eloquent, groovy, hydro, indigo, jade, kinetic, lunar, melodic, noeticThere was a problem hiding this comment.
I'll add RuntimeError to the except block in PackageCheck and PackageReport. In check, it will pass error message to result; in report, it will output an empty report with report name only.
ros2doctor/ros2doctor/api/package.py
Outdated
| """Check packages within the directory where command is called.""" | ||
| result = Result() | ||
| try: | ||
| distro_packages_info = get_ros_packages_info() |
There was a problem hiding this comment.
Recommend distro_packages_info -> distro_repositories_info and get_ros_packages_info -> get_ros_repositories_info since it's a dictionary of repositories, which may have 1 or more packages in them.
ros2doctor/ros2doctor/api/package.py
Outdated
| return result | ||
| for package_name, package_prefix in local_packages_prefixes.items(): | ||
| file_path = os.path.join(package_prefix, 'share', package_name) | ||
| if package_name in distro_packages_info: |
There was a problem hiding this comment.
It looks like this won't work for repositories that have more than one package. For example, rcl_action is in the repository rcl.
>>> distro_packages_info['rcl']
{'doc': {'type': 'git', 'url': 'https://github.com/ros2/rcl.git', 'version': 'dashing'}, 'release': {'url': 'https://github.com/ros2-gbp/rcl-release.git', 'version': '0.7.7-1', 'tags': {'release': 'release/dashing/{package}/{version}'}, 'packages': ['rcl', 'rcl_action', 'rcl_lifecycle', 'rcl_yaml_param_parser']}, 'source': {'type': 'git', 'url': 'https://github.com/ros2/rcl.git', 'version': 'dashing', 'test_pull_requests': True}, 'status': 'developed'}Note the rcl repo has a key packages which is a list of packages in the repo, which includes rcl_action.
There is no repo rcl_action.
>>> distro_packages_info['rcl_action']
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
KeyError: 'rcl_action'
ros2doctor/ros2doctor/api/package.py
Outdated
| result.add_warning('Unable to parse `%s` package.xml file.' % package_name) | ||
| local_ver = '' | ||
| if required_ver and local_ver: | ||
| if required_ver[:3] != package_obj.version[:3]: |
There was a problem hiding this comment.
Recommend warning only if the released version is greater than the installed version, meaning the user needs to upgrade their packages. If the installed version is greater, the person calling ros2 doctor may be intentionally building a newer version from source.
ros2doctor/ros2doctor/api/package.py
Outdated
| if distro_packages_info and local_package_prefixes: | ||
| for package_name, package_prefix in local_package_prefixes.items(): | ||
| try: | ||
| package_info = distro_packages_info.get(package_name) |
There was a problem hiding this comment.
Same comment about distro_packages_info being repository info, and this not working for repos with multiple packages.
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
claireyywang
force-pushed
the
claire/check-package-version
branch
from
5ae1456 to
e20baaf
Compare
This reverts commit e20baaf.
Signed-off-by: claireyywang <[email protected]>
This reverts commit 28f284c.
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: claireyywang <[email protected]>
|
messed up local, gonna make a new branch and PR |
close #343
Check locally installed package versions against ones on rosdistro. Local versions are obtained by parsing
package.xmlfound in install space.