ros2 / tinyxml2_vendor

temporary vendor package for tinyxml2
Apache License 2.0
2 stars 9 forks source link

why are you not building this package as other vendor packages? #11

Closed asobhy-qnx closed 3 years ago

asobhy-qnx commented 3 years ago

why not provide an external project add for this package and automatically grab the source code and build it?

clalancette commented 3 years ago

For this particular vendor package, we have external packages available on all of our target platforms (in apt for Ubuntu, homebrew for macOS, and chocolatey for Windows). When that is the case, we prefer to use the external package instead of building it from source.

Are you running into a particular problem with the way this works? If so, can you please give some more information on the problem you are seeing?

asobhy-qnx commented 3 years ago

for QNX it has to be built from source

clalancette commented 3 years ago

for QNX it has to be built from source

There are 2 ways to solve this:

  1. If there is a concept of packages on QNX, then we can make part of the setup depend on that. This is the preferred solution if it is available.
  2. If we can't do the first one, then we could make this a true vendored package like other ones. For our primary support platforms, it would essentially be a no-op, but for QNX it would actually build something.
asobhy-qnx commented 3 years ago

So for QNX there are several dependencies that users will need to compile from source such as:

Foxy, Dashing: apr apr-util log4cxx asio eigen3 libpng16 opencv numpy lxml libffi tinyxml2

Dashing: libxslt

Foxy: netifaces libbullet-dev

We plan to provide instructions on how to build those dependencies from source. We will not be providing binary packages to the public, because it is a matter of liability from our side.

clalancette commented 3 years ago

So for QNX there are several dependencies that users will need to compile from source such as:

Right, so this just remains as part of that list. Is that a satisfactory solution for you?

asobhy-qnx commented 3 years ago

I honestly prefer solution number 2 that you suggested above

clalancette commented 3 years ago

I honestly prefer solution number 2 that you suggested above

It's not ideal to me, but I'm OK with it. @ros2/team any other opinions here?

wjwwood commented 3 years ago

It's always better long term to push dependencies onto the package manager of the OS if possible, but if QNX isn't able to do that then building from source within the vendor package is fine.

sloretz commented 3 years ago

Definitely the first option is less effort to maintain. There are always going to be system dependencies, and if another platform doesn't have them then they will need to be compiled from source. Building just one more in that list seems like only a tiny help.

There was an ExternalProject_Add() in the initial commit, but it was removed in #1. All that appears to be left is a CMake module. I would recommend tinyxml2_vendor be replaced with a tinyxml2_cmake_module package like https://github.com/ros2/eigen3_cmake_module to avoid the expectations that come with a vendor package.

asobhy-qnx commented 3 years ago

@sloretz my perspective and my goal is to make the steps for building ros2 for users as easy as it can be. The easier it is to build ROS2 from source whether on QNX or any other OS the more attractive it will be for users and as a result more people using ROS.

I see vendor packages as an automated and easy way to build the dependencies needed for ROS. The developer only has to run one command "colcon" to build the entire src directory with all the dependencies contained within, how convenient is that. Unfortunately for QNX some dependencies have to be built from source and the shorter this list of dependencies is, the easier it is for the user. I can definitely provide instructions to build tinyxml2 independently if it will be a hassle for you.

There are may users in the industry and in educational institutions that are using QNX and are interested in using ROS2 + QNX. There are also QNX users who developed their own port of ROS2 on QNX, and ended up spending resources doing that. I think both of our goals should be to make this more attractive to them as much as possible, so doing a bit of extra work to make it easier for the end user is in both of our benefits I believe.

sloretz commented 3 years ago

I see vendor packages as an automated and easy way to build the dependencies needed for ROS. The developer only has to run one command "colcon" to build the entire src directory with all the dependencies contained within, how convenient is that.

I definitely see the value of having a CMake package in a colcon workspace that builds an system dependencies on platforms that don't have them. In tinyxml2's case, it wouldn't be used on any of the target platforms in REP 2000. All that's needed here is a CMake module.

Unfortunately for QNX some dependencies have to be built from source and the shorter this list of dependencies is, the easier it is for the user.

Shorter, but there would still be a lot of other system dependencies. Right now the vendor packages are released into the ROS distro so that binaries are built for it. I'm not sure the vendor package is a good fit for this use case. It would lead down the path of adding a bullet_vendor and netifaces_vendor. I would recommend something a little bit different for a package that is only expected to be built from source, and not released.

Say there was a CMake package which only had a CMakeLists.txt and a package.xml. The CMakeLists.txt would have an ExternalProject_Add() to build tinyxml2 from source and install() calls to install the artifacts. The package.xml would say the package name was tinyxml2, the same name as the rosdep key for tinyxml2. If you have this package in your workspace, it will be built from source, and packages depending on tinyxml2 will use it. If you don't have this package in your workspace, then it can still be installed as a system dependency via rosdep, and packages depending on tinyxml2 will use the system version.

I think that pattern that could be applied in general without the maintenance burden of having to release those packages. I would recommend making a package like that and open sourcing it for other QNX+ROS 2 users rather than having a tinyxml2_vendor package.

asobhy-qnx commented 3 years ago

Yes that could be an option. I will keep tinyxml2 in the list of QNX dependencies then

clalancette commented 3 years ago

@asobhy-qnx Is it OK to close this out now?

asobhy-qnx commented 3 years ago

yes