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

Account for potential target namespace when finding cmake module... #2

Merged
merged 5 commits into from
Dec 13, 2018

Conversation

jacobperron
Copy link
Member

for tinyxml2.

Resolves ros2/build_farmer#156

@jacobperron jacobperron added the in progress Actively being worked on (Kanban column) label Dec 11, 2018
@jacobperron jacobperron self-assigned this Dec 11, 2018
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

sloretz
sloretz previously approved these changes Dec 11, 2018
@jacobperron
Copy link
Member Author

jacobperron commented Dec 11, 2018

Looks like the namespace is not required when linking.

@jacobperron
Copy link
Member Author

Test on CI machine with latest tinyxml2 release: Build Status

Requires ros2/Fast-RTPS (find_tinyxml2_update).

@jacobperron
Copy link
Member Author

Regular CI (includes MacOS with older tinyxml2 release to test backwards compatibility):

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

@jacobperron jacobperron added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Dec 11, 2018
@jacobperron
Copy link
Member Author

Take 2:

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

@jacobperron
Copy link
Member Author

Take 3:

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

@jacobperron
Copy link
Member Author

The test CI job made it past the tinyxml related errors and is stuck on an unrelated error.


Rebuild Windows: Build Status 🤞

Copy link
Contributor

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

I contributed to this patch, but +1 because it also solves the problems locally on my osx.

Copy link
Contributor

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

rebuilding on my machine seems to work. so +1

@jacobperron
Copy link
Member Author

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

@nuclearsandwich
Copy link
Member

03:57:23 -- Found fastrtps_cmake_module: 0.6.0 (/home/rosbuild/ci_scripts/ws/install/fastrtps_cmake_module/share/fastrtps_cmake_module/cmake)
03:57:23 CMake Error at /home/rosbuild/ci_scripts/ws/install/fastrtps/share/fastrtps/cmake/fastrtps-config.cmake:50 (find_package):
03:57:23   By not providing "FindTinyXML2.cmake" in CMAKE_MODULE_PATH this project has
03:57:23   asked CMake to find a package configuration file provided by "TinyXML2",
03:57:23   but CMake did not find one.
03:57:23 
03:57:23   Could not find a package configuration file provided by "TinyXML2" with any
03:57:23   of the following names:
03:57:23 
03:57:23     TinyXML2Config.cmake
03:57:23     tinyxml2-config.cmake
03:57:23 
03:57:23   Add the installation prefix of "TinyXML2" to CMAKE_PREFIX_PATH or set
03:57:23   "TinyXML2_DIR" to a directory containing one of the above files.  If
03:57:23   "TinyXML2" provides a separate development package or SDK, be sure it has
03:57:23   been installed.
03:57:23 Call Stack (most recent call first):
03:57:23   CMakeLists.txt:38 (find_package)

@jacobperron
Copy link
Member Author

After update to eProsima/Fast-DDS#349:

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

@mjcarroll
Copy link
Member

Looks like network issues for both of these, rebuilding.

  • macOS Build Status
  • Windows Build Status

@nuclearsandwich
Copy link
Member

nuclearsandwich commented Dec 12, 2018

The VCS import failed again. When I try to import that repos file locally I get

=== src/ros2/Fast-RTPS (git) ===
Could not checkout ref 'find_tinyxml2_update': error: pathspec 'find_tinyxml2_update' did not match any file(s) known to git.

Since Fast-RTPS has merged eProsima/Fast-DDS#349 here is the same again with just this branch.

🍎 Build Status
🎐 (queued) Build Status

@jacobperron
Copy link
Member Author

Aaand the updated 🍎 Build Status

@nuclearsandwich
Copy link
Member

Again and again... 🍎 Build Status

This time qt is linked so qmake is on PATH

@nuclearsandwich
Copy link
Member

The CI failures above are caused by a rash of infrastructure related troubles and a recent issue with Windows that is not introduced by this change.

Build on our current MacOS configuration is passing: Build Status
The updated MacOS configuration where this issue was first discovered is now able to build packages dependent on TinyXML2 but has issues with other upgraded packages Build Status
Windows test failures are unrelated Build Status (also failures are causing builds to run very long)

I am proposing that given the reviews received on this change and the CI coverage as available, that this PR be considered merge-able.

@jacobperron jacobperron merged commit 2a1ed4b into master Dec 13, 2018
@jacobperron jacobperron deleted the find_tinyxml2_update branch December 13, 2018 05:29
@jacobperron jacobperron removed the in review Waiting for review (Kanban column) label Dec 13, 2018
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.

6 participants