Up poco version to 1.8.0.1

[sloretz]

Edit: This PR ups the version of poco to 1.8.0.1 (same as in bionic). It also removes the custom find module by instead installing poco into the same install prefix as poco_vendor.

Old description

More info in this comment.

connects to #11

Running windows packaging job so I can use the artifact to test this patch. I've checked it on Ubuntu and did not see a regression.

[mikaelarguedas]

lgtm, once the artifact has proven to fix the pb

[mikaelarguedas]

Alternatively we could use all caps for consistency https://cmake.org/cmake/help/v3.11/variable/CMAKE_DEBUG_POSTFIX.html.
I expect both to have the same effect.

Though I'm fine with the current one if we prefer matching the case of the string representing the build type rather than following a strict "all caps for all CMake variables".

[sloretz]

The debug build produced PocoFoundation.dll on the windows packaging job. Maybe all-caps is mandatory.

[sloretz]

Another windows packaging job

[sloretz]

Yet another packaging job

Review was conditional on testing outcome. It didn't work and there have been new changes since then.

[sloretz]

Trying another approach

[sloretz]

Getting closer

[sloretz]

The lastest build artifact fixes the issue. I've tested building Debug of ros2/examples in an overlay workspace an confirmed it still works on Ubuntu and now Windows using the artifact from the last CI packaging job.

The fix overwrites the build and install commands to pass the build type via --config. This should make no difference on ubuntu/osx but is required when using visual studio.

CI build type Debug

• Linux
  • False positive cmake warning, see Cmake warning about unused variable SECURITY build_farmer#79
• Linux-aarch64
• macOS
• Windows
  • 9 test failures, waiting for nightly job to see if they're exclusive to this branch

[sloretz]

CI Windows build type None

[sloretz]

Re-Rebuild CI windows

[sloretz]

Trying a job with a more recent version of poco: 1.8.0.1

Windows packaging job

[sloretz]

Windows packaging job

[sloretz]

[sloretz]

CI build type None

• Linux
• Linux-aarch64
• macOS
• Windows

[mikaelarguedas]

additional context available at ros2/ci#148 (comment)

[sloretz]

With this PR I don't see the issue described in #11 in windows

However, I have to explicitly use CMake commands to test a debug build on windows. The default generator used when I run colcon build does not make use of the CMAKE_BUILD_TYPE variable.

To test on windows

1. clone ros2/examples
2. mkdir some_folder && cd some_folder
3. cmake -G "Visual Studio 14 2015 Win64" ..\src\examples\rclcpp\minimal_composition\"
4. cmake --build . --config Debug

[mikaelarguedas]

The default generator used when I run colcon build does not make use of the CMAKE_BUILD_TYPE variable.

This looks like a bug.
Can you provide the colcon invocation you ran that didnt work ?
Could it be an issue when escaping the build type in the colcon invocation ?

I guess colcon is using the VS 2017 generator by default?

[mikaelarguedas]

Following up from offline discussion with @sloretz:

• the patch needs to be modified to keep the check for the version of poco we find and build anyway if the version is too old: https://github.com/ros2/poco_vendor/pull/10/files

• the cmake generator error seems tied to the environment that was used for testing (a VS2015 VM), we should check if we can reproduce in our Windows node in VS2017 terminal and optionally in a VS2015 terminal as well

• We should test the updated patch on a Xenial system to see if Poco is downloaded and built properly and confirm that an overlay workspace building in Debug still works

[sloretz]

I tested b70d08e manually for each of these cases and did not see the issue described by #11

• bionic with libpoco-dev installed
• bionic without libpoco-dev installed
• xenial without libpoco-dev installed

Test steps

cd ros2_ws
rm -rf build install
colcon build --cmake-args " -DCMAKE_BUILD_TYPE=RelWithDebInfo"
cd ..
mkdir overlay_ws
git clone https://github.com/ros2/examples.git
colcon build
./install/examples_rclcpp_minimal_composition/lib/examples_rclcpp_minimal_composition/composition_composed
rm -rf build install
colcon build --cmake-args " -DCMAKE_BUILD_TYPE=Debug"
./install/examples_rclcpp_minimal_composition/lib/examples_rclcpp_minimal_composition/composition_composed

Here's another windows packaging job . When that finishes I'll test the above steps on it
Edit: seems to work on windows, putting this ticket into review

CI

• Linux
• Linux-aarch64
• macOS
• Windows

[mikaelarguedas]

LGTM, with 2 nitpicks/questions

Can we use CMAKE_CXX_STANDARD instead of passing -std=c++14 explicitly to cmake ?

[mikaelarguedas]

Is this necessary ? It looks like the default is NEW in CMake 3.5: https://cmake.org/cmake/help/v3.5/policy/CMP0048.html

[sloretz]

fc22ae0

Oops, thanks. I think I set the policy first, then later upped the minimum required version and didn't check the policy's default.

[sloretz]

@mikaelarguedas changed to CMAKE_CXX_STANDARD in 0cfdd5c

ci up to composition since the two new commits are minor changes and I think composition tests should confirm poco_vendor built successfully. I'll merge if the tests pass.

• Linux
  • False positive cmake warning Cmake warning about unused variable SECURITY build_farmer#79
• Linux-aarch64
• macOS
• Windows

[mikaelarguedas]

We could use the CMAKE_CXX_STANDARD outside of the if as it's not platform specific. Though I think Windows just ignores that variable so it wouldn't make any difference to the current implementation so we can leave it as is.

ci up to composition since the two new commits are minor changes and I think composition tests should confirm poco_vendor built successfully. I'll merge if the tests pass.

👍