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

Collapse testing back into pluginlib package. #134

Merged
merged 9 commits into from
Dec 11, 2018
Merged

Conversation

mjcarroll
Copy link
Member

@mjcarroll mjcarroll commented Dec 10, 2018

Merges testing back into the pluginlib package.

Depends on ros2/tinyxml2_vendor#3

@mjcarroll
Copy link
Member Author

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

@mjcarroll
Copy link
Member Author

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

@mjcarroll
Copy link
Member Author

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

@mjcarroll
Copy link
Member Author

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

pluginlib/CMakeLists.txt Outdated Show resolved Hide resolved
pluginlib/test/package.xml Outdated Show resolved Hide resolved
pluginlib/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Member

@nuclearsandwich nuclearsandwich 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 on Xenial and the pluginlib tests ran without issue.

I am somewhat concerned that the fixture package.xml could be picked up by some tools (not colcon, clearly as the build did not fail) as part of a valid package all its own. Since the file is copied out during the test phase this could be mitigated by naming the source file something like fixture_package.xml instead.

I liked the acceptance tests when they were introduced as a top-level package in #130 but I'm not sure if it's worth the CMake hoops here to have them.

@dirk-thomas
Copy link
Member

I am somewhat concerned that the fixture package.xml could be picked up by some tools

The spec forbids nesting of packages so that shouldn't be a problem with compliant tools.

@nuclearsandwich
Copy link
Member

The spec forbids nesting of packages so that shouldn't be a problem with compliant tools.

No more find -name package.xml for me 😭

@mjcarroll
Copy link
Member Author

I will do as you suggest and change the name, so it won't be picked up by simple searches in the source space, at least, but it's good to know that it shouldn't choke any of the tooling.

@mjcarroll
Copy link
Member Author

Nevermind on renaming, because CMake doesn't support it on file(COPY or file(INSTALL, it will remain package.xml.

New CI:

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

@dirk-thomas
Copy link
Member

CMake doesn't support it on file(COPY or file(INSTALL, it will remain package.xml.

configure_file (... COPY_ONLY) might be an option.

@mjcarroll
Copy link
Member Author

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

@mjcarroll
Copy link
Member Author

Just realized I had some vestigial debug output that needed to be removed.

Copy link
Member

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

👍 🙇‍♂️

@nuclearsandwich
Copy link
Member

This needs ros2/tinyxml2_vendor#3 before merging

@mjcarroll mjcarroll merged commit fd77f26 into ros2 Dec 11, 2018
@mjcarroll mjcarroll deleted the collapse_testing branch December 11, 2018 17:51
rcutils
TinyXML2
)
target_link_libraries(${PROJECT_NAME}_utest ${TinyXML2_LIBRARIES})
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, this was an accident. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants