Skip to content
This repository has been archived by the owner on Jul 1, 2021. It is now read-only.

Enable forcing build from source, and pass through c++ flags #24

Merged
merged 3 commits into from
Mar 21, 2019
Merged

Enable forcing build from source, and pass through c++ flags #24

merged 3 commits into from
Mar 21, 2019

Conversation

emersonknapp
Copy link
Contributor

@emersonknapp emersonknapp commented Mar 5, 2019 •
edited
Loading

Utilize the variable names proposed in the libcxx colcon mixin colcon/colcon-mixin-repository#16 to be able to force

  1. building the package from source
  2. using libcxx instead of libstdc++

Related to ros2/ros2#664

@tfoote tfoote added the in review Waiting for review (Kanban column) label Mar 5, 2019
@emersonknapp emersonknapp mentioned this pull request Mar 5, 2019
Merged
dirk-thomas
dirk-thomas reviewed Mar 5, 2019
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
Enable forcing the build, and pass through global C++ flags
80917dd 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp
Copy link
Contributor Author

@thomas-moulard https://gist.githubusercontent.com/emersonknapp/4aad1eaadcec008d8e4037362b8620b9/raw/584f088d7bbc8390744ae7e378c958d19eed127b/ros2.repos

This should cover everything for a CI build. We should probably specify --cmake-args -DFORCE_BUILD_POCO to properly test the change

@emersonknapp emersonknapp changed the title Enable forcing the build, and forcing libcxx Enable forcing build from source, and pass through c++ flags Mar 6, 2019
@thomas-moulard
Copy link

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

@nuclearsandwich nuclearsandwich self-assigned this Mar 7, 2019
@nuclearsandwich

This comment has been minimized.

Sign in to view
dirk-thomas
dirk-thomas reviewed Mar 8, 2019
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
@nuclearsandwich nuclearsandwich added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Mar 12, 2019
@nuclearsandwich nuclearsandwich mentioned this pull request Mar 12, 2019
Merged
@emersonknapp
Copy link
Contributor Author

Pushed new commit on this and other vendor pkgs, plus the colcon mixin

@emersonknapp emersonknapp mentioned this pull request Mar 12, 2019
Closed
3 tasks
@nuclearsandwich
Copy link
Member

nuclearsandwich commented Mar 20, 2019 •
edited
Loading

This is a full build and partial test that I originally intended to test that things are good when building forcing the vendor builds but because I messed up the option name now it's testing that this doesn't regress the default case of only building when the package isn't found. Covers all the #ros2/ros2#664 vendor packages.

Edit: re-triggered with an updated gist for rclcpp_action

  • Linux Build Status retest due to infra failure Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

nuclearsandwich
nuclearsandwich approved these changes Mar 20, 2019
View reviewed changes
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.

This can be merged once green CI comes back for both the standard configuration and with FORCE_BUILD_VENDOR_PKG=ON 🥗

@nuclearsandwich nuclearsandwich mentioned this pull request Mar 20, 2019
Merged
@nuclearsandwich nuclearsandwich added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Mar 20, 2019
@nuclearsandwich
Copy link
Member

nuclearsandwich commented Mar 20, 2019 •
edited
Loading

CI with clang-libcxx enabled and FORCE_BUILD_VENDOR_PKG=ON

  • Linux Build Status
  • Linux-aarch64 Build Status
Mac and Windows builds are not expected to work
  • macOS Build Status
  • macOS Build Status

@nuclearsandwich
Copy link
Member

@emersonknapp that last CI right might actually be overzealousness on my part. Is the libcxx stuff even expected to be functional on Mac and Win as is?

@emersonknapp
Copy link
Contributor Author

emersonknapp commented Mar 20, 2019 •
edited
Loading

@emersonknapp that last CI right might actually be overzealousness on my part. Is the libcxx stuff even expected to be functional on Mac and Win as is?

My understanding is that we were not targeting Windows or OSX for the clang-libcxx build yet.

Looking into the OSX failure for rviz

@emersonknapp emersonknapp mentioned this pull request Mar 21, 2019
Merged
@nuclearsandwich
Copy link
Member

It looks like the libcxx build is creating failures with class_loader Build Status but I haven't identified a cause yet.

@emersonknapp
Copy link
Contributor Author

Can we disable tests for this build for now? Or, start it out as being unstable because of failed tests? I think we've made a significant milestone in compiling end to end with libcxx

@nuclearsandwich
Copy link
Member

Can we disable tests for this build for now? Or, start it out as being unstable because of failed tests? I think we've made a significant milestone in compiling end to end with libcxx

Based on the failing tests, it doesn't seem like class_loader, and therefore pluginlib is functional with the libcxx changes. As long as those aren't needed by what you plan to test then we can avoid running class loader tests.

@emersonknapp
Copy link
Contributor Author

The first goal is to test compilation - in order to get thread safety compile-time warnings. I think we can work towards making all the tests pass - it would be great to get it going as a nightly that we can start working on fixes for.

@nuclearsandwich nuclearsandwich mentioned this pull request Mar 21, 2019
Merged
@nuclearsandwich
Copy link
Member

Re-triggering CI. Full build, default configuration (vendor packages will use system package when available), testing packages above the vendor packages but omitting class_loader and pluginlib for now.

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

@nuclearsandwich nuclearsandwich merged commit db38063 into ros2:master Mar 21, 2019
@nuclearsandwich nuclearsandwich removed the in review Waiting for review (Kanban column) label Mar 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wjwwood wjwwood wjwwood left review comments

@thomas-moulard thomas-moulard thomas-moulard left review comments

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

@nuclearsandwich nuclearsandwich nuclearsandwich approved these changes

Assignees

@nuclearsandwich nuclearsandwich

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@emersonknapp @thomas-moulard @nuclearsandwich @wjwwood @dirk-thomas @tfoote