rosin-project / rxros

Reactive programming for ROS
BSD 3-Clause "New" or "Revised" License
48 stars 6 forks source link

Missing or needless dependency on tf? #3

Closed wasowski closed 5 years ago

wasowski commented 5 years ago

I am trying to follow the installation instructions and I fail on the catkin_call. It appears that we have a dependency on tf, but I don't know exactly where. I think this dependency should not be there, but @henrik7264 try to convince me (then we should put it in package.xml).

Or do I misinterpret the error message @gavanderhoorn ?

wasowski@research104:~/work/rx_ros_ws$ docker run -it ros
root@6aaceb91be0e:/# mkdir rxros_ws
root@6aaceb91be0e:/# cd rxros_ws/
root@6aaceb91be0e:/rxros_ws# ls
root@6aaceb91be0e:/rxros_ws#  wstool init src https://raw.githubusercontent.com/rosin-project/rx_ros/master/rosinstall.yaml
Using initial elements from: https://raw.githubusercontent.com/rosin-project/rx_ros/master/rosinstall.yaml
Writing /rxros_ws/src/.rosinstall
[rx_ros] Fetching https://github.com/rosin-project/rx_ros (version None) to /rxros_ws/src/rx_ros
Cloning into '/rxros_ws/src/rx_ros'...
remote: Enumerating objects: 257, done.
remote: Counting objects: 100% (257/257), done.
remote: Compressing objects: 100% (176/176), done.
remote: Total 257 (delta 91), reused 231 (delta 73), pack-reused 0
Receiving objects: 100% (257/257), 250.41 KiB | 925.00 KiB/s, done.
Resolving deltas: 100% (91/91), done.
[rx_ros] Done.

update complete.
root@6aaceb91be0e:/rxros_ws# catkin_make
Base path: /rxros_ws
Source space: /rxros_ws/src
Build space: /rxros_ws/build
Devel space: /rxros_ws/devel
Install space: /rxros_ws/install
Creating symlink "/rxros_ws/src/CMakeLists.txt" pointing to "/opt/ros/melodic/share/catkin/cmake/toplevel.cmake"
####
#### Running command: "cmake /rxros_ws/src -DCATKIN_DEVEL_PREFIX=/rxros_ws/devel -DCMAKE_INSTALL_PREFIX=/rxros_ws/install
-G Unix Makefiles" in "/rxros_ws/build"
####
-- The C compiler identification is GNU 7.4.0
-- The CXX compiler identification is GNU 7.4.0
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Using CATKIN_DEVEL_PREFIX: /rxros_ws/devel
-- Using CMAKE_PREFIX_PATH: /opt/ros/melodic
-- This workspace overlays: /opt/ros/melodic
-- Found PythonInterp: /usr/bin/python2 (found suitable version "2.7.15", minimum required is "2")
-- Using PYTHON_EXECUTABLE: /usr/bin/python2
-- Using Debian Python package layout
-- Using empy: /usr/bin/empy
-- Using CATKIN_ENABLE_TESTING: ON
-- Call enable_testing()
-- Using CATKIN_TEST_RESULTS_DIR: /rxros_ws/build/test_results
-- Found gmock sources under '/usr/src/googletest': gmock will be built
-- Found PythonInterp: /usr/bin/python2 (found version "2.7.15")
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Looking for pthread_create
-- Looking for pthread_create - not found
-- Looking for pthread_create in pthreads
-- Looking for pthread_create in pthreads - not found
-- Looking for pthread_create in pthread
-- Looking for pthread_create in pthread - found
-- Found Threads: TRUE
-- Found gtest sources under '/usr/src/googletest': gtests will be built
-- Using Python nosetests: /usr/bin/nosetests-2.7
-- catkin 0.7.17
-- BUILD_SHARED_LIBS is on
-- BUILD_SHARED_LIBS is on
-- ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-- ~~  traversing 5 packages in topological order:
-- ~~  - teleop_msgs
-- ~~  - brickpi3_msgs
-- ~~  - rxros
-- ~~  - teleop
-- ~~  - brickpi3
-- ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-- +++ processing catkin package: 'teleop_msgs'
-- ==> add_subdirectory(rx_ros/examples/teleop_msgs)
-- Using these message generators: gencpp;geneus;genlisp;gennodejs;genpy
-- teleop_msgs: 2 messages, 0 services
-- +++ processing catkin package: 'brickpi3_msgs'
-- ==> add_subdirectory(rx_ros/examples/brickpi3_msgs)
-- Using these message generators: gencpp;geneus;genlisp;gennodejs;genpy
-- brickpi3_msgs: 7 messages, 0 services
-- +++ processing catkin package: 'rxros'
-- ==> add_subdirectory(rx_ros/rxros)
-- +++ processing catkin package: 'teleop'
-- ==> add_subdirectory(rx_ros/examples/teleop)
-- +++ processing catkin package: 'brickpi3'
-- ==> add_subdirectory(rx_ros/examples/brickpi3)
-- Using these message generators: gencpp;geneus;genlisp;gennodejs;genpy
-- Could NOT find tf (missing: tf_DIR)
-- Could not find the required component 'tf'. The following CMake error indicates that you either need to install the pac
kage with the same name or change your environment so that it can be found.
CMake Error at /opt/ros/melodic/share/catkin/cmake/catkinConfig.cmake:83 (find_package):
  Could not find a package configuration file provided by "tf" with any of
  the following names:

    tfConfig.cmake
    tf-config.cmake

  Add the installation prefix of "tf" to CMAKE_PREFIX_PATH or set "tf_DIR" to
  a directory containing one of the above files.  If "tf" provides a separate
  development package or SDK, be sure it has been installed.
