Skip to content

Extend workspace packages using ament index for ROS2 support#699

Merged
wjwwood merged 8 commits intoros-infrastructure:masterfrom
ruffsl:ament
Sep 12, 2019
Merged

Extend workspace packages using ament index for ROS2 support#699
wjwwood merged 8 commits intoros-infrastructure:masterfrom
ruffsl:ament

Conversation

@ruffsl
Copy link
Contributor

@ruffsl ruffsl commented Aug 14, 2019

Fixes #660

ruffsl added 3 commits August 13, 2019 23:50
Signed-off-by: ruffsl <roxfoxpox@gmail.com>
Signed-off-by: ruffsl <roxfoxpox@gmail.com>
Signed-off-by: ruffsl <roxfoxpox@gmail.com>
Signed-off-by: ruffsl <roxfoxpox@gmail.com>
@codecov-io
Copy link

codecov-io commented Aug 15, 2019

Codecov Report

Merging #699 into master will increase coverage by 0.29%.
The diff coverage is 95.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #699      +/-   ##
==========================================
+ Coverage   75.47%   75.77%   +0.29%     
==========================================
  Files          32       37       +5     
  Lines        2968     3013      +45     
==========================================
+ Hits         2240     2283      +43     
- Misses        728      730       +2
Impacted Files Coverage Δ
src/rosdep2/ament_packages/packages.py 100% <100%> (ø)
src/rosdep2/ament_packages/constants.py 100% <100%> (ø)
src/rosdep2/ament_packages/resources.py 100% <100%> (ø)
src/rosdep2/ament_packages/__init__.py 100% <100%> (ø)
src/rosdep2/ament_packages/search_paths.py 87.5% <87.5%> (ø)
src/rosdep2/main.py 49.35% <90%> (+0.76%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab1f06e...a52f89f. Read the comment docs.

@dirk-thomas dirk-thomas requested a review from wjwwood August 26, 2019 20:44
Copy link
Contributor

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

Copying the code sucks, but I guess it's unavoidable right now. However, please make a comment (or a README.md in the ament_packages folder) mentioning that this code is essentially copied directly from ament_package or ament_index_python or wherever it came from. That way it can be more easily updated in the future.

I think at least one test would be a good idea, and maybe one for the case I mentioned in my comment (mixing ROS 1 and ament packages).

Add fake_catkin_package that depends on packages in ament inedex.
Also add testboost as a colliding package in ament inedex.
@ruffsl
Copy link
Contributor Author

ruffsl commented Aug 28, 2019

@wjwwood , I've added the readme linking to the repo/commit copied from, as well a extension to the install tests to check the behavior when colliding packages from ament and package path.

@ruffsl
Copy link
Contributor Author

ruffsl commented Sep 1, 2019

While testing this branch in the wild, I found the ament_packages sub module wasn't being installed. I've updated the setup.py to fix this and can confirm this patch is working as expected. See this before, after Dockerfile that achieve the same effect, now without needing to monkeypatch ROS_PACKAGE_PATH everyware: ros-swg/turtlebot3_demo@97d9c33

Could we merge this upstream for the next eventual sync?

@ruffsl
Copy link
Contributor Author

ruffsl commented Sep 3, 2019

I'll be triggering an update to the ros docker image library soon. I would be neat to have this included.
bump @wjwwood @dirk-thomas .

Copy link
Contributor

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

@wjwwood
Copy link
Contributor

wjwwood commented Sep 4, 2019

I'll try to merge and release this asap, but I can't do it today.

@ruffsl
Copy link
Contributor Author

ruffsl commented Sep 11, 2019

@wjwwood , now that dashing sync is out, I'd like to get this in the docker library rebuild as well.
Please ping back when ready so that I can open the PR upstream.

@wjwwood wjwwood merged commit 6b0c25f into ros-infrastructure:master Sep 12, 2019
@wjwwood
Copy link
Contributor

wjwwood commented Sep 12, 2019

Looks like either I will release this next week (I'm pretty busy for the rest of this week), or maybe @nuclearsandwich because I think he mentioned on discourse he may do a release soon.

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.

rosdep and ROS2

4 participants