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

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 <[email protected]>
This transitively installs yaml-cpp for the target platform.

Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
@jacobperron
Copy link
Member Author

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

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.

Windows support
3 participants