Call Stack (most recent call first):
  rx_ros/examples/brickpi3/CMakeLists.txt:10 (find_package)

-- Configuring incomplete, errors occurred!
See also "/rxros_ws/build/CMakeFiles/CMakeOutput.log".
See also "/rxros_ws/build/CMakeFiles/CMakeError.log".
Invoking "cmake" failed
gavanderhoorn commented 5 years ago

Or do I misinterpret the error message @gavanderhoorn ?

No, you're correct.

The brickpi3 package has a build_depend on tf. It also correctly states that here.

What I'm missing in your console output is a rosdep check .. or rosdep install .. step, which would've made sure that the tf package is installed.

wasowski commented 5 years ago

I am thinking that it would be nice to factor out the examples to a separate package ...

gavanderhoorn commented 5 years ago

I am thinking that it would be nice to factor out the examples to a separate package ...

you mean a separate repository? They already are in separate packages.

wasowski commented 5 years ago

oh right. of course, my knowledge is short as usual :)

We can only install the entire stack into a workspace, right? Then probably a separate repo. But I could do turtlesim, or, even a simpler, a publisher/subscriber example, to minimize deps for packages included in the repo; How this is normally done in ROS?

gavanderhoorn commented 5 years ago

Examples are certainly always in a separate package. Sometimes in a separate repository.

A good example of this is the ros_tutorials repository. It contains demo packages for a nr of client libraries and other ROS related infrastructure. We could create something similar.

The teleop example seems relatively stand-alone though. That could probably be kept with the package in this repository.

gavanderhoorn commented 5 years ago

Related: #5.

wasowski commented 5 years ago

The rxros library is very thin, and it just relates to how roscore api is presented, so I think it would be beneficial to minimize the dependencies on the stack altogether. People may want to use it on some minimal ros systems.

gavanderhoorn commented 5 years ago

The teleop example only depends on: roscpp, rxros, std_msgs, teleop_msgs, message_generation, message_runtime and catkin.

That is pretty minimal, and most of those will be part of a minimal ROS installation already.

But I'm fine with moving the examples out of this repository.

It would just require an extra line in the .rosinstall file to check it out in case a user wants a batteries-included experience.

henrik7264 commented 5 years ago

Hi Andrzej and Gijs

Oh my what have I done now. I am on my way home from work. I shall l look at it as soon as I come home.

Regards . Henrik

wasowski commented 5 years ago

Don't worry Henrik. We are just trying to help. First look at the PR that @gavanderhoorn has sent you.

@gavanderhoorn , I think teleop should stay in here, but the brick stuff is way too heavy for the standard distribution.

I just tested that the version without any examples succeeds with catkin_make. But I made this branch too early, before we agreed that teleop will stay. So I will try to make another one, with teleop.

gavanderhoorn commented 5 years ago

I think teleop should stay in here, but the brick stuff is way too heavy for the standard distribution.

