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.

Sign up for GitHub

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

Jump to bottom

Image tools tests redux #67

Merged
merged 11 commits into from
Sep 7, 2016
Merged

Image tools tests redux #67

merged 11 commits into from
Sep 7, 2016

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented May 25, 2016

replacement for #48 that does not use a movie file, but generates burgers instead (thanks @codebot).

screen shot 2016-05-24 at 8 25 42 pm

@wjwwood wjwwood added the in progress Actively being worked on (Kanban column) label May 25, 2016
@wjwwood
Copy link
Member Author

wjwwood commented May 25, 2016 •
edited
Loading

CI:

  • Linux:
    • Build Status
  • OS X:
    • Build Status
  • Windows:
    • Build Status

@codebot
Copy link
Member

codebot commented May 25, 2016

If you can't have a camera or a movie file, at least you can have flying burgers. 🍔 🍔 🍔

@wjwwood
Copy link
Member Author

wjwwood commented May 25, 2016

Btw, this new test is disabled for connext, connext dynamic, and fastrtps for various known issues. I put a comment in the cmake which disables it.

@wjwwood
Copy link
Member Author

wjwwood commented May 26, 2016

Ok, it's broken on Windows and OS X. OS X is a problem with how OpenCV is linked in Homebrew and I don't know how to fix it generically. Windows is a mystery too. It's got something to do with how I'm using file(GENERATE ...). I'm going to put this on hold until after the alpha. I've wasted way too much time on it. If someone else wants to pick it up before then, that'd be great.

@wjwwood wjwwood added ready Work is about to start (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jun 13, 2016
@wjwwood wjwwood force-pushed the image_tools_tests_redux branch from b8d533f to c90df87 Compare June 24, 2016 01:46
@wjwwood wjwwood mentioned this pull request Jun 25, 2016
Closed
@codebot codebot force-pushed the image_tools_tests_redux branch from 9552290 to 07833bb Compare July 1, 2016 21:12
@codebot codebot force-pushed the image_tools_tests_redux branch from 07833bb to 1e92fee Compare July 1, 2016 21:15
@wjwwood
Copy link
Member Author

wjwwood commented Sep 7, 2016

CI:

  • Linux:
    • Build Status
  • OS X:
    • Build Status
  • Windows:
    • Build Status

@wjwwood wjwwood merged commit 16c3bc5 into master Sep 7, 2016
@tfoote tfoote removed the ready Work is about to start (Kanban column) label Sep 7, 2016
@wjwwood wjwwood deleted the image_tools_tests_redux branch September 7, 2016 23:13
@codebot
Copy link
Member

codebot commented Sep 7, 2016

🐔 👍

dirk-thomas
dirk-thomas reviewed Sep 14, 2016
View reviewed changes
image_tools/CMakeLists.txt
"${rmw_implementation} " STREQUAL "rmw_fastrtps_cpp "
)
message(STATUS "Skipping tests for '${rmw_implementation}'")
return()
Copy link
Member

@dirk-thomas dirk-thomas Sep 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I assume the purpose of this return is to skip the current rmw implementation (so jump out of the macro). But return doesn't work like that in a macro. Please see https://cmake.org/cmake/help/v3.5/command/return.html

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be resolved in #78.

@dirk-thomas
Copy link
Member

It also looks like this change broke the Windows build when using OpenSplice: see http://ci.ros2.org/job/ci_windows/1650/console

@wjwwood
Copy link
Member Author

wjwwood commented Sep 14, 2016

I'll look at it, but the CI jobs don't test OpenSplice any longer.

@dirk-thomas dirk-thomas mentioned this pull request Sep 14, 2016
Closed
@dirk-thomas
Copy link
Member

I created a ticket for it: #79

@dirk-thomas dirk-thomas mentioned this pull request Dec 20, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirk-thomas dirk-thomas dirk-thomas left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wjwwood @codebot @dirk-thomas @tfoote