Skip to content

Fix failing test on rolling#416

Merged
bmagyar merged 4 commits intoros-controls:masterfrom
vatanaksoytezer:pr-transmission_add_macro
May 18, 2021
Merged

Fix failing test on rolling#416
bmagyar merged 4 commits intoros-controls:masterfrom
vatanaksoytezer:pr-transmission_add_macro

Conversation

@vatanaksoytezer
Copy link
Copy Markdown
Contributor

Fixes the failin rolling test with a specfic macro for MacOS devices. See #406 and #390 .

This will allow ros2_control to compile on rolling

Copy link
Copy Markdown
Member

@matthew-reynolds matthew-reynolds left a comment

Choose a reason for hiding this comment

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

See #406 (comment) for a potential way to avoid duplication of the whole test suite instantiation.

@vatanaksoytezer
Copy link
Copy Markdown
Contributor Author

Thanks @matthew-reynolds I think this definitely looks way better! @bmagyar @Karsten1987 I think this should be ready to merge.

Copy link
Copy Markdown
Contributor

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

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

I tested this locally on rolling and it works for me.

@bmagyar
Copy link
Copy Markdown
Member

bmagyar commented May 18, 2021

Thanks guys!

@bmagyar bmagyar merged commit 4fdfe1d into ros-controls:master May 18, 2021
@Karsten1987
Copy link
Copy Markdown
Contributor

Did anybody test this on OSX? This still fails for the exact same reason locally for me as before.

@tylerjw
Copy link
Copy Markdown
Contributor

tylerjw commented May 21, 2021

Did anybody test this on OSX? This still fails for the exact same reason locally for me as before.

😢 I don't think any of us have an OSX setup to test on. We had assumed this would work because the #define should work the same way it did before on osx. Is APPLE not the right name to test for?

@Karsten1987
Copy link
Copy Markdown
Contributor

The problem is the , in the preprocessor directive. I am fairly sure this doesn't work as intended and shouldn't work on gcc either. That's what makes me believe that nobody actually tested this - independently from the OS.

@vatanaksoytezer
Copy link
Copy Markdown
Contributor Author

I think a good way to test it would be setup a CI for OSX. I have no way of testing this but I assumed the APPLE macro would have worked. If you have a solution to get it working both on OSX and Linux, I think it that would be more valuable since we cannot test this.

@Karsten1987
Copy link
Copy Markdown
Contributor

Also, come to think about this. I don't think dividing it via OS is the right approach here in the first place. The issue is that macros are defined differently depending on the gtest version. That gtest version might get updated on OSX as well in future ROS versions. So we'd have to address this if/else once again.
We should switch this depending on the ROS distro. If foxy then, ...

@bmagyar
Copy link
Copy Markdown
Member

bmagyar commented May 21, 2021

@Karsten1987 I think OSX was dropped to Tier3 from Galactic & onwards

You just said that the gtest version may be different on osx even in foxy, why not try an if based on the gtest version? Worst case we can inject it via the cmake on compilation

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.

5 participants