-
Notifications
You must be signed in to change notification settings - Fork 57
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
Update setup-ros action #86
Conversation
Posting as an FYI: |
@ooeygui cool! I've managed to get the Linux build working, but colcon has issues with the packages in both UWP builds. Moreover the desktop build can't build rcutils, I'll spin a Windows VM and try building that locally. |
Thank you for your work on this! |
I experimented on different approaches to CI - including attempting to convert the solution completely to ros-tooling. I think building all of ros is contributing to the circular dependency. Perhaps leveraging RoboStack could improve the CI round trip and eliminate the circular reference. |
Looking at this. |
@ooeygui did you have time to look into this? Anything I can help with? |
Hi @esteve, |
I set up a local build of Github and figured out why the builds aren't working with the ROS2 action. There's a pending change in ros-tooling/action-ros-ci#712 which corrects the build environment for windows. I think there is still a bug with checked in vcs files which I'm currently debugging. |
@ooeygui thank you so much for so much work. I'd like to make some changes to the CI to make it look cleaner, but I can wait until you're done with the fixes. Thanks again! |
I'm testing the 712 PR and in general it looks good. However, I'm debugging why local_setup.ps1 isn't getting generated, which causes the build to fail. colcon_package_source_powershell_script : not found: (traceback)
'C:\Users\...\github\ros2_dotnet\ros_ws\install\share/ament_cmake_export_assemblies/local_setup.ps1'
At C:\Users\...\github\ros2_dotnet\ros_ws\install\share\ament_cmake_export_assemblies\package.ps1:63 char:1
+ colcon_package_source_powershell_script "$env:COLCON_CURRENT_PREFIX\s ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo : NotSpecified: (:) [Write-Error], WriteErrorException
+ FullyQualifiedErrorId : Microsoft.PowerShell.Commands.WriteErrorException,colcon_package_source_powershell_script |
I'm debugging why $ is not set in the CI. I can repro locally. |
In order to handle the build race condition with the generator, we need to use --merge-install. However, merge-install executes a codepath which runs in the generate phase - where <$CONFIG> cannot be evaluated. The dotnet_cmake_module project uses <$CONFIG> to switch between Release and Debug, because CMAKE_BUILD_TYPE on the msbuild generator is a complex string, not a simple Debug/Release like on other generators. Ninja is a has a simple build type, but difficult to set using colcon through the CI scripts (not as simple as --use-ninja). I experimented with trying to use ninja in the build, but CI is unhappy with that. Looking at fixing the dotnet_cmake_module now. |
I was able to resolve the build issue; except it looks like github released a new CI image a couple of days ago which impacts OpenSSL:
|
It looks like this is resolved in the master branch, but not a tagged release. This change relies on in progress PRs which have been outstanding for a bit. Looking for guidance: This now builds: name: Build (Desktop)
on: [push, pull_request]
jobs:
build-desktop:
runs-on: windows-latest
env:
VisualStudioVersion: '16.0'
steps:
- name: Checkout source
uses: actions/checkout@v2
- name: Setup ROS2
uses: ros-tooling/setup-ros@master
with:
required-ros-distributions: foxy
- uses: wmmc88/action-ros-ci@wmmc88/fix-windows-builds
with:
package-name: rcldotnet_examples
target-ros2-distro: foxy
vcs-repo-file-url: ${{github.workspace}}/.github/workflows/ci.repos
extra-cmake-args: -DCMAKE_SYSTEM_VERSION=10.0.19041.0 -Wno-dev |
The PR for wmmc88's change is in master now - ros-tooling/action-ros-ci#712. |
@ooeygui sorry for the slow response, I missed your message. Thank you so much for tracking this issue, no problem with using an untagged version, though I assume 0.2.4, which was released two weeks ago should contain this fix. In any case, feel free to update the action and then we can move onto the other PRs. Thanks! |
Hi @esteve, |
I've confirmed that the tagged versions work correctly. @esteve, I wanted to confirm your ask - do you want me to patch this PR with the following? OR do you want me to create a new PR? name: Build (Desktop)
on: [push, pull_request]
jobs:
build-desktop:
runs-on: windows-2019
env:
VisualStudioVersion: '16.0'
steps:
- name: Checkout source
uses: actions/checkout@v3
- name: Setup ROS2
uses: ros-tooling/setup-ros@master
with:
required-ros-distributions: foxy
- uses: ros-tooling/[email protected]
with:
package-name: rcldotnet_examples
target-ros2-distro: foxy
vcs-repo-file-url: ${{github.workspace}}/.github/workflows/ci.repos
extra-cmake-args: -DCMAKE_SYSTEM_VERSION=10.0.19041.0 -Wno-dev |
Sorry to hear that, I hope everything is ok and let me know if you'd like me to pick up this PR. |
That's awesome! Push to this PR is fine, thanks! |
I'm going to iterate on the UWP builds on ms-iot so I don't pollute this change. |
@esteve I've been working on the UWP CI. There are dependencies on work that I need to upstream from ms-iot in other repositories which prevent it from working in this repo until that work has been completed. ProposalDisable the UWP CI build in ROS2.net JustificationThe Hololens 2 uses a version of Windows called WCOS, which is not quite Windows 10 and not quite Windows 11. It is also based on ARM64. The UWP build for Hololens 2 requires significant changes to ROS2 itself, as well as changes to dependencies such as FastDDS. This work was done in the ms-iot organization, and being prepared for upstreaming. In order to unblock ROS2.net development, I'd like to disable the UWP CI until that other work can be up streamed. Once upstreamed, separate work can be done in this repo to reenable UWP. |
@ooeygui thanks for looking into that. I'm checking the logs and it's rather weird, because |
@esteve thanks for the reply. I'll patch this change to remove UWP. I'm working through the process of upstreaming the following forks which have Most are simple fixes, but first have to go into the main branches, before being cherry picked into the foxy branches. I don't have an estimate of the upstream velocity in each repo just yet. |
@esteve Since we both made changes to this PR, I'd like to get final approval from you before pulling it. |
@ooeygui thanks for the links. I can only say, good luck getting the fixes into Fast-DDS, it took them an entire year (literally, 365 days between since I created the PR until it got reviewed) to get my UWP changes merged (eProsima/Fast-DDS#26) and then broke them again, so I had to fix the UWP build (eProsima/Fast-DDS#247). You'd probably have a better experience and have more chances of getting UWP to work by switching to Cyclone DDS, additionally their codebase is simpler and better organized. Anyway, let me know if you need reviewers for the ROS2 stuff, happy to chime in.
I can't approve this PR because I created it, but feel free to merge it 👍 |
No description provided.