search

ros2 / rosidl_python

rosidl support for Python
Apache License 2.0
19 stars 45 forks source link

use ament_python_install_package when package is not installed #187

Closed knorth55 closed 4 months ago

knorth55 commented 1 year ago

this PR fix #141 with this PR, we can use rosidl_generator_interfaces and ament_python_install_package in the same CMakeLists.txt.

But the order is important. as https://github.com/ros-drivers/audio_common/pull/212, we need to ament_python_install_package first and rosidl_generate_interfaces later.

ament_python_install_package(${PROJECT_NAME})

rosidl_generate_interfaces(${PROJECT_NAME}
  ${msg_files}
  ${action_files}
  DEPENDENCIES
    action_msgs
    audio_common_msgs
    builtin_interfaces
)
knorth55 commented 1 year ago

@clalancette kindly ping. I made a PR to avoid the error reported in #141 . Could you check this PR?

quarkytale commented 1 year ago

This makes sense to me based on the conversation in the linked issue. CI:

knorth55 commented 1 year ago

I update this PR to follow this comment. thank you @MarcoMagriDev https://github.com/ros2/rosidl_python/commit/07ff2fcc47ef64bb600f1c54329d3bda22ac047e#r98895589

knorth55 commented 1 year ago

@sloretz kindly ping. can you review this PR?

141 is a huge bug in humble and rolling, and this PR fixes the issue.

sillkjc commented 1 year ago

It's rather insane we got to this far after Humble release and this is an issue. We're attempting to migrate our large code base from Galactic -> Humble and this has come up. We follow our own design pattern which does not include having messages in their own package due to the overhead required to maintain so many packages, as well as legacy. Then this gets sprung on us without any rep discussion, conversion warnings or understanding of how big an issue this is.

Sometimes you want messages and python in the one package. This or a similar fix really needs to be merged.

knorth55 commented 1 year ago

@sloretz kindly ping. can you review and merge this pr? sound_play in audio_common cannot be built in humble with this change. rosidl_python in humble has serious build error in #141

dgarcu commented 1 year ago

Hi there,

I recently have installed audio_common package, but I needed the assist of @knorth55 to do so. It is working just fine. Apparently the bug that prevented me to do it on my own is already fixed on his contributions. Just a friendly reminder! I am sure that it is worth the effort :smile:

Greetings!

knorth55 commented 1 year ago

@clalancette i pinged @sloretz several times, but i got no response. can you or others make a review for this PR? some people are also facing the same issue.

knorth55 commented 1 year ago

@sloretz

Thank you for your review. I agree your idea and it is ideal, but it requires packages to change cmake. Also, this issue stops many packages release, so merging this hot fix is also a good thing, IMO.

I will update ament_python_install_package near future, but I'm not sure when it will be. I cannot make enough time to do that now.

So my plan is

  1. merge this PR
  2. update ament_python_install_package
  3. revert this PR

what do you think?

MarcoMagriDev commented 9 months ago

Any update on this?

clalancette commented 9 months ago

Any update on this?

I think we are waiting on an update to ament_python_install_package, which is the way forward that @sloretz pointed out.

knorth55 commented 9 months ago

@clalancette I don't get any reply about my suggestion, but this PR will not be merged as a hotfix? I can work on to solve the issue in ament_python_install_package, but it will take time (on weekend project)... https://github.com/ros2/rosidl_python/pull/187#issuecomment-1582106506

clalancette commented 9 months ago

I don't get any reply about my suggestion, but this PR will not be merged as a hotfix?

I don't think we should do that, no. Workarounds like this tend to live forever once they are in. Given that we have another way forward, I think we should do that.

knorth55 commented 9 months ago

@clalancette thanks. I see. I will work on the other way.

brta-jc commented 9 months ago

At this point I think it is okay if it stays forever, because it will allow us to move forward. It's been on an ongoing issue for a year now. This prevents us merging code that can run on ROS-industrial-CI pipelines on upstream Github. Please merge, revert later if necessary.

RoBeau commented 9 months ago

I would also vote for a short term solution. Otherwise is there anyway we can get support from the core maintainers to fix this issue? Seems like building packages with messages in them is a pretty core feature....

brta-jc commented 4 months ago

@quarkytale for review?

rolker commented 4 months ago

Thanks for the PR!

It looks like this is working around an issue where ament_python_install_package() doesn't allow the same package name to be installed twice. I see why this would be important for packages that both generate interfaces and ship a python library. I don't think this is the right place for it. It exposes ament_cmake_python's internal variable name AMENT_CMAKE_PYTHON_INSTALL_INSTALLED_NAMES to rosidl_generator_py, and once we add it here I don't see a path forward to eventually remove it.

For the time being I would recommend putting this code in the affected package itself instead of it calling ament_python_install_package() a second time.

I think the best option would be to update ament_python_install_package() to allow installing more files into an existing package.

For example,

rosidl_generate_interfaces(...)

ament_python_install_package(... EXTEND_EXISTING)

Here's my attempt to solve this: https://github.com/ament/ament_cmake/pull/517 I would appreciate any help in improving the quality of the commit to make sure it get accepted and solves our problem.

clalancette commented 4 months ago

See my comment in https://github.com/ros2/rosidl_python/issues/141#issuecomment-1986288774 . I'm going to close this in favor of https://github.com/ament/ament_cmake/issues/514