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

removed dependency to ros1_bridge package #90

Merged

Conversation

Projects
None yet
4 participants
@DensoADAS
Copy link
Contributor

commented Feb 12, 2019

fixes #85

removed dependency to ros1_bridge package:

  • checking if package is available
  • if not skipping (with warnings)
  • now rosbag2_tests builds on systems without ros1
Joshua Hampp
removed dependency to ros1_bridge package:
 * checking if package is available
 * if not skipping (with warnings)
 * now rosbag2_tests builds on systems without ros1

@tfoote tfoote added the in review label Feb 12, 2019

@Karsten1987

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

@DensoADAS thanks for the patch. I hope you don't mind me editing your original description for this PR. I connected it to the open issue so that our waffle board picks it up correctly and automatically closes the referenced issue when this PR is merged.

extremely helpful patch. Running CI on it:

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

I believe the cmake warning has to be turned intoSTATUS in order make CI green.

@Karsten1987

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

new round of CI, disabling ros1_rosbag_storage_vendor rosbag2_bag_v2_plugins

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

that being said, I would really appreciate if we could use this PR to extend the same logic to the two excluded packages so that this complete repo can actually be built without any ros1 dependency.

@Karsten1987

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

Looking at the latest CI, this patch is not sufficient. Looks like we should patch the two packages in question as well in . order to run CI correctly.

Show resolved Hide resolved rosbag2_tests/CMakeLists.txt Outdated
Show resolved Hide resolved rosbag2_tests/CMakeLists.txt Outdated

Karsten1987 and others added some commits Feb 14, 2019

Merge pull request #1 from ros2/export_cmake_file
check ros1 deps correctly on all packages
@Karsten1987

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2019

message(WARNING "Failed to find PkgConfig, skipping...")
# include bridge first to not create package if ros1 packages cannot be found
find_package(ros1_bridge QUIET)
if (NOT ros1_bridge_FOUND)

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Feb 22, 2019

Contributor
Suggested change
if (NOT ros1_bridge_FOUND)
if(NOT ros1_bridge_FOUND)
@Karsten1987
Copy link
Contributor

left a comment

there is one linter error left in the patch. If you'd accept the suggestion we should be good to go.

Sorry for the long turnover time here, I struggled to set up a suitable CI job for this. Here it is:
Build Status

@Karsten1987 Karsten1987 merged commit 05b9c4c into ros2:master Mar 25, 2019

1 check failed

Cpr__rosbag2__ubuntu_bionic_amd64 Build finished.
Details

@Karsten1987 Karsten1987 removed the in review label Mar 25, 2019

Karsten1987 added a commit that referenced this pull request Apr 8, 2019

[backport] ros1 dependency handling (#98)
* removed dependency to ros1_bridge package (#90)

* removed dependency to ros1_bridge package:
 * checking if package is available
 * if not skipping (with warnings)
 * now rosbag2_tests builds on systems without ros1

* check ros1 deps correctly on all packages

* add ros1_bridge to test package

* silently try to find the bridge

* correct missing linter errors (#96)

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.