rosin-project / rxros

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

Migrate examples from rxros repository (and other suggestions) #14

Closed gavanderhoorn closed 5 years ago

gavanderhoorn commented 5 years ago

As per subject.

This PR suggests a new repository layout, in which all examples are moved to the rxros_examples repository (rosin-project/rxros_examples#1), rxros is the only package in this repository and a new dependency is added: rxcpp_vendor (which packages in the RxCpp dependency).

Some other changes:

To test this new setup, do the following:

source /opt/ros/$ROS_DISTRO/setup.bash
rosdep update
mkdir -p $HOME/rxros_refactor_ws
cd $HOME/rxros_refactor_ws
wstool init src https://raw.githubusercontent.com/gavanderhoorn/rxros_examples/refactor/rxros_examples.rosinstall
rosdep install --from-paths $HOME/rxros_refactor_ws/src --ignore-src
catkin_make  # or if you prefer: catkin build
source devel/setup.bash

This should build rxcpp_vendor, rxros, the BrickPi and the teleop packages/examples.

After sourceing the workspace you should be able to run all of the examples.

Note: to initialise a workspace with just rxros (so none of the examples), use this .rosinstall file instead of the one shown in the listing above.

gavanderhoorn commented 5 years ago

If/when we agree to merge this, I can add CI (based on Travis CI) and setup some (regression) testing if that would be desirable.

gavanderhoorn commented 5 years ago

@henrik7264: I've left the tf dependency in rxros (so no rxros_tf yet), but at least made it explicit in 8e883cb8.

rosdep should now install it on any system that doesn't already have it installed (there probably aren't many, but this way we can be sure).

wasowski commented 5 years ago

I'll look at this tomorrow evening (will try to install and test drive).

wasowski commented 5 years ago

Testing will be desirable :), I was planning to write some tests, when I have time. So a setup for getting them to run will be needed.

gavanderhoorn commented 5 years ago

Testing will be desirable :), I was planning to write some tests, when I have time. So a setup for getting them to run will be needed.

Just having a will-it-compile test is already very valuable in my experience.

wasowski commented 5 years ago

I have managed to run your commands and build successfully in a clean container with ros melodic. There are some very minor problems with your instructions above, but this does not matter much (they are only for use in this PR).

I have also read through all the commits. I also have some trouble understanding the bottom one (about installation of include files), but this is simply caused by lack of background. It does look reasonable.

I noticed that you did not split away the tf stuff, and I think this is the right thing to do. We should do this in a separate PR. Otherwise it is too hard to discuss and too hard to understand what is going on.

I suggest that we merge this (after @henrik7264 gives as a second opinion).

gavanderhoorn commented 5 years ago

@wasowski wrote:

There are some very minor problems with your instructions above, but this does not matter much (they are only for use in this PR).

could you describe those problems? They may be important to fix before attempting to release the package.

I noticed that you did not split away the tf stuff, and I think this is the right thing to do. We should do this in a separate PR. Otherwise it is too hard to discuss and too hard to understand what is going on.

Yes, I didn't want to conflate the different issues.

This PR is about restructuring the repository and splitting of the examples. Nothing more.


Edit: I believe I've fixed on of those 'minor problems' (the path to the workspace in the rosdep invocation).

henrik7264 commented 5 years ago

Worked fine on my machine (ros-melodic-desktop-full). Installation as described above worked fine and the catkin_build was sucessfull. Ran a "rospack list" to verify that the packages were available - all OK.

wasowski commented 4 years ago

@gavanderhoorn of course, I wen underground for a month, and no longer remember what the issues were. You probably already fixed what needed to be fixed. I consider this case to be closed :)

gavanderhoorn commented 4 years ago

@wasowski: yes, I believe we've covered everything here.