WIP: Fix CLI use for source-only rosdistros. #44

mikepurvis · 2018-02-17T05:52:58Z

Since we finished the process of transitioning away from using Bloom, we no longer have release stanzas in our rosdistro repo— it's exclusively tagged source repos. I noticed today that under these circumstances, rosinstall_generator was broken at the command line, which we hadn't noticed previously because our build tooling uses it by importing and calling the underlying functions directly (which among other things bypasses a lot of the wet/dry compatibility stuff).

Anyway, this fix allows the following invocations to work on a source-only rosdistro where an appropriate source cache is available. Example specifying repos with dependencies:

$ rosinstall_generator --rosdistro indigo --upstream-development --deps --repos ros_comm
- git:
    local-name: catkin
    uri: https://github.com/ros/catkin.git
    version: 0.7.8
[...]
- git:
    local-name: ros_comm
    uri: https://github.com/ros/ros_comm.git
    version: e69b098bd4d80e7c8feda1eb1a9aaf67cdac352e
- git:
    local-name: ros_comm_msgs
    uri: https://github.com/ros/ros_comm_msgs.git
    version: 1.11.2
- git:
    local-name: roscpp_core
    uri: https://github.com/ros/roscpp_core.git
    version: 0.6.7
- git:
    local-name: roslisp
    uri: https://github.com/ros/roslisp.git
    version: 1.9.21
- git:
    local-name: rospack
    uri: https://github.com/ros/rospack.git
    version: 2.2.8
- git:
    local-name: std_msgs
    uri: https://github.com/ros/std_msgs.git
    version: 0.5.11

Example specifying exact package with deps, getting tarballs:

$ rosinstall_generator --rosdistro indigo --upstream-development --deps --tar rospack
- tar:
    local-name: catkin
    uri: https://github.com/ros/catkin/archive/0.7.8.tar.gz
    version: catkin-0.7.8
- tar:
    local-name: cmake_modules
    uri: https://github.com/ros/cmake_modules/archive/0.4.1.tar.gz
    version: cmake_modules-0.4.1
- tar:
    local-name: rospack
    uri: https://github.com/ros/rospack/archive/2.2.8.tar.gz
    version: rospack-2.2.8

I've also validated that this change doesn't break the --upstream-development flag when used with a rosdistro cache that does not include the source package xml cache (such as the one at http://repositories.ros.org/).

sloretz · 2018-02-20T15:23:36Z

Hi @mikepurvis, Thanks for the PR! I won't have time to look at this for a little while.

Would you be willing to add a test for this?

dirk-thomas

Beside the minor comment this LGTM.

dirk-thomas · 2018-02-20T18:38:09Z

src/rosinstall_generator/generator.py

+            source_package_xmls = wet_distro.get_source_repo_package_xmls(repo_name)
+        if source and source_package_xmls:
+            source_package_names = source_package_xmls.keys()
+            source_package_names.remove("_ref")


Can you please add a comment above this line why it is necessary. It took quite a bit to figure it out when reading the patch.

Added, also filed some related tickets against rosdistro which would help this in the future:

ros-infrastructure/rosdistro#115

ros-infrastructure/rosdistro#116

mikepurvis · 2018-02-21T16:07:33Z

@sloretz Sure, but we should talk about what kind of testing strategy is desired or appropriate for this repo. Currently, there's a lot of code in here which handles corner cases pertaining to rosbuild packages (which I have no interest in attempting to support or test support for), and there's also a lot of dependence on network access, which would be pretty tricky to mock out in a way that retained the legitimacy of the test itself.

Basically, would you be interested in a suite which was based on having some separate mini test rosdistros (one source-only, one source/release, one release-only) consisting of tags/hashes to repos, where the suite would build the relevant caches and then exercise some standard rosinstall_generator operations against those? Such a test would be heavily network/github dependent, but I think that's just a reality of this code— and still a huge step up from the current state affairs, which is no tests at all.

dirk-thomas

I am fine merging this patch as-is (we don't plan to use the added feature and if you are willing to use it without tests I don't mind).

It would be nice if ros-infrastructure/rosdistro#116 would be implemented to avoid the hack in this patch. Not sure if you would like to do that before this is being merged or sometime after.

mikepurvis · 2018-02-21T17:15:08Z

It's probably a fairly small fix, and making it now would avoid having to do a compatibility dance to change it down the road. If you guys are okay with merging both PRs and coordinating the next release, then I'll put up one for rosdistro, and then this one will be updated to remove the hack and bump up the version dependency.

dirk-thomas · 2018-02-21T17:19:56Z

If you guys are okay with merging both PRs and coordinating the next release, then I'll put up one for rosdistro, and then this one will be updated to remove the hack and bump up the version dependency.

Absolutely. Thanks.

mikepurvis · 2018-02-22T03:54:39Z

Okay, this is updated now, but obviously this shouldn't merge until ros-infrastructure/rosdistro#117 is merged and released.

dirk-thomas · 2018-02-26T19:41:00Z

setup.py

@@ -8,7 +8,7 @@
 setup(
    name='rosinstall_generator',
    version=__version__,
-    install_requires=['argparse', 'catkin_pkg >= 0.1.28', 'rosdistro >= 0.5.0', 'rospkg', 'PyYAML', 'setuptools'],
+    install_requires=['argparse', 'catkin_pkg >= 0.1.28', 'rosdistro >= 0.6.7', 'rospkg', 'PyYAML', 'setuptools'],


The version also needs to be updated in these two lines in the stdeb.cfg file:

rosinstall_generator/stdeb.cfg

Lines 2 to 3 in ce4e6ec

Depends: python-argparse, python-catkin-pkg (>= 0.1.28), python-rosdistro (>= 0.5.0), python-rospkg, python-yaml

Depends3: python3-catkin-pkg (>= 0.1.28), python3-rosdistro (>= 0.5.0), python3-rospkg, python3-yaml

Conflict introduced by #45 is also resolved now.

dirk-thomas · 2018-02-28T17:53:32Z

setup.py

@@ -4,7 +4,7 @@
 import sys
 from setuptools import setup, find_packages

-install_requires=['catkin_pkg >= 0.1.28', 'rosdistro >= 0.5.0', 'rospkg', 'PyYAML', 'setuptools']
+install_requires=['catkin_pkg >= 0.1.28', 'rosdistro >= 0.6.7', 'rospkg', 'PyYAML', 'setuptools']


Do you want to bump the minimum version to 0.6.8 (same below)?

dirk-thomas · 2018-02-28T19:46:09Z

Thanks. 🚢

Pip doesn't do transitive dependencies so we must specify it here. This commit was needed due to ros-infrastructure/rosinstall_generator#44

mikepurvis added 2 commits February 17, 2018 00:36

Fix CLI use for source-only rosdistros.

18316d4

Proper fallback for missing source package xml cache.

df3e797

dirk-thomas reviewed Feb 20, 2018

View reviewed changes

mikepurvis mentioned this pull request Feb 21, 2018

Wrap source repo package xmls in an object ros-infrastructure/rosdistro#116

Closed

Add comments about the _ref key.

c094124

dirk-thomas approved these changes Feb 21, 2018

View reviewed changes

mikepurvis changed the title ~~Fix CLI use for source-only rosdistros.~~ WIP: Fix CLI use for source-only rosdistros. Feb 21, 2018

mikepurvis closed this Feb 21, 2018

Use new SourceRepositoryCache class.

049f27d

mikepurvis reopened this Feb 22, 2018

dirk-thomas reviewed Feb 26, 2018

View reviewed changes

mikepurvis and others added 2 commits February 27, 2018 12:06

Update stdeb dependency on python-rosdistro.

9640e3d

Merge branch 'master' into fix-source-only-rosdistro

984dc96

dirk-thomas reviewed Feb 28, 2018

View reviewed changes

rosdistro dependency to 0.6.8.

107a960

mikepurvis force-pushed the fix-source-only-rosdistro branch from c5e3af2 to 107a960 Compare February 28, 2018 19:41

dirk-thomas merged commit c786df2 into ros-infrastructure:master Feb 28, 2018

Rayman added a commit to Rayman/ros-get that referenced this pull request Mar 1, 2018

Fix rosdistro requirements that were broken

b65b513

Pip doesn't do transitive dependencies so we must specify it here. This commit was needed due to ros-infrastructure/rosinstall_generator#44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Fix CLI use for source-only rosdistros. #44

WIP: Fix CLI use for source-only rosdistros. #44

mikepurvis commented Feb 17, 2018 •

edited

Loading

sloretz commented Feb 20, 2018

dirk-thomas left a comment

dirk-thomas Feb 20, 2018

mikepurvis Feb 21, 2018

mikepurvis commented Feb 21, 2018

dirk-thomas left a comment

mikepurvis commented Feb 21, 2018

dirk-thomas commented Feb 21, 2018

mikepurvis commented Feb 22, 2018

dirk-thomas Feb 26, 2018

mikepurvis Feb 27, 2018

mikepurvis Feb 28, 2018

dirk-thomas Feb 28, 2018

mikepurvis Feb 28, 2018

dirk-thomas commented Feb 28, 2018

	Depends: python-argparse, python-catkin-pkg (>= 0.1.28), python-rosdistro (>= 0.5.0), python-rospkg, python-yaml
	Depends3: python3-catkin-pkg (>= 0.1.28), python3-rosdistro (>= 0.5.0), python3-rospkg, python3-yaml

WIP: Fix CLI use for source-only rosdistros. #44

WIP: Fix CLI use for source-only rosdistros. #44

Conversation

mikepurvis commented Feb 17, 2018 • edited Loading

sloretz commented Feb 20, 2018

dirk-thomas left a comment

Choose a reason for hiding this comment

dirk-thomas Feb 20, 2018

Choose a reason for hiding this comment

mikepurvis Feb 21, 2018

Choose a reason for hiding this comment

mikepurvis commented Feb 21, 2018

dirk-thomas left a comment

Choose a reason for hiding this comment

mikepurvis commented Feb 21, 2018

dirk-thomas commented Feb 21, 2018

mikepurvis commented Feb 22, 2018

dirk-thomas Feb 26, 2018

Choose a reason for hiding this comment

mikepurvis Feb 27, 2018

Choose a reason for hiding this comment

mikepurvis Feb 28, 2018

Choose a reason for hiding this comment

dirk-thomas Feb 28, 2018

Choose a reason for hiding this comment

mikepurvis Feb 28, 2018

Choose a reason for hiding this comment

dirk-thomas commented Feb 28, 2018

mikepurvis commented Feb 17, 2018 •

edited

Loading