use ament_python_install_package when package is not installed

[knorth55]

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 ros-drivers/audio_common#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]

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

[quarkytale]

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

• Linux
• Linux-aarch64
• Windows

[knorth55]

I update this PR to follow this comment.
thank you @MarcoMagriDev
07ff2fc#r98895589

[knorth55]

@sloretz kindly ping. can you review this PR?
#141 is a huge bug in humble and rolling, and this PR fixes the issue.

[sillkjc]

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]

@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]

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 😄

Greetings!

[knorth55]

@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.

[sloretz]

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)

[knorth55]

@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]

Any update on this?

[clalancette]

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]

@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)...
#187 (comment)

[clalancette]

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]

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

[brta-jc]

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]

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]

@quarkytale for review?

[rolker]

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: ament/ament_cmake#517
I would appreciate any help in improving the quality of the commit to make sure it get accepted and solves our problem.

[clalancette]

See my comment in #141 (comment) . I'm going to close this in favor of ament/ament_cmake#514