2 ➡️ 5

[jennuine]

➡️ Forward port

Port ign-launch2 to ign-launch5

Branch comparison: ign-launch5...ign-launch2

Note to maintainers: Remember to Merge with commit (not squash-merge or rebase)

[mabelzhang]

Argh, looks like UNIT_ign_TEST failed on something else in Bionic. Will check tomorrow.

[mabelzhang]

I reverted the Ls test because I wasn't running it correctly locally, which was why it failed for me. It was supposed to be run from the build/ignition-launch5 directory, where the CMakeFiles and Makefile files in the test are to be found. I ran it from the top of the workspace and thought it was broken, but it was actually fine.

But the other tests were failing on CI because they couldn't even find the ign launch subcommand. That is weird. I reverted the IGN_CONFIG_PATH changes, thinking maybe that has something to do with it... because I don't see anything else that could have caused the subcommand not to be found. Let's see what the CI says.

If that doesn't fix it, maybe we should look at this PR further how it replaced that environment variable... f76990e
As of now, the tests aren't using that variable anymore.

[codecov]

Codecov Report

Merging #180 (a4e4631) into ign-launch5 (0005cc2) will increase coverage by 32.92%.
The diff coverage is n/a.

❗ Current head a4e4631 differs from pull request most recent head 6ca4edb. Consider uploading reports for the commit 6ca4edb to get more accurate results

@@               Coverage Diff                @@
##           ign-launch5     #180       +/-   ##
================================================
+ Coverage             0   32.92%   +32.92%     
================================================
  Files                0        4        +4     
  Lines                0      817      +817     
================================================
+ Hits                 0      269      +269     
- Misses               0      548      +548     

Impacted Files	Coverage Δ
src/cmd/launch_main.cc	74.60% <0.00%> (ø)
src/Manager.cc	55.10% <0.00%> (ø)
src/cmd/ign.cc	80.00% <0.00%> (ø)
src/vendor/backward.hpp	8.55% <0.00%> (ø)

---

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 0005cc2...6ca4edb. Read the comment docs.

[mabelzhang]

All CI passing except for Windows.
All 7 tests in UNIT_ign_TEST are failing on Windows, but actually it looks like they were never passing on Windows in the original port to Windows? That PR had all 6 tests (1 is new since) failing on Windows too #120

(With tests passing on Ubuntu after the reverts, I guess that means the IGN_CONFIG_PATH has to be set the original way it was. I think it was failing for us locally because there were some artifacts from before that variable was removed in f76990e, and using it in the custom execution string like IGN_CONFIG_PATH=/path was not liked by the tests.)

[chapulina]

Yeah the Windows test failures are pre-existing:

https://build.osrfoundation.org/job/ign_launch-ign-5-win/14/#showFailuresLink

This PR looks good to me, it would just be nice to squash the last 3 commits and suffix the commit message with (#180) so that it points back to this PR after merged.

[jennuine]

This PR looks good to me, it would just be nice to squash the last 3 commits and suffix the commit message with (#180) so that it points back to this PR after merged.

Done 6ca4edb

Thanks for the help @mabelzhang !

[jennuine]

Yeah the Windows test failures are pre-existing:

Should we disable these tests for windows?

[chapulina]

Should we disable these tests for windows?

Yeah the command line ones definitely. The other one may be fixable. That can come in a separate PR though.