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 dependencies in tf2_ros. #269

Merged
merged 1 commit into from
Jun 17, 2020
Merged

Conversation

clalancette
Copy link
Contributor

This will allow overlays on it to work properly.

Signed-off-by: Chris Lalancette [email protected]
Signed-off-by: Dirk Thomas [email protected]

This should fix #268 .

Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM

The testing failing in the PR job is an existing flake.

@dirk-thomas
Copy link
Member

Can you clarify what the diff between this PR and the previous branch clalancette/overlay-include-problem is (and why there was a need to create a new separate one)?

@clalancette
Copy link
Contributor Author

Can you clarify what the diff between this PR and the previous branch clalancette/overlay-include-problem is (and why there was a need to create a new separate one)?

Two main reasons:

  1. The clalancette/overlay-include-problem branch has some nonsense code in it that we don't want to merge; it is just there to show the problem.
  2. The changes on that branch to all of the cpp/hpp files to add include headers as appropriate are technically more correct, but not really necessary. Given how late we are in Foxy release cycle, this was the minimum amount of change we needed to solve this particular problem. I'll follow up with the include header changes in a separate PR that we can merge later.

@clalancette
Copy link
Contributor Author

Here's CI:

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

@clalancette
Copy link
Contributor Author

The compilation warnings on macOS are expected; they show up on nightlies as well.

The compilation warnings on Windows are also expected; they show up on nightlies as well.

What is not expected is the test failures on Windows. The tests that failed looked like they immediately segfaulted on startup, and we do not see that on the nightlies. That needs more investigation before we merge this.

@jacobperron
Copy link
Member

I can reproduce the RViz test failures locally on Windows. Looks like a bunch of "Entry Point Not Found" errors, for example if I run:

C:/Users/jacob/dev/ws/ros/geometry2/build/rviz_common/Release/frame_manager_test.exe

I get a dialog box containing the following error:

The procedure entry point ?hitButton@QPushButton@@MEBA_NAEBVQPoint@@@Z could not be located in the dynamic link library C:\Users\jacob\dev\ws\ros\geometry2\build\rviz_common\Release\rviz_common.dll

I don't really know why this change is causing the tests in RViz to fail. We also see the same test failures happening in Windows CI for #270 (which is an orthogonal change). Maybe it's a difference in our CI job and the nightly?

@clalancette clalancette force-pushed the clalancette/tf2-ros-fix-overlays branch from 356aec5 to f498434 Compare June 9, 2020 20:57
@clalancette clalancette force-pushed the clalancette/tf2-ros-fix-overlays branch from f498434 to c73b6ba Compare June 17, 2020 15:14
This will allow overlays on it to work properly.

Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Dirk Thomas <[email protected]>
@clalancette clalancette force-pushed the clalancette/tf2-ros-fix-overlays branch from c73b6ba to 3131930 Compare June 17, 2020 15:24
@clalancette
Copy link
Contributor Author

clalancette commented Jun 17, 2020

I've now rebased this onto the latest. I think that the rviz failures are unrelated, as we are seeing them on all rviz PRs that go through CI (but not on nightlies). So those will have to be investigated separately.

I'm going to run CI one more time on this change, just to make sure everything else is good. Given that there have been no substantial changes since the approvals, I'm going to take those as still valid, and will merge assuming CI comes back OK.

CI:

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

@clalancette clalancette merged commit b50439a into ros2 Jun 17, 2020
@clalancette clalancette deleted the clalancette/tf2-ros-fix-overlays branch June 17, 2020 18:49
jacobperron pushed a commit that referenced this pull request Jul 22, 2020
This will allow overlays on it to work properly.

Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Dirk Thomas <[email protected]>
jacobperron added a commit that referenced this pull request Aug 3, 2020
This will allow overlays on it to work properly.

Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Dirk Thomas <[email protected]>

Co-authored-by: Chris Lalancette <[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.

Foxy: Cannot overlay tf2_ros over existing tf2_ros
3 participants