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

Port ign-launch to Windows #120

Merged
merged 25 commits into from
Sep 13, 2021
Merged

Port ign-launch to Windows #120

merged 25 commits into from
Sep 13, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jul 5, 2021

🎉 New feature

Related issue #89

Summary

This PR is part of the port of ign-launch to Windows and a continuation of the work started by @j-rivero

Right now it's compiling but some of the tests are commented out. The key class of this port is Manager.

Test it

For now we can only test Manager_TEST

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Jul 5, 2021
@ahcorde ahcorde mentioned this pull request Jul 5, 2021
10 tasks
@chapulina
Copy link
Contributor

Thanks, @ahcorde !

Right now it's compiling but some of the tests are commented out

It's perfectly fine to disable tests on Windows for this first pass. We can just get it compiling first, and address other issues later.

What's left to open this PR for review? Is it just a general cleanup to make sure Linux and macOS are still working?

@chapulina chapulina added the Windows Windows support label Jul 6, 2021
@ahcorde ahcorde mentioned this pull request Jul 7, 2021
5 tasks
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
@codecov
Copy link

codecov bot commented Jul 8, 2021

Codecov Report

Merging #120 (59af2e9) into main (fc1efd2) will decrease coverage by 0.84%.
The diff coverage is 23.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #120      +/-   ##
==========================================
- Coverage   34.09%   33.25%   -0.85%     
==========================================
  Files           4        4              
  Lines         786      806      +20     
==========================================
  Hits          268      268              
- Misses        518      538      +20     
Impacted Files Coverage Δ
src/cmd/launch_main.cc 88.46% <ø> (ø)
src/vendor/backward.hpp 8.55% <ø> (-0.52%) ⬇️
src/Manager.cc 55.10% <23.07%> (-0.91%) ⬇️

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 fc1efd2...59af2e9. Read the comment docs.

@ahcorde ahcorde marked this pull request as ready for review July 8, 2021 12:40
@ahcorde ahcorde requested a review from nkoenig as a code owner July 8, 2021 12:40
@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 8, 2021

@chapulina this is ready for review, but we should add the windows job.

Signed-off-by: ahcorde <[email protected]>
@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 8, 2021

@osrf-jenkins retest this please

Signed-off-by: ahcorde <[email protected]>
@ahcorde ahcorde mentioned this pull request Jul 9, 2021
4 tasks
@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 9, 2021

Windows build should be fixed with this PR gazebosim/gz-transport#250

@ahcorde ahcorde added the needs upstream release Blocked by a release of an upstream library label Jul 9, 2021
@chapulina
Copy link
Contributor

Niiice! It's building on Windows CI, it just has some warnings and failing tests, which I think can be addressed later.

We still need to fix macOS and DCO though.

Signed-off-by: ahcorde <[email protected]>
@ahcorde ahcorde marked this pull request as draft July 22, 2021 15:16
@scpeters
Copy link
Member

I noticed a windows-related problem with the cmd*.rb files when I was testing gazebosim/gz-transport#216. The following lines have some logic for detecting when a string is an absolute or relative path based on whether it starts with /, but that is not valid logic for Windows, which has absolute paths that can start with C:\

https://github.com/ignitionrobotics/ign-launch/blob/main/src/cmd/cmdlaunch.rb.in#L33-L40

We can put this in an issue if you aren't planning to address the ruby code in this PR

@j-rivero
Copy link
Contributor

j-rivero commented Sep 1, 2021

We can put this in an issue if you aren't planning to address the ruby code in this PR

Let's do it to help with merging this as soon as possible to gain CI on Windows on other changes: #129

@j-rivero j-rivero marked this pull request as ready for review September 2, 2021 14:37
@j-rivero
Copy link
Contributor

j-rivero commented Sep 2, 2021

Compilation on Windows is working again, although tests needs attention.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

LGTM! We can address the test failures later. DCO needs to be fixed.

@j-rivero
Copy link
Contributor

j-rivero commented Sep 2, 2021

LGTM! We can address the test failures later. DCO needs to be fixed.

I'll squash-merge the pull request and will set the right signed-off.

@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2021
@chapulina chapulina merged commit e41bebb into main Sep 13, 2021
@chapulina chapulina deleted the ahcorde/win branch September 13, 2021 22:34
@mabelzhang mabelzhang mentioned this pull request Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏯 fortress Ignition Fortress Windows Windows support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants