Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Windows support #58

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Windows support #58

wants to merge 4 commits into from

Conversation

jacobperron
Copy link
Member

@jacobperron jacobperron commented Oct 21, 2021

Changes to supporting building on Windows (3aa08a8).

  • Add missing visibility macros, including CMake target definition
  • Fix build error passing std::filesystem::path instead of std::string
  • Fix linker error building against yaml-cpp
  • Quiet warnings caused by including "yaml-cpp/yaml.h"
  • Fix build warning in test_domain_bridge.cpp

Also trying to add Windows CI to the GitHub workflow (db17a54)

Closes #57

Comment on lines 24 to 25
vcs-repo-file-url: >
https://raw.githubusercontent.com/ros2/ros2/master/ros2.repos
Copy link
Member

Choose a reason for hiding this comment

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

Why a specific repos file for Windows?

Copy link
Member Author

@jacobperron jacobperron Oct 21, 2021

Choose a reason for hiding this comment

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

Based on the previous failure; I'm pretty sure the ros-tooling actions are not installing any binaries for Windows (like they do for Linux): https://github.com/ros2/domain_bridge/runs/3969771793?check_suite_focus=true

So, I think we need to build everything from source. Please correct me if I'm wrong.

Copy link
Member

@aprotyas aprotyas Oct 21, 2021

Choose a reason for hiding this comment

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

Ah okay I see. It looks like that didn't work. Here's some discussion that might be relevant: ros-tooling/action-ros-ci#643 (comment)
[edit: Never mind, the CI failure from 7682b0c might be something else. It can't find catkin_pkg now]

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I've learned that the setup-ros action supports Windows binaries, just not for Rolling: https://github.com/ros-tooling/setup-ros/blob/c4e8165ee5518c04dc4588bff7c1028ccf563e6f/src/setup-ros-windows.ts#L9-L17

So, I'm thinking for now we only enable Windows CI for the galactic branch, (until ros-tooling/setup-ros#453 is resolved).

For this pull request, I can try running CI from ci.ros2.org infrastructure.

Copy link
Member

Choose a reason for hiding this comment

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

note that action-ros-ci might not actually properly work for Windows. This PR should apparently fix it though: ros-tooling/action-ros-ci#712

@jacobperron jacobperron force-pushed the windows_support branch 3 times, most recently from db17a54 to 3aa08a8 Compare October 22, 2021 17:36
@jacobperron
Copy link
Member Author

jacobperron commented Oct 22, 2021

Windows CI: Build Status

Edit: we can build on Windows now, but the tests are failing. The output isn't particularly helpful.

Changes to support building on Windows.

- Add missing visibility macros, including CMake target definition
- Fix build error passing std::filesystem::path instead of std::string
- Fix linker error building against yaml-cpp
- Quiet warnings caused by including "yaml-cpp/yaml.h"
- Fix build warning in test_domain_bridge.cpp

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
This transitively installs yaml-cpp for the target platform.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows support
3 participants