agreed.

wasowski commented 5 years ago

My joy was premature. The core file rxros.h has a direct dependency on tf. See:

https://github.com/rosin-project/rx_ros/blob/467f6c8b47f087b8b8b97224033cf6be97e52f3e/rxros/include/rxros.h#L222-L225

The build without examples succeeds, because bare bones header files always compile :). It is nice to have at least one example in the same repo as we force compilation and detect dependency bugs.

Since it is the core thing that depends on tf, I think we should maintain the dependency first. And once the release is tiedied up, we can refactor rxros_tf from rxros. Is this the right course of action @gavanderhoorn or am I being paranoid?

gavanderhoorn commented 5 years ago

The build without examples succeeds, because bare bones header files always compile :). It is nice to have at least one example in the same repo as we force compilation and detect dependency bugs.

Agreed.

Since it is the core thing that depends on tf, I think we should maintain the dependency first. And once the release is tiedied up, we can refactor rxros_tf from rxros. Is this the right course of action @gavanderhoorn or am I being paranoid?

No, your concern is very much justified.

I would go even further and say that we should create a rxros_tf immediately and just factor it out before even releasing rxros. One additional package takes only a small amount of time and it would very cleanly split dependencies in a sensible way.

henrik7264 commented 5 years ago

Hi Andrzej and Gijs,

I remembered similar problems from the past. There are as I remember it three ways to install ROS Melodic:

ros-melodic-desktop-full ros-melodic-desktop ros-melodic-ros-base

I have always used the "ros-melodic-desktop-full” installation to avoid these dependencies problems. Especially the examples sources requires both messages and tf from the desktop-full installation

I don’t know if it is to much to require?

henrik7264 commented 5 years ago

Hi Gijs and Andrzej,

I am actually a bit sorry for moving the examples. They do provide different examples of how RxROS can be applied to solve more complex problems. I would actually like to end up with a full example of a LEGO robot that is based on RxROS.

I agree that it is not especially nice that they can not compile without the ros-melodic-desktop-full installation. Is your expectation that rx_ros can be build using only ros-melodic-ros-base ?

What do you intend to put in the rxros_tf package? I am not sure if I can follow you here?

/Henrik

gavanderhoorn commented 5 years ago

@henrik7264: the package.xml file supports specifying the dependencies of a package very precisely. The rxros package manifest should simply state it depends on tf. Any user building from source could then use rosdep to install all required dependencies automatically. The buildfarm would be able to automatically figure out which packages are needed to properly build the package as well.

We must do this, as otherwise a release would be impossible (the buildfarm is automated, it does not read your readme).

I have always used the "ros-melodic-desktop-full” installation to avoid these dependencies problems. Especially the examples sources requires both messages and tf from the desktop-full installation

I don’t know if it is to much to require?

a ros-melodic-desktop-full install is gigabytes in size (and includes things such as Gazebo). Besides installing many unneeded packages, it's also rather wasteful, when a simple dependency on tf would completely solve all the problems.

Personally, I would say "yes, installing all of ros-melodic-desktop-full just to be able to depend on tf is too much".

I am actually a bit sorry for moving the examples. They do provide different examples of how RxROS can be applied to solve more complex problems. I would actually like to end up with a full example of a LEGO robot that is based on RxROS.

Putting packages in a separate repository does not mean that they disappear. In fact, the exact location of packages doesn't really matter: wstool will happily clone repositories from wherever you desire, and this is all opaque to the user.

We're not deleting or destroying the examples, but are trying to make it easier to depend on rxros when users don't want or need the examples. Storing them in a separate repository makes this somewhat easier to achieve.

I agree that it is not especially nice that they can not compile without the ros-melodic-desktop-full installation. Is your expectation that rx_ros can be build using only ros-melodic-ros-base ? What do you intend to put in the rxros_tf package? I am not sure if I can follow you here?

rxros_tf would probably contain a single header that contains the from_transform(..) function.

Users not interested in TF could ignore that package and would only need roscpp installed. Users that are interested in TF would have both rxros and rxros_tf installed (the latter depending on the former). This is a very common pattern in ROS used to avoid introducing unnecessary or unwanted dependencies and is relatively low-overhead.

henrik7264 commented 5 years ago

Hi Gijs & Andrzej,

Thanks Gijs for the excelent explanation - it helped me a lot. I must admit I am very happy with the contents of current workspace/project, but OK it gives good meaning to separate the examples from the library. It will be a clean separation. I will not leave any examples in rx_ros. I will however update the README.md file and explain how to install a build the examples. You are welcome to create a rx_ros_examples project or something similar (you are also welcome to remove the rx_ros-release project - I don’t think we will need that any more)

I will play a bit with the rxros_tf. I am not entirely happy about the solution as I think we are on the road too complicated setup/installation. What would you instead say to a

  1. A solution with a dependent build option?
  2. A solution with 2 installation files (I am not entirely sure this is a good idea - it depends a on how flexible the installation.yaml files are)?
  3. Remove the from_transform entirely - for the moment?

I will try to get the separation finished tomorrow and include your comments.

/Henrik

gavanderhoorn commented 5 years ago

My suggestions:

  1. create a separate rx_ros_examples repository (I will do this)
  2. move all readme content that discusses the examples, their dependencies and other details to the readme of rx_ros_examples
  3. point to the new rx_ros_examples repository from the rx_ros readme

that way the base library in rx_ros is very simple to setup, setup/installation is very easy to grok and the release process of rxros is also straightforward.

I will play a bit with the rxros_tf. I am not entirely happy about the solution as I think we are on the road too complicated setup/installation.

I realise it may seem that way, but consider this:

you are also welcome to remove the rx_ros-release project - I don’t think we will need that any more)

I believe we do: rx_ros (without the examples) should be releasable through the buildfarm. I'm currently working on packaging RxCpp itself as a ROS package (very thin wrapper) which would remove the need to copy the headers here in this repository (easier tracking of upstream changes, easier licensing, etc) and allows us to depend on it from rxros.

gavanderhoorn commented 5 years ago

I will try to get the separation finished tomorrow and include your comments.

Could you please first look at #7?

gavanderhoorn commented 5 years ago

And #13.

henrik7264 commented 5 years ago

Here is another proposal to the tf problem. My biggest concern about splitting the rxros library is:

  1. The library files get scattered/spread to different packages. I like the idea that they are contained in one directory. Future development of the library will profit from this.
  2. The end user will have to make a decision whether he wants the tf or just the core version. Over time this concept will just be more and more complicated. The end user should not need to take such decisions.

So here is another proposal. It is not completely implemented, but the basic features are working. The proposal is not without problems, but lets look at them later. Here is the proposal:

  1. The overall idea is to have a rxros_config.h file that the rxros.h will include. The rxros_config.h could look as follows (slightly simplified) on a machine where the ROS tf packages are installed:
#ifndef RXROS_CONFIG
#define RXROS_CONFIG

#define ROS_TF_VERSION 1.12.0

#endif

The rxros_config.h file will allow us to control what should be enabled or not in the rxros.h file.

  1. The rxros_config.h will be generated by a simple script that is executed each time the rxros workspace is build. The script will collect information about what is installed on the users machine and create a number of define statements that can be used in the rxros.h file to include/exclude code.

The bad thing about this solution is the rxros workspace will have to be rebuild each time the user changes his ROS installation. The other bad thing is the code will contain several conditional statements like #ifdef ROS_TF_VERSION - I personally hate this kind of code, but I think I can organize the code so it will look nice and easy to maintain.

Can you follow me and what do you think about the idea?

gavanderhoorn commented 5 years ago

If I understand you correctly, you'd like to:

and not made explicit but:

The alternative that was discussed earlier in this thread (and in #8):

Can you follow me and what do you think about the idea?

regardless of my personal opinion, I believe that there are some downsides to your proposal, some of which you've already identified yourself:

The library files get scattered/spread to different packages. I like the idea that they are contained in one directory. Future development of the library will profit from this.

To be fair, the idea in #8 does split up rxros in two parts: one part with and one without tf support. The tf addon cannot be used without the base package.

However, I believe and expect ROS users to be able to deal with this: it's used in other parts of ROS (roscpp and tf are split along similar lines)and it's used in non-ROS situations as well (in the end it's essentially the plugin/extension idea, but with packages).

The end user will have to make a decision whether he wants the tf or just the core version. Over time this concept will just be more and more complicated. The end user should not need to take such decisions.

Doesn't the alternative proposal do exactly the same thing (ie: make the user responsible for this configuration choice)? It's just a header with some #ifdefs now, but that's the same as changing your CMakeLists.txt to add another dependency that brings in TF support and adding an #include. But in #8, the changes are only in user code. In this alternative, the user would be changing their own code (using the APIs that use TF) as well as changing (y)our code (== bad, for reasons stated above).

gavanderhoorn commented 5 years ago

One additional comment on the buildfarm aspect: the alternative makes it rather difficult to release rxros as a binary package. We'd have two choices:

  1. release with TF support
  2. release without TF support

But in reality, we'd only have a single option: release with TF support, as we cannot release the same package twice and in order for everything to keep working, a user must have TF already installed on his system to be able to even have the option to choose to enable it in rxros.

For that we need to add the build and run depend on tf to rxros, otherwise we cannot make sure TF is present on the user's system.

And with that we're back to the initial problem that we started out to solve: avoid installing unnecessary dependencies on the user's system.

henrik7264 commented 5 years ago

Thanks Gijs for all the information. Yes, I agree that my proposed solution by far is optimal. I will let your experience and argumentation in this area come the benefit of the project - so lets go for the rxros_tf package and split the rxros.h file up accordingly. I may not like it, but I may get a pleasant surprise. Please let me know if I can support you in any way.

gavanderhoorn commented 5 years ago

Thanks Gijs for all the information. Yes, I agree that my proposed solution by far is optimal. I will let your experience and argumentation in this area come the benefit of the project - so lets go for the rxros_tf package and split the rxros.h file up accordingly.

Ok. If it really doesn't work for you, or others, we can always reintegrate everything.

I may not like it, but I may get a pleasant surprise.

Let's hope so.

Please let me know if I can support you in any way.

If you could take a look at #14 and rosin-project/rxros_examples#1 and tell me whether things build and run for you that would be great. We can then proceed to merge it and would have a good basis for further changes.

gavanderhoorn commented 5 years ago

24 refactors rxros such that the TF dependency is pushed into a separate package, together with all TF related functionality.

@wasowski: if you could take a look.

gavanderhoorn commented 5 years ago

Just tested installing rxros and rxros_tf in a clean Melodic Docker container.

I believe the following provides a good justification for why we split the TF support out of the base rxros package.

  1. installing just rxros:

    root@16da4763121c:/# apt install ros-melodic-rxros
    Reading package lists... Done
    Building dependency tree
    Reading state information... Done
    The following additional packages will be installed:
      ros-melodic-rxcpp-vendor
    The following NEW packages will be installed:
      ros-melodic-rxcpp-vendor ros-melodic-rxros
    0 upgraded, 2 newly installed, 0 to remove and 91 not upgraded.
    Need to get 94.3 kB of archives.
    After this operation, 998 kB of additional disk space will be used.
  2. installing rxros_tf (after having installed rxros itself previously):

    root@94eb33a6df9d:/# apt install ros-melodic-rxros-tf
    Reading package lists... Done
    Building dependency tree
    Reading state information... Done
    The following additional packages will be installed:
      fonts-liberation graphviz libann0 libcdt5 libcgraph6 libgd3 libgts-0.7-5 libgts-bin libgvc6 libgvpr2 liblab-gamut1
      libpathplan4 libwebp6 libxaw7 libxmu6 libxpm4 libxt6 ros-melodic-tf ros-melodic-tf2 ros-melodic-tf2-msgs
      ros-melodic-tf2-py ros-melodic-tf2-ros
    Suggested packages:
      gsfonts graphviz-doc libgd-tools
    The following NEW packages will be installed:
      fonts-liberation graphviz libann0 libcdt5 libcgraph6 libgd3 libgts-0.7-5 libgts-bin libgvc6 libgvpr2 liblab-gamut1
      libpathplan4 libwebp6 libxaw7 libxmu6 libxpm4 libxt6 ros-melodic-rxros-tf ros-melodic-tf ros-melodic-tf2
      ros-melodic-tf2-msgs ros-melodic-tf2-py ros-melodic-tf2-ros
    0 upgraded, 23 newly installed, 0 to remove and 91 not upgraded.
    Need to get 4007 kB of archives.
    After this operation, 16.6 MB of additional disk space will be used.

Note the 16MB vs ~1MB installation size difference.