Fix Windows Build Functionality #712

wmmc88 · 2021-09-29T17:00:07Z

run in cmd instead of bash
disable colcon-lcov result
use --merge-install as suggessted in ros2 docs
disable colcon-test on windows

Windows Test and Test-Coverage functionality will be addressed in a future pr

Signed-off-by: Melvin Wang melvin.mc.wang@gmail.com

* run in cmd instead of bash * disable colcon-lcov result * use --merge-install as suggessted in ros2 docs Signed-off-by: Melvin Wang <melvin.mc.wang@gmail.com>

aprotyas · 2021-11-14T20:01:22Z

@wmmc88 thanks for working on this! Do you have a map of TODOs for this? I just wanted to gauge how far along the draft was.. no rush though, just curious!

wmmc88 · 2021-11-15T23:20:11Z

@wmmc88 thanks for working on this! Do you have a map of TODOs for this? I just wanted to gauge how far along the draft was.. no rush though, just curious!

Its been a long while since I looked at this but I think that after I fixed commands running in bash and disabled colcon-lcov result, it got further in the pipeline, but still failed at colcon test. IIRC the issue was that action-setup-ros and action-ros-ci was always using python3.7 so the colcon test command would fail.

wmmc88 · 2021-11-15T23:23:15Z

Actually I think it might have also been that the windows runners by default have some other python version installed and the because of the way the path was setup, colcon test was trying to use the wrong python version so the command would fail with some uninformative error or smth...

IIRC, if you just comment the test step, the rest of the pipeline works fine... so the only thing left should be fixing the test step (which probs involves some changes to both action-ros-ci and action-setup-ros). I'll see if I can maybe pick this back up later this week.

emersonknapp · 2021-11-16T00:24:50Z

I would also be inclined to move forward with an incremental update "it builds on windows now, but the test doesn't run right", which is better than the current situation of "can't do anything on windows"

Signed-off-by: Melvin Wang <melvin.mc.wang@gmail.com>

wmmc88 · 2021-11-17T23:47:50Z

Can a maintainer approve builds? Builds are now all passing on my forked repo

christophebedard · 2021-11-17T23:49:52Z

Can a maintainer approve builds? Builds are now all passing on my forked repo

done!

codecov · 2021-11-17T23:50:33Z

Codecov Report

Merging #712 (9d1d44f) into master (c8ee1c9) will increase coverage by 1.23%.
The diff coverage is 30.30%.

@@            Coverage Diff             @@
##           master     #712      +/-   ##
==========================================
+ Coverage   48.38%   49.61%   +1.23%     
==========================================
  Files           2        2              
  Lines         248      262      +14     
  Branches       58       68      +10     
==========================================
+ Hits          120      130      +10     
- Misses        128      132       +4

Impacted Files	Coverage Δ
src/action-ros-ci.ts	`43.34% <30.30%> (+1.79%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8ee1c9...9d1d44f. Read the comment docs.

wmmc88 · 2021-11-18T00:27:57Z

Have the mac-os tests had spurious failures before? In my fork, the action passes, but it seems to be failing in this pr due to osrf_testing_tools_cpp failing to build. Maybe something to do with a mac runner having wrong deps or smth?

wmmc88#1

christophebedard · 2021-11-18T14:24:06Z

Have the mac-os tests had spurious failures before?

not from what I remember. I re-triggered the CI jobs twice and they keep failing. From past experience, I don't think our clang macOS setup is quite right, or at least it's not exactly the same as the ROS 2 macOS CI, which might be lagging behind IIRC.

Can you try re-triggering CI on your fork?

wmmc88 · 2021-11-18T22:38:20Z

Have the mac-os tests had spurious failures before?

not from what I remember. I re-triggered the CI jobs twice and they keep failing. From past experience, I don't think our clang macOS setup is quite right, or at least it's not exactly the same as the ROS 2 macOS CI, which might be lagging behind IIRC.

Can you try re-triggering CI on your fork?

I've re-triggered CI in my fork and it still all passes. This is a snapshot of the diff of the step that's failing on macos (Test all packages in single repo, default options):

The left(ie. failing build in this fork) runner is running a newer version of clang. This is the error:

In file included from /Users/runner/work/action-ros-ci/action-ros-ci/ros_ws/build/osrf_testing_tools_cpp/googletest-1.8.0-extracted/googletest-1.8.0-src/googletest/src/gtest-all.cc:39:
  In file included from /Users/runner/work/action-ros-ci/action-ros-ci/ros_ws/build/osrf_testing_tools_cpp/googletest-1.8.0-extracted/googletest-1.8.0-src/googletest/include/gtest/gtest.h:62:
  In file included from /Users/runner/work/action-ros-ci/action-ros-ci/ros_ws/build/osrf_testing_tools_cpp/googletest-1.8.0-extracted/googletest-1.8.0-src/googletest/include/gtest/gtest-param-test.h:193:
  /Users/runner/work/action-ros-ci/action-ros-ci/ros_ws/build/osrf_testing_tools_cpp/googletest-1.8.0-extracted/googletest-1.8.0-src/googletest/include/gtest/internal/gtest-param-util-generated.h:107:8: error: definition of implicit copy constructor for 'ValueArray2<bool, bool>' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy]
    void operator=(const ValueArray2& other);
         ^
  /Users/runner/work/action-ros-ci/action-ros-ci/ros_ws/build/osrf_testing_tools_cpp/googletest-1.8.0-extracted/googletest-1.8.0-src/googletest/include/gtest/gtest-param-test.h:354:10: note: in implicit copy constructor for 'testing::internal::ValueArray2<bool, bool>' first required here
    return internal::ValueArray2<T1, T2>(v1, v2);
           ^
  /Users/runner/work/action-ros-ci/action-ros-ci/ros_ws/build/osrf_testing_tools_cpp/googletest-1.8.0-extracted/googletest-1.8.0-src/googletest/include/gtest/gtest-param-test.h:1221:10: note: in instantiation of function template specialization 'testing::Values<bool, bool>' requested here
    return Values(false, true);
           ^
  1 error generated.
  make[2]: *** [googletest-1.8.0-extracted/googletest-1.8.0-build/googlemock/gtest/CMakeFiles/gtest.dir/src/gtest-all.cc.o] Error 1
  make[1]: *** [googletest-1.8.0-extracted/googletest-1.8.0-build/googlemock/gtest/CMakeFiles/gtest.dir/all] Error 2
  make[1]: *** Waiting for unfinished jobs....
  make: *** [all] Error 2

It seems like its this error: osrf/osrf_testing_tools_cpp#35
..but that's been resolved already. This also looks like a similar issue that causes this step to fail on windows too(its disabled here)...

…ndows builds. re-enable windows for 'Test all packages in single repo, default options' step Signed-off-by: Melvin Wang <melvin.mc.wang@gmail.com>

…single repo, default options' test Signed-off-by: Melvin Wang <melvin.mc.wang@gmail.com>

.github/workflows/repo_file_for_test_cpp_package.repos

.github/workflows/test.yml

christophebedard

There are many little changes, but this looks good to me. I'd like to have someone else review and approve as well though.

emersonknapp

Mostly this looks good, I only have a couple requests to help make future maintenance more manageable.

src/action-ros-ci.ts

.github/workflows/test.yml

Signed-off-by: Melvin Wang <melvin.mc.wang@gmail.com>

wmmc88 · 2021-12-29T17:15:16Z

Mostly this looks good, I only have a couple requests to help make future maintenance more manageable.

done!

wmmc88 · 2022-01-20T23:43:16Z

@emersonknapp Could you re-review?

ooeygui · 2022-01-25T19:51:59Z

I'm currently investigating using this action with ROS2.net. This PR fixes several issues discovered during the CI implementation.

I'd love to see this get checked in.

the review looks great. @wmmc88 - thank you for the contribution!

ooeygui · 2022-02-03T20:58:07Z

Awesome! Thank you @wmmc88 and @emersonknapp !

wmmc88 · 2022-02-03T21:46:03Z

FYI: It looks like the pipeline for the merged commit failed, despite it succeeding on the pr. It looks like Github recently moved windows-latest to windows server 2022. The Windows server 2022 image has [visual studio 2022] instead of 2019, which is why the build is now failing on master.

Not sure what the resolution to this should be since the official docs for ros2 seem to imply it only is supported for vs2019.

christophebedard · 2022-02-04T14:17:42Z

I brought up the potential move to VS 2022 recently and was told that it should be done before Humble, but it will probably require some work: ros2/ros2_documentation#2112 (comment).

For now, I think we can change it to windows-2019.

wmmc88 · 2022-02-04T14:48:12Z

I brought up the potential move to VS 2022 recently and was told that it should be done before Humble, but it will probably require some work: ros2/ros2_documentation#2112 (comment).

For now, I think we can change it to windows-2019.

This action probably needs some logic to determine which version to use based off of the ros distribution then. Assuming that humble uses 2022, I assume all the older versions will only work with 2019.

I think it'd be useful to document somewhere that this action needs to be run on windows-2019 or use another action that installs vs 2019 when running windows builds.

christophebedard · 2022-02-04T16:49:43Z

This action probably needs some logic to determine which version to use based off of the ros distribution then. Assuming that humble uses 2022, I assume all the older versions will only work with 2019.

Most likely.

I think it'd be useful to document somewhere that this action needs to be run on windows-2019 or use another action that installs vs 2019 when running windows builds.

I'll add it to the README in #719

wmmc88 force-pushed the wmmc88/fix-windows-builds branch from 087f3bb to 3dac559 Compare September 29, 2021 17:13

fix windows functionality:

e8058da

* run in cmd instead of bash * disable colcon-lcov result * use --merge-install as suggessted in ros2 docs Signed-off-by: Melvin Wang <melvin.mc.wang@gmail.com>

wmmc88 force-pushed the wmmc88/fix-windows-builds branch from 3dac559 to e8058da Compare September 29, 2021 17:26

wmmc88 mentioned this pull request Oct 5, 2021

Install openssl on windows ros-tooling/setup-ros#454

Merged

christophebedard mentioned this pull request Oct 22, 2021

Windows support ros2/domain_bridge#58

Open

disable windows colcon test

3b96a52

Signed-off-by: Melvin Wang <melvin.mc.wang@gmail.com>

wmmc88 force-pushed the wmmc88/fix-windows-builds branch from 9684b8d to 3b96a52 Compare November 16, 2021 23:50

wmmc88 marked this pull request as ready for review November 16, 2021 23:51

wmmc88 requested a review from a team as a code owner November 16, 2021 23:51

wmmc88 requested review from MichaelOrlov and prajakta-gokhale and removed request for a team November 16, 2021 23:51

wmmc88 changed the title ~~Fix Windows Functionality~~ Fix Windows Build Functionality Nov 16, 2021

fix tests

c5d1595

Signed-off-by: Melvin Wang <melvin.mc.wang@gmail.com>

wmmc88 force-pushed the wmmc88/fix-windows-builds branch from 28bb529 to c5d1595 Compare November 17, 2021 05:08

wmmc88 marked this pull request as draft November 17, 2021 19:04

wmmc88 added 6 commits November 17, 2021 14:19

fix ros1 tests

906d71f

Signed-off-by: Melvin Wang <melvin.mc.wang@gmail.com>

fix ros1 tests

6e2b7f9

Signed-off-by: Melvin Wang <melvin.mc.wang@gmail.com>

fix ros1 tests

720f84a

Signed-off-by: Melvin Wang <melvin.mc.wang@gmail.com>

use lists instead of strings to pass options around

6d47a0c

Signed-off-by: Melvin Wang <melvin.mc.wang@gmail.com>

fix github workflow to handle merge-install in windows

1648f31

Signed-off-by: Melvin Wang <melvin.mc.wang@gmail.com>

fix colcon mixin url parsing

47e4f55

Signed-off-by: Melvin Wang <melvin.mc.wang@gmail.com>

wmmc88 marked this pull request as ready for review November 17, 2021 23:43

wmmc88 added 2 commits November 18, 2021 17:42

Uprev osrf_testing_tools_cpp in github workflow to fix flaky macos/wi…

e7d2374

…ndows builds. re-enable windows for 'Test all packages in single repo, default options' step Signed-off-by: Melvin Wang <melvin.mc.wang@gmail.com>

fix github workflow to handle merge-install in 'Test all packages in …

b14615a

…single repo, default options' test Signed-off-by: Melvin Wang <melvin.mc.wang@gmail.com>

wmmc88 commented Nov 19, 2021

View reviewed changes

.github/workflows/repo_file_for_test_cpp_package.repos Show resolved Hide resolved

.github/workflows/test.yml Outdated Show resolved Hide resolved

wmmc88 force-pushed the wmmc88/fix-windows-builds branch from c7d0a03 to b14615a Compare November 23, 2021 03:25

christophebedard approved these changes Nov 30, 2021

View reviewed changes

emersonknapp requested changes Dec 1, 2021

View reviewed changes

src/action-ros-ci.ts Outdated Show resolved Hide resolved

.github/workflows/test.yml Outdated Show resolved Hide resolved

wmmc88 added 2 commits December 13, 2021 22:36

Reduce clutter in test.yml due to windows-specific CI steps

175d00a

Signed-off-by: Melvin Wang <melvin.mc.wang@gmail.com>

simplify windows specific logic

9d1d44f

Signed-off-by: Melvin Wang <melvin.mc.wang@gmail.com>

wmmc88 requested a review from emersonknapp December 14, 2021 04:07

aprotyas mentioned this pull request Jan 23, 2022

[CI] Add windows to job matrix aprotyas/trac_ik#30

Open

ooeygui mentioned this pull request Jan 25, 2022

Update setup-ros action ros2-dotnet/ros2_dotnet#86

Merged

emersonknapp approved these changes Feb 3, 2022

View reviewed changes

emersonknapp merged commit ee483a7 into ros-tooling:master Feb 3, 2022

christophebedard mentioned this pull request Feb 4, 2022

Temporarily use windows-2019 #719

Merged

wmmc88 mentioned this pull request Feb 18, 2022

Windows build environment fails to propagate to CMake external project #643

Closed

This was referenced Feb 18, 2022

lcov code breaks build on Mac, Windows #312

Open

Support running 'colcon test' on Windows #725

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Windows Build Functionality #712

Fix Windows Build Functionality #712

wmmc88 commented Sep 29, 2021 •

edited

aprotyas commented Nov 14, 2021

wmmc88 commented Nov 15, 2021

wmmc88 commented Nov 15, 2021

emersonknapp commented Nov 16, 2021

wmmc88 commented Nov 17, 2021

christophebedard commented Nov 17, 2021

codecov bot commented Nov 17, 2021 •

edited

wmmc88 commented Nov 18, 2021 •

edited

christophebedard commented Nov 18, 2021

wmmc88 commented Nov 18, 2021

christophebedard left a comment

emersonknapp left a comment

wmmc88 commented Dec 29, 2021

wmmc88 commented Jan 20, 2022

ooeygui commented Jan 25, 2022

ooeygui commented Feb 3, 2022

wmmc88 commented Feb 3, 2022

christophebedard commented Feb 4, 2022

wmmc88 commented Feb 4, 2022

christophebedard commented Feb 4, 2022

Fix Windows Build Functionality #712

Fix Windows Build Functionality #712

Conversation

wmmc88 commented Sep 29, 2021 • edited

aprotyas commented Nov 14, 2021

wmmc88 commented Nov 15, 2021

wmmc88 commented Nov 15, 2021

emersonknapp commented Nov 16, 2021

wmmc88 commented Nov 17, 2021

christophebedard commented Nov 17, 2021

codecov bot commented Nov 17, 2021 • edited

Codecov Report

wmmc88 commented Nov 18, 2021 • edited

christophebedard commented Nov 18, 2021

wmmc88 commented Nov 18, 2021

christophebedard left a comment

Choose a reason for hiding this comment

emersonknapp left a comment

Choose a reason for hiding this comment

wmmc88 commented Dec 29, 2021

wmmc88 commented Jan 20, 2022

ooeygui commented Jan 25, 2022

ooeygui commented Feb 3, 2022

wmmc88 commented Feb 3, 2022

christophebedard commented Feb 4, 2022

wmmc88 commented Feb 4, 2022

christophebedard commented Feb 4, 2022

wmmc88 commented Sep 29, 2021 •

edited

codecov bot commented Nov 17, 2021 •

edited

wmmc88 commented Nov 18, 2021 •

edited