-
Notifications
You must be signed in to change notification settings - Fork 203
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
Conversation
There was a problem hiding this 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.
Can you clarify what the diff between this PR and the previous branch |
Two main reasons:
|
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. |
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:
I get a dialog box containing the following error:
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? |
356aec5
to
f498434
Compare
f498434
to
c73b6ba
Compare
This will allow overlays on it to work properly. Signed-off-by: Chris Lalancette <[email protected]> Signed-off-by: Dirk Thomas <[email protected]>
c73b6ba
to
3131930
Compare
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: |
This will allow overlays on it to work properly. Signed-off-by: Chris Lalancette <[email protected]> Signed-off-by: Dirk Thomas <[email protected]>
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]>
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 .