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

Fix/futurize foxy dependency fixes #780

Merged
merged 5 commits into from
Jun 16, 2021
Merged

Fix/futurize foxy dependency fixes #780

merged 5 commits into from
Jun 16, 2021

Conversation

hexonxons
Copy link
Contributor

@hexonxons hexonxons commented Jun 12, 2021

Fix #698

Needs to be refactored after ros2/ros2#1150

How to check if everything goes "OK":

  • Build via colcon build --ament-cmake-args -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_ROSBAG2_BENCHMARKS=ON --cmake-args -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_ROSBAG2_BENCHMARKS=ON
  • Open build/compile_commands.json
  • Test against regex -((I)|(isystem))\s*/opt(?!((/ros/foxy/src/gmock)|(/ros/foxy/src/gtest))).*-((I)|(isystem))\s*/home, where /home - first part of absolute path to local workspace

@hexonxons hexonxons requested a review from a team as a code owner June 12, 2021 00:07
@hexonxons hexonxons requested review from Karsten1987 and zmichaels11 and removed request for a team June 12, 2021 00:07
@emersonknapp
Copy link
Collaborator

When I tried to build this branch with the following sequence,

apt-get install ros-foxy-ros-base
source /opt/ros/foxy/setup.bash
colcon build --packages-up-to rosbag2

I still get the problem of reading globally installed dependencies before the local ones:

--- stderr: rosbag2_py
/ws/src/rosbag2/rosbag2_py/src/rosbag2_py/_reader.cpp: In member function ‘void rosbag2_py::Reader<T>::open(rosbag2_storage::StorageOptions&, rosbag2_cpp::ConverterOptions&)’:
/ws/src/rosbag2/rosbag2_py/src/rosbag2_py/_reader.cpp:47:19: error: cannot convert ‘rosbag2_storage::StorageOptions’ to ‘const rosbag2_cpp::StorageOptions&’
   47 |     reader_->open(storage_options, converter_options);
      |                   ^~~~~~~~~~~~~~~
      |                   |
      |                   rosbag2_storage::StorageOptions
In file included from /ws/src/rosbag2/rosbag2_py/src/rosbag2_py/_reader.cpp:24:
/opt/ros/foxy/include/rosbag2_cpp/reader.hpp:70:36: note:   initializing argument 1 of ‘void rosbag2_cpp::Reader::open(const rosbag2_cpp::StorageOptions&, const rosbag2_cpp::ConverterOptions&)’
   70 |   void open(const StorageOptions & storage_options, const ConverterOptions & converter_options);
      |             ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
/ws/src/rosbag2/rosbag2_py/src/rosbag2_py/_reader.cpp: In function ‘std::unordered_set<std::__cxx11::basic_string<char> > rosbag2_py::get_registered_readers()’:

Note /opt/ros/foxy header for the mismatched function declaration.

Am I missing something?

@emersonknapp emersonknapp self-requested a review June 15, 2021 00:09
@hexonxons
Copy link
Contributor Author

hexonxons commented Jun 15, 2021

@emersonknapp

Hmm, I can't reproduce on host Ubuntu 20.04 via

mkdir -p ws/src
git clone -b fix/futurize-foxy-dependency-fixes [email protected]:hexonxons/rosbag2.git ws/src/rosbag2
cd ws
source /opt/ros/foxy/setup.bash
colcon build --packages-up-to rosbag2

Also in container:

docker pull ros:foxy
mkdir -p ws/src
git clone -b fix/futurize-foxy-dependency-fixes [email protected]:hexonxons/rosbag2.git ws/src/rosbag2
docker run --pull --rm -it -v $(pwd)/ws:/ws --workdir="/ws" ros:foxy /bin/bash

# Inside container
# Not really needed. ros-foxy-ros-base is already installed
apt update && apt install -y ros-foxy-ros-base
apt update && apt install -y ros-foxy-test-msgs pybind11-dev
source /opt/ros/foxy/setup.bash
colcon build --packages-up-to rosbag2
colcon test --packages-up-to rosbag2

Everything seems okay

Did you checkout fix/futurize-foxy-dependency-fixes branch from my forked repo? Could you post git remote show origin output?

@emersonknapp
Copy link
Collaborator

It seems that I ran into the problem because I installed ros-foxy-pybind11-vendor instead of pybind11-dev. rosbag2_py depends on pybind11_vendor, and:

# rosdep resolve pybind11_vendor
#apt
ros-foxy-pybind11-vendor

I'll merge this into the main PR however as it is clearly much closer to the end goal

@emersonknapp emersonknapp merged commit cfbf717 into ros2:emersonknapp/futurize-foxy Jun 16, 2021
@emersonknapp
Copy link
Collaborator

I'll try to add an automated test case that builds while rosbag2 is installed in the underlay from apt - and we can try to figure out this one remaining issue. A user who does rosdep install --from-paths will hit this problem. It seems that if ros-foxy-pybind11-vendor is installed at all, then the rosbag2_py build picks up the /opt/ros/foxy/include path first.

emersonknapp pushed a commit that referenced this pull request Jun 16, 2021
* Kludge: add transitive dependencies for rosbag2_cpp
* Kludge: add transitive dependencies for rosbag2_compression_zstd
* Kludge: add transitive dependencies for rosbag2_compression
* Kludge: add transitive dependencies for rosbag2_transport
* Kludge: add transitive dependencies for rosbag2_tests

Signed-off-by: Aleksandr Rozhdestvenskii <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
emersonknapp added a commit that referenced this pull request Jun 18, 2021
* Fix/futurize build against Foxy (#780)
* Kludge: add transitive dependencies for rosbag2_cpp
* Kludge: add transitive dependencies for rosbag2_compression_zstd
* Kludge: add transitive dependencies for rosbag2_compression
* Kludge: add transitive dependencies for rosbag2_transport
* Kludge: add transitive dependencies for rosbag2_tests
* Action CI build as overlay on Foxy rosbag2 binaries
* Kludge: fix pybind include path for rosbag2_py (#783)

Signed-off-by: Emerson Knapp <[email protected]>
Co-authored-by: Alexander <[email protected]>
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.

2 participants