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

feat: add source entry for ncnn into jazzy and rolling #42127

Closed
wants to merge 1 commit into from

Conversation

wep21
Copy link
Contributor

@wep21 wep21 commented Jul 19, 2024

Please add the following dependency to the rosdep database.

Package name:

TODO

Package Upstream Source:

TODO link to source repository

Purpose of using this:

TODO Replace. This dependency is being used for this reason. This is why I think it's valuable to be added to the rosdep database.

Distro packaging links:

Links to Distribution Packages

Please Add This Package to be indexed in the rosdistro.

ROSDISTRO NAME

The source is here:

https://github.com/Tencent/ncnn.git

Checks

  • All packages have a declared license in the package.xml
  • This repository has a LICENSE file
  • This package is expected to build on the submitted rosdistro

This is a third party package, so I will add package.xml to the release repository.
https://wiki.ros.org/bloom/Tutorials/ReleaseThirdParty

@wep21 wep21 requested a review from nuclearsandwich as a code owner July 19, 2024 04:36
@github-actions github-actions bot added jazzy Issue/PR is for the ROS 2 Jazzy distribution rolling Issue/PR is for the ROS 2 Rolling distribution labels Jul 19, 2024
@christophebedard christophebedard added the held for sync Issue/PR has been held because the distribution is in a sync hold label Jul 22, 2024
@christophebedard
Copy link
Member

Holding for Jazzy sync

@marcoag
Copy link
Contributor

marcoag commented Jul 23, 2024

I am very confused about this PR, is the intention to add a rosdep dependency or a ROS package to the jazzy and rolling distribution?

@marcoag marcoag added more information needed Maintainers have asked for additional information and removed held for sync Issue/PR has been held because the distribution is in a sync hold labels Jul 23, 2024
@wep21
Copy link
Contributor Author

wep21 commented Jul 23, 2024

@marcoag Unfortunately, ncnn is not provided as a debian/apt package, so I would like to register ncnn as a ROS package.

@christophebedard
Copy link
Member

christophebedard commented Jul 23, 2024

Will you be providing a package.xml file for it using a release repository? Like this: https://github.com/ros2-gbp/fastrtps-release/tree/master/rolling

@mjcarroll
Copy link
Member

While I have it on my backlog to write an article about this (#41221), this may be a good opportunity to create a vendor package for this piece of software, especially if upstream isn't interested in creating debs.

@wep21
Copy link
Contributor Author

wep21 commented Jul 24, 2024

@christophebedard The package.xml would be like

<package format="3">
  <name>ncnn</name>
  <version>20240410.0.0</version>
  <description>ncnn is a high-performance neural network inference computing framework optimized for mobile platforms.</description>

  <author email="[email protected]">nihui</author>
  <maintainer email="[email protected]">caishanli</maintainer>
  <license>BSD 3-Clause</license>

  <url type="website">https://github.com/Tencent/ncnn</url>

  <buildtool_depend>cmake</buildtool_depend>

  <depend>libomp-dev</depend>
  <depend>libopencv-dev</depend>
  <depend>libvulkan-dev</depend>
  <depend>protobuf-dev</depend>

  <export>
    <build_type>cmake</build_type>
  </export>
</package>

@clalancette clalancette added changes requested Maintainers have asked for changes to the pull request and removed more information needed Maintainers have asked for additional information labels Jul 31, 2024
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Just to reiterate @mjcarroll 's comment: given that this is not a ROS package, and contains no specific ROS things, this is more appropriate as a vendor package. You can look at https://github.com/ros2/spdlog_vendor for an example of a vendor package.

@wep21
Copy link
Contributor Author

wep21 commented Jul 31, 2024

@clalancette I feel creating a vendor package is redundant, so I don't feel like creating it. There are some cases just adding package.xml as patch such as https://github.com/ros2-gbp/azure_iot_sdk_c-release. Could you clarify the requirements to create a vendor package.

@wep21 wep21 requested a review from clalancette July 31, 2024 18:28
@clalancette
Copy link
Contributor

@clalancette I feel creating a vendor package is redundant, so I don't feel like creating it. There are some cases just adding package.xml as patch such as https://github.com/ros2-gbp/azure_iot_sdk_c-release. Could you clarify the requirements to create a vendor package.

The basic reason we discourage people from patching in the package.xml at bloom/release time is that people building from source won't get it. So they won't be able to use e.g. rosdep to install their dependencies.

By using a vendor package, you avoid this. The vendor package is a standard ROS package, so all of the tools work, either via source or via binaries. And then the actual code (that is being vendored) doesn't have to know about or care about ROS at all.

@gavanderhoorn
Copy link
Contributor

While I mostly agree with the reasoning, I'm wondering whether it's not acceptable for consumers to have to clone the release repository for packages like these (instead of the source repository).

We also accept other types of patches which only get added at bloom/release time. Having the package.xml only in the release repository doesn't seem like it's too different.

Building from the release repository also means you'd get "the same sources as the buildfarm sees", facilitating recreating a specific version/build.

@clalancette
Copy link
Contributor

While I mostly agree with the reasoning, I'm wondering whether it's not acceptable for consumers to have to clone the release repository for packages like these (instead of the source repository).

While this can work, I'm not a huge fan of it. It's not the form that people are generally used to interacting with, and doing development against it would be awkward. That's because it doesn't share any ancestors with the original source repository. So if you were doing development against this, you'd have to create patches, export them, then reapply them to the "true" source repository. It's possible, but not a great experience, in my opinion.

We also accept other types of patches which only get added at bloom/release time. Having the package.xml only in the release repository doesn't seem like it's too different.

I'll argue that it is different conceptually.

That is, putting a patch in at release time to fix the linking on a particular platform isn't a change in the nature of the package; it is fixing one particular issue for one particular package. Those seem fine to me, though even there, if we can patch at the source level, we still prefer to do that.

Putting a package.xml in at release time changes the nature of the package. It basically converts a package from a non-ROS package to a ROS package. Further, it only works for pure-CMake packages (if you tried to patch a package.xml into e.g. an autotools or python package, it wouldn't work properly).

Copy link

This PR hasn't been activity in 14 days. If you are still are interested in getting it merged please provide an update. Otherwise it will likely be closed by a rosdistro maintainer following our contributing policy. It's been labeled "stale" for visibility to the maintainers. If this label isn't appropriate, you can ask a maintainer to remove the label and add the 'persistent' label.

@github-actions github-actions bot added the stale Issue/PR hasn't been active in a while and may be closed. label Aug 16, 2024
@wep21
Copy link
Contributor Author

wep21 commented Aug 16, 2024

@clalancette Thank you for your detailed explanation. I gave up merging this PR once, and I will consider creating a vendor package.

@wep21 wep21 closed this Aug 16, 2024
@wep21 wep21 deleted the add-source-entry-ncnn branch August 16, 2024 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes requested Maintainers have asked for changes to the pull request jazzy Issue/PR is for the ROS 2 Jazzy distribution rolling Issue/PR is for the ROS 2 Rolling distribution stale Issue/PR hasn't been active in a while and may be closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants