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

"Futurize" foxy-future branch #687

Merged
merged 6 commits into from
Jun 18, 2021
Merged

Conversation

emersonknapp
Copy link
Collaborator

@emersonknapp emersonknapp commented Mar 19, 2021

Resolves #777
Part of #657

Notes:

  • Some copy-pasted code from the Galactic core that is not present in Foxy for the utilities in here.
  • Some minor API tweaks for things that changed since Foxy
  • Add information to the README about this special branch
  • Add a notice to the ros2bag CLI that informs you that you're using this version

See https://discourse.ros.org/t/fast-forward-merging-rosbag2-master-api-to-foxy/18927/38 for more context

@emersonknapp emersonknapp requested a review from a team as a code owner March 19, 2021 19:39
@emersonknapp emersonknapp requested review from mjeronimo and Karsten1987 and removed request for a team March 19, 2021 19:39
@emersonknapp emersonknapp changed the base branch from master to foxy-future March 19, 2021 19:39
@emersonknapp emersonknapp marked this pull request as draft March 22, 2021 21:46
@emersonknapp
Copy link
Collaborator Author

It builds all the way through right now, but still having test failures - I think mostly related to plugins

@emersonknapp emersonknapp marked this pull request as ready for review March 23, 2021 00:27
Copy link
Contributor

@zmichaels11 zmichaels11 left a comment

Choose a reason for hiding this comment

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

LGTM with green CI.

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/fast-forward-merging-rosbag2-master-api-to-foxy/18927/43

@hexonxons
Copy link
Contributor

hexonxons commented Mar 28, 2021

Change all package names to *_future and add <conflict> tag for original package name

Won't <conflict> tag brake packages which depends on rosbag2 already? Like

root@df084f762237:/# apt-cache depends ros-foxy-ros-base
ros-foxy-ros-base
  Depends: ros-foxy-geometry2
  Depends: ros-foxy-kdl-parser
  Depends: ros-foxy-robot-state-publisher
  Depends: ros-foxy-ros-core
  Depends: ros-foxy-rosbag2
  Depends: ros-foxy-urdf
  Depends: ros-foxy-ros-workspace

@JWhitleyWork
Copy link

Any updates to this? There are lots of requests for this to be available in Foxy and there is a sync coming up tomorrow (I know we won't make this).

@jfinken
Copy link

jfinken commented May 5, 2021

End-user questions if I may.

  • I believe I am directly impacted by this (dropped messages in Foxy rosbags)
  • I have no scripts written around ros2 bag *
  • I have no code written around a current or future rosbag2 API

So, is it is possible for me to just run the emersonknapp/futurize-foxy branch? E.g. ros2 bag-future *? If so, how? I've built the branch but am unclear how to cleanly run the (new?) verb via the ros2cli.

I've built ROS2-foxy from source, could I set aside /opt/ros/foxy/install/rosbag2* and/or symlink in the shared libs from my dedicated rosbag2-future overlay workspace?

Thanks in advance.

@emersonknapp
Copy link
Collaborator Author

This branch currently still registers the verb as ros2 bag. I'm not 100% certain, but i think the last-loaded one takes priority, so your overlay workspace should be the thing that gets called after you build and source it.

@emersonknapp
Copy link
Collaborator Author

I'll be taking another pass at this in the next week, to make it an officially supported Foxy-compatible source branch for users to build if they want more improvements than can be backported.

@emersonknapp
Copy link
Collaborator Author

Hello rosbag2 developers! I have updated the description, and this branch - to the final effect that we discussed. I could use a re-review so we can put the official branch in front of people to use for their Foxy projects.

@Karsten1987 @mjeronimo @MichaelOrlov @jhdcs @zmichaels11 much obliged for any eyes on this!

Copy link
Contributor

@jhdcs jhdcs left a comment

Choose a reason for hiding this comment

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

Looks good to me. Only some style issues that I could notice.

@emersonknapp
Copy link
Collaborator Author

@jhdcs thanks for being thorough - but these are literal copy-pastes for compatibility, from C code - introduced to RMW in ros2/rmw#301. I won't be modifying them for style

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
ros2bag/ros2bag/command/bag.py Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/qos.cpp Outdated Show resolved Hide resolved
@Karsten1987
Copy link
Collaborator

Can you trigger a full Foxy CI on the future branch?

rosbag2_transport/src/rosbag2_transport/qos.cpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/qos.cpp Outdated Show resolved Hide resolved
emersonknapp and others added 5 commits June 15, 2021 17:41
…tool

Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
* 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
Copy link
Collaborator Author

OK - I've set up the Action CI to build with rosbag2 Foxy in the global underlay. You can see the rosbag2_py failure there now in build_and_test - this will help us validate the CMake fixes before sending off to the Jenkins farm.

The last remaining problem, as discussed on #780 is that if ros-foxy-pybind11-vendor is installed, then rosbag2_py picks up the /opt/ros/foxy/include directories first.

@emersonknapp
Copy link
Collaborator Author

@hexonxons do you have any ideas about the pybind11-vendor issue, none of my attempts (so far) has yielded a solution

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/fast-forward-merging-rosbag2-master-api-to-foxy/18927/48

@hexonxons
Copy link
Contributor

hexonxons commented Jun 17, 2021

@emersonknapp
Seems fixed in #783, could you check it please?

Signed-off-by: Aleksandr Rozhdestvenskii <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Jun 17, 2021

Action CI green and ready to rumble! Thanks @hexonxons very much for the help

Running Foxy CI now (@Karsten1987 did you have any specific arguments in mind other than the ones I did here?)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Jun 17, 2021

Retry, had to add pybind11_vendor to the .repos list, since it's not part of the default Foxy list

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@emersonknapp
Copy link
Collaborator Author

Windows CMake warnings are not a concern - one is unrelated and the other is harmless. Since this is not build on ci.ros2.org, it'll only be witnessed as a single CMake warning by people building this branch on Windows.

@emersonknapp emersonknapp merged commit b7c3b21 into foxy-future Jun 18, 2021
@delete-merged-branch delete-merged-branch bot deleted the emersonknapp/futurize-foxy branch June 18, 2021 18:25
@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2021-06-17/21017/1

@JWhitleyWork
Copy link

Does this now require a PR to ros/rosdistro to release it as rosbag2-future?

@emersonknapp
Copy link
Collaborator Author

No. This branch will not be released as a binary, you need to build it from source if you want to use it. See the README for more information.

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.

9 participants