Filter ros2 distros out when testing ros1 rosdep data. #652
Conversation
| @@ -52,7 +52,9 @@ def test_url_constants(): | |||
| def test_get_gbprepo_as_rosdep_data(): | |||
| from rosdep2.rosdistrohelper import get_index | |||
| from rosdep2.gbpdistro_support import get_gbprepo_as_rosdep_data | |||
| distro = sorted(get_index().distributions.keys())[0] | |||
| distro = sorted( | |||
| d for d, info in get_index().distributions.items() | |||
There was a problem hiding this comment.
Instead of d I would suggest using name as the variable name.
| distro = sorted(get_index().distributions.keys())[0] | ||
| distro = sorted( | ||
| d for d, info in get_index().distributions.items() | ||
| if info['distribution_type'] == 'ros1')[0] |
There was a problem hiding this comment.
Since info might not contain distribution_type this should use info.get('distribution_type') instead.
There was a problem hiding this comment.
This change would generate an empty list causing the [0] to index error immediately after. Is your goal to allow this to work without a specified distribution_type or to fail explicitly rather than with arbitrary errors?
There was a problem hiding this comment.
info['distribution_type'] might fail with a key error where my proposal would not.
If there is not a single ROS 1 distribution the result is the same as before where it tries to access the first item on an empty list.
There was a problem hiding this comment.
info['distribution_type'] might fail with a key error where my proposal would not.
I don't see any significant difference between a KeyError and an IndexError in this case but it's an easy request to satisfy.
There was a problem hiding this comment.
Both happen in two different places. 60dabda addresses the KeyError introduced by this patch. The IndexError was already present in the code before this patch and is still afterwards.
Codecov Report
@@ Coverage Diff @@
## master #652 +/- ##
=========================================
Coverage ? 75.17%
=========================================
Files ? 30
Lines ? 2888
Branches ? 0
=========================================
Hits ? 2171
Misses ? 717
Partials ? 0Continue to review full report at Codecov.
|
Followup to #652 I forgot that this was specified both here and in the setup.py.
Followup to #652 I forgot that this was specified both here and in the setup.py.
|
This change seems to be the reason for the following reported regression: https://discourse.ros.org/t/rosdep-and-eol-distros/7640/2 |
|
See #657 for the follow up. |
|
I believe the issue should be resolved now that the packages have been yanked from the older repositories. I've announced the change on discourse https://discourse.ros.org/t/yanked-releases-of-bloom-and-rosdep-on-pre-trusty-platforms/7661 |
This should get CI green again. Since we need to use a property only available with index v4 we also need to require the version of rosdistro which uses it by default.