Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove invalid dependency in rmw_ecal_shared_cpp. #61

Merged
merged 1 commit into from
May 22, 2023
Merged

Remove invalid dependency in rmw_ecal_shared_cpp. #61

merged 1 commit into from
May 22, 2023

Conversation

JWhitleyWork
Copy link
Contributor

Dependency keys for package.xml are used by rosdep to resolve and install dependencies. Specifically, system keys (non-ROS-packages) are defined here. The key eCAL does not exist and causes rosdep to fail to properly resolve dependencies for the rmw_ecal_shared_cpp package. Since eCAL is ideally installed as a system package (e.g. to /usr) and should be resolvable by CMake if installed, there is no need for this key and it only causes problems.

Signed-off-by: Joshua Whitley <josh@electrifiedautonomy.com>
@JWhitleyWork
Copy link
Contributor Author

FYI, the above being said, the "proper" way to handle the above is one of the following:

  1. Release eCAL to a Debian or Ubuntu host repository (not fun, lots of maintenance hoops).
  2. Release an eCAL "ROS Wrapper" package which uses CMake to download and install eCAL as a ROS-only installation (also not ideal since this makes eCAL tied to the installed ROS environment).
  3. Make the eCAL repo into a colcon-buildable package by adding a package.xml and some CMake rules at the base of the eCAL repo.

3 is the ideal scenario. See the CycloneDDS package.xml and CMakeLists.txt and colcon.pkg files for great examples. Is this something you think the eCAL group would be interested in doing?

@FlorianReimold
Copy link
Member

FYI, the above being said, the "proper" way to handle the above is one of the following:

  1. Release eCAL to a Debian or Ubuntu host repository (not fun, lots of maintenance hoops).

You mean like making it an official Ubuntu package? That is something we would actually like to do. We are afraid however, that the eCAL Version we release for a specific Ubuntu Release will be deprecated quickly, as Ubuntu lives much longer than a specific eCAL Version. Also - you are right with that - it is some extra work we need to justify doing.
Therefore, we currently only provide the PPAs, where you can choose the eCAL minor version yourself.

  1. Release an eCAL "ROS Wrapper" package which uses CMake to download and install eCAL as a ROS-only installation (also not ideal since this makes eCAL tied to the installed ROS environment).

I actually don't know how to do that, as I have never looked into it. Our old maintainer probably knew.

  1. Make the eCAL repo into a colcon-buildable package by adding a package.xml and some CMake rules at the base of the eCAL repo.

That would be totally fine for me. If it helps and doesn't break anything else, we can add pretty much anything to the eCAL repo and the main CMakeLists.txt. If the build artifacts only end up in some private ROS directories anyways, I would suggest to only build the ecal_core.so with those files to keep the compile time and the requirement list as low as possible. The user can install the eCAL tools from the PPA, .deb file or copile them separately. Is that a good idea?

3 is the ideal scenario. See the CycloneDDS package.xml and CMakeLists.txt and colcon.pkg files for great examples. Is this something you think the eCAL group would be interested in doing?

It always depends on the amount of work, this will impose on us, as we are not actively using eCAL in ROS at the moment. But If you can support with that, option 3 sounds like a very clean solution that I would love to see for eCAL.

@JWhitleyWork
Copy link
Contributor Author

The failures in CI are all network issues. Closing and re-opening to kick CI.

@JWhitleyWork
Copy link
Contributor Author

@FlorianReimold Thanks for the reply! I will carefully consider how to best do this. Cyclone has a special deal with OSRF to be their "release agent" for their DDS implementation but since DDS is technically the only "supported" transport mechanism, I don't know if they would offer eCAL the same.

@FlorianReimold
Copy link
Member

Closing and re-opening to kick CI.

I think that won't work, as the CI will wait for our approval to run actions from first time contributors. I approved it.

@FlorianReimold FlorianReimold merged commit 4280658 into eclipse-ecal:master May 22, 2023
4 of 8 checks passed
@JWhitleyWork JWhitleyWork deleted the remove-useless-dependency branch May 22, 2023 07:31
@JWhitleyWork
Copy link
Contributor Author

Thank you!

@FlorianReimold
Copy link
Member

Now that there is a commit on master, issued by you, the CI will automatically run your workflows.

@JWhitleyWork
Copy link
Contributor Author

Now that there is a commit on master, issued by you, the CI will automatically run your workflows.

Good to know, thanks!

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.

None yet

2 participants