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

Enable use of ign gazebo -s on Windows #1574

Closed

Conversation

traversaro
Copy link
Contributor

@traversaro traversaro commented Jul 5, 2022

🎉 New feature

This PR enable the use of ign gazebo -s on Windows. Similarly to what is done in macOS, as ign gazebo and ign gazebo -g are not supported, those options are explicitly disabled to give a nice error to the users.

Summary

Summary of changes:

  • Add src/cmd directory also on Windows
  • Correctly mark export visibility in ign.hh and ModelCommandAPI.hh
  • Ensure that the correct directory if used in cmdgazebo.rb.in to load the gazebo plugin
  • Fix the logic to copy the unversion versions of the plugins
  • Fix UNIT_ign_TEST to work correctly on Windows
  • Disable generation of test-only ruby scripts for ign-tools when using multiple config CMake generators, and consequently disable UNIT_ign_TEST and UNIT_ModelCommandAPI_TEST` when using multiple config CMake generators.
  • As only ign gazebo -s is working, explicitly disable ign gazebo -g and ign gazebo as done on macOS .

Test it

Note, the following instructions are not complete as a version of libignition-common4 with gazebosim/gz-common#389 needs to be released before the tests work again.

Personally, I tested it with conda-forge provided depedencies. I created an environment with:

mamba create -n igngaz vs2019_win-64 cmake ninja pkg-config libignition-cmake2 libsdformat12 libignition-plugin1 libignition-transport11 libignition-msgs8 libignition-common4 libignition-fuel-tools7 libignition-gui6 libignition-physics5 libignition-sensors6 libignition-rendering6 libignition-math6 libignition-tools1 libprotobuf qt-main tinyxml2

Then, I compiled ign-gazebo with:

mamba activate igngaz
git clone https://github.com/traversaro/ign-gazebo
cd ign-gazebo
git checkout fixigngazeboserveronwin
mkdir build
cd build
cmake -GNinja -DCMAKE_INSTALL_PREFIX=%CONDA_PREFIX%\Library-DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_SYSTEM_RUNTIME_LIBS_SKIP:BOOL=ON ..

After that, I was able to run correctly the server:

mamba activate igngaz
ign gazebo --verbose -s shapes.sdf

To verify the server was running correctly, I run in another terminal:

(igngaz) C:\Users\STraversaro>ign model --list

Requesting state for world [shapes]...

Available models:
    - ground_plane
    - box
    - cylinder
    - sphere
    - capsule
    - ellipsoid

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@traversaro traversaro requested a review from chapulina as a code owner July 5, 2022 19:12
@traversaro traversaro force-pushed the fixigngazeboserveronwin branch from d0d939c to 7bca2d0 Compare July 5, 2022 19:13
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

linters and update branch with ign-gazebo6

@traversaro traversaro force-pushed the fixigngazeboserveronwin branch from 7bca2d0 to ee57695 Compare July 5, 2022 19:50
@chapulina chapulina added Windows Windows support 🏯 fortress Ignition Fortress labels Jul 5, 2022
@traversaro traversaro force-pushed the fixigngazeboserveronwin branch from ee57695 to b170a72 Compare July 5, 2022 20:23
@codecov
Copy link

codecov bot commented Jul 5, 2022

Codecov Report

Merging #1574 (d00c62b) into ign-gazebo6 (25bdef6) will decrease coverage by 0.66%.
The diff coverage is n/a.

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

@@               Coverage Diff               @@
##           ign-gazebo6    #1574      +/-   ##
===============================================
- Coverage        64.69%   64.02%   -0.67%     
===============================================
  Files              321      317       -4     
  Lines            26055    25649     -406     
===============================================
- Hits             16855    16422     -433     
- Misses            9200     9227      +27     
Impacted Files Coverage Δ
src/gui/Gui.cc 61.97% <0.00%> (-10.17%) ⬇️
src/systems/user_commands/UserCommands.cc 53.33% <0.00%> (-4.77%) ⬇️
src/systems/physics/Physics.cc 70.37% <0.00%> (-1.07%) ⬇️
src/systems/log/LogRecord.cc 78.13% <0.00%> (-1.02%) ⬇️
src/Util.cc 92.53% <0.00%> (-0.58%) ⬇️
src/ServerConfig.cc 90.69% <0.00%> (-0.31%) ⬇️
.../plugins/component_inspector/ComponentInspector.cc 5.33% <0.00%> (-0.29%) ⬇️
src/Model.cc 95.77% <0.00%> (-0.23%) ⬇️
src/ServerPrivate.cc 82.81% <0.00%> (-0.16%) ⬇️
src/EntityComponentManager.cc 89.36% <0.00%> (-0.03%) ⬇️
... and 19 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

set(plugin_location ${CMAKE_INSTALL_LIBDIR})
endif()

set(library_location "../../../${plugin_location}/$<TARGET_FILE_NAME:${ign_lib_target}>")

Copy link
Contributor

Choose a reason for hiding this comment

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

Windows CI is failing on this file:

[12.204s] CMake Error in src/cmd/CMakeLists.txt:
[12.204s]   Evaluation file to be written multiple times with different content.  This
[12.204s]   is generally caused by the content evaluating the configuration type,
[12.204s]   language, or location of object files:
[12.204s] 
[12.204s]    C:/Jenkins/workspace/ign_gazebo-pr-win/ws/build/ignition-gazebo6/test/lib/ruby/ignition/cmdgazebo6.rb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. For reference, I was developing using Ninja cmake generator, while this error appears with Visual Studio 16 2019.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This turned out to be quite a can of worms. Basically, all the logic that is generating .yaml files and ruby scripts for testing was written strictly assuming the use of a single-config CMake generator. So everything was working fine when using make, single-config Ninja or NMake Makefiles, but it was going to fail as soon as Visual Studio, XCode or multiple config Ninja was used.

Initially, I tried to make the CMake code support also multiple config generator, but that was going to make that code quite difficult to mantain. For this reason, I switched the PR to explicitly disable the UNIT_ign_TEST and UNIT_ModelCommandAPI_TEST test when a multiple-config CMake generator is used. Note that this test can be run on Windows using any of the available single-config CMake generators, such as Ninja or NMake Makefiles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, while working on this I also got UNIT_ign_TEST to work on Windows.

@traversaro
Copy link
Contributor Author

@ahcorde I worked on this locally, I still need to push the local version in the repo, sorry!

@traversaro traversaro force-pushed the fixigngazeboserveronwin branch 2 times, most recently from 8427a9d to 0ea9426 Compare July 19, 2022 21:30
@traversaro
Copy link
Contributor Author

@ahcorde I worked on this locally, I still need to push the local version in the repo, sorry!

The PR is now updated. I update the original post to reflect the changes, note that the unreleased change gazebosim/gz-common#389 is required to actually get the UNIT_ign_TEST test to pass.

@ahcorde ahcorde added the needs upstream release Blocked by a release of an upstream library label Jul 20, 2022
@traversaro
Copy link
Contributor Author

It seems that there is some not intentional interaction with INTEGRATION_log_system.

@traversaro
Copy link
Contributor Author

It seems that there is some not intentional interaction with INTEGRATION_log_system.

I tried to reproduce the problem locally, but in my case that test fails also when I am on the master branch (commit 8c66ef3):

197: Running command [IGN_GAZEBO_SYSTEM_PLUGIN_PATH=/home/straversaro/gz-sim/build3/lib LD_LIBRARY_PATH=/home/straversaro/gz-sim/build3/lib:/usr/local/lib:${LD_LIBRARY_PATH} ign gazebo -s  -r -v 4 --iterations 5 --record /home/straversaro/gz-sim/test/worlds/log_record_dbl_pendulum.sdf]
197: The 'ign' command provides a command line interface to the ignition tools.
197:
197:   ign <command> [options]
197:
197: List of available commands:
197:
197:   help:          Print this help text.
197:   fuel:          Manage simulation resources.
197:   gui:           Launch graphical interfaces.
197:   msg:           Print information about messages.
197:   plugin:        Print information about plugins.
197:   sdf:           Utilities for SDF files.
197:   topic:         Print information about topics.
197:   service:       Print information about services.
197:   log:           Record or playback topics.
197:
197: Options:
197:
197:   --force-version <VERSION>  Use a specific library version.
197:   --versions                 Show the available versions.
197:   --commands                 Show the available commands.
197: Use 'ign help <command>' to print help for a command.
197:
197: /home/straversaro/gz-sim/test/integration/log_system.cc:397: Failure
197: Expected equality of these values:
197:   nEntries + 1
197:     Which is: 1
197:   entryCount(logPath)
197:     Which is: 0
197: /home/straversaro/gz-sim/test/integration/log_system.cc:402: Failure
197: Expected equality of these values:
197:   1ul
197:     Which is: 1
197:   entriesDiff.size()
197:     Which is: 0
1/2 Test #197: INTEGRATION_log_system ...........***Exception: SegFault  1.48 sec

@traversaro
Copy link
Contributor Author

It seems that there is some not intentional interaction with INTEGRATION_log_system.

I tried to reproduce the problem locally, but in my case that test fails also when I am on the master branch (commit 8c66ef3):

Ah, I think I know the problem here. I am using WSLg, so the problems:

Are still presents. In theory #1116 was fixed, but thre is still not release of gz-rendering that contains the fix.

@traversaro
Copy link
Contributor Author

@ahcorde I worked on this locally, I still need to push the local version in the repo, sorry!

The PR is now updated. I update the original post to reflect the changes, note that the unreleased change gazebosim/gz-common#389 is required to actually get the UNIT_ign_TEST test to pass.

gz-common4 4.5.2 was released some days ago, containing this change: https://github.com/gazebosim/gz-common/releases/tag/ignition-common4_4.5.2 .

@traversaro traversaro force-pushed the fixigngazeboserveronwin branch from 0ea9426 to 579addc Compare August 28, 2022 17:45
ahcorde
ahcorde previously approved these changes Aug 29, 2022
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

a minor style issue

src/CMakeLists.txt Outdated Show resolved Hide resolved
@traversaro
Copy link
Contributor Author

Still some failureso on Linux/macOS, and some new warnings on Windows.

@ahcorde ahcorde dismissed their stale review August 29, 2022 07:10

errors

@azeey azeey requested a review from nkoenig October 3, 2022 18:13
@scpeters
Copy link
Member

@osrf-jenkins run tests please

@nkoenig
Copy link
Contributor

nkoenig commented Oct 18, 2022

@traversaro , I have a windows build setup from these instructions. I'm using powershell on windows 11. When I run ign gazebo -s shapes.sdf the command exits immediately. Alternatively, when I run ign model --list, the command does run.

Any thoughts, or extra information I should provide?

@traversaro
Copy link
Contributor Author

I would expect everything to work also on Powershell, but just to check: can you try if the commands are working fine on Command Prompt?

@traversaro
Copy link
Contributor Author

traversaro commented Oct 19, 2022

Note, the following instructions are not complete as a version of libignition-common4 with gazebosim/gz-common#389 needs to be released before the tests work again.

I think I understood the problem: this PR was merged and a new release of ign-common4 was released (i.e. 4.5.2, see #1574 (comment)), but the related package of conda-forge was not updated and is still stick to 4.5.1 (probably due to conda-forge/libignition-gazebo-feedstock#24). We can manually bump ign-common4, also to simplify the test of conda-forge/libignition-gazebo-feedstock#49 .

@traversaro
Copy link
Contributor Author

We can manually bump ign-common4, also to simplify the test of conda-forge/libignition-gazebo-feedstock#49 .

Related PR: conda-forge/libignition-common-feedstock#54 .

@nkoenig
Copy link
Contributor

nkoenig commented Oct 19, 2022

I found that the installed cmdmodel6.rb and cmdgazebo6.rb files were identical. Both files contained the model command code. Will the conda-forge PR you linked to fix this?

@traversaro
Copy link
Contributor Author

I found that the installed cmdmodel6.rb and cmdgazebo6.rb files were identical. Both files contained the model command code. Will the conda-forge PR you linked to fix this?

No, that seems a different issue, hopefully fixed by 124c13d .

@traversaro traversaro force-pushed the fixigngazeboserveronwin branch from 124c13d to bb924ea Compare October 19, 2022 21:31
traversaro and others added 10 commits October 19, 2022 23:31
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Silvio <[email protected]>
Adding some API docs for changes from gazebosim#1076

Signed-off-by: Kenji Brameld <[email protected]>
Signed-off-by: Silvio <[email protected]>
* Add topic parameter.

Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Silvio <[email protected]>
* bumped minor and updated changelog

Signed-off-by: Dharini Dutia <[email protected]>

* fixed changelog as per feedback and updated migration guide

Signed-off-by: Dharini Dutia <[email protected]>

Signed-off-by: Dharini Dutia <[email protected]>
Signed-off-by: Silvio <[email protected]>
* initial commit to allow plugin to call a service

Signed-off-by: Liam Han <[email protected]>

* adding tutorial and modifying the world sdf

Signed-off-by: Liam Han <[email protected]>

* added test for single input and single service output

Signed-off-by: Liam Han <[email protected]>

* added test for single input and multiple service output

Signed-off-by: Liam Han <[email protected]>

* added test for invalid matching service name => timeout

Signed-off-by: Liam Han <[email protected]>

* modified variables the camelCase

Signed-off-by: Liam Han <[email protected]>

* fixed typo, indentation, grammar, lines that exceeded 80 char

Signed-off-by: Liam Han <[email protected]>

* fixing ubuntu bionic ci issue

Signed-off-by: Liam Han <[email protected]>

* silly syntax mistake on expect_eq

Signed-off-by: Liam Han <[email protected]>

* added three more test cases that addesses incorrect response type, incorrect request type and false result

Signed-off-by: Liam Han <[email protected]>

* WIP: major restructuring and currently working. Requires more cleanup and test

Signed-off-by: Liam Han <[email protected]>

* WIP: fixed preprocessor define bug

Signed-off-by: Liam Han <[email protected]>

* WIP: working but extremely convoluted

Signed-off-by: Liam Han <[email protected]>

* WIP major modification but a lot of errors and tests failed

Signed-off-by: Liam Han <[email protected]>

* stable version: had to revert back to previous work. all tests passed

Signed-off-by: Liam Han <[email protected]>

* modified to use blocking Request method as well as reduce a service worker thread to just one thread with the publisher. all tests passed

Signed-off-by: Liam Han <[email protected]>

* stable version: had to revert back to previous work. all tests passed

Signed-off-by: Liam Han <[email protected]>

* successfully reverted and tested

Signed-off-by: Liam Han <[email protected]>

* fixing PR suggestions

Signed-off-by: Liam Han <[email protected]>

* changed string with 'serv' to 'srv' and included <mutex> to the header

Signed-off-by: Liam Han <[email protected]>

* fixed indentation and removed rep.set_data since it's unused on the client service

Signed-off-by: Liam Han <[email protected]>

* getting rid of the id

Signed-off-by: Liam Han <[email protected]>

* fixed race condition resulting seldom test failure

Signed-off-by: Liam Han <[email protected]>

* changed from triggerSrv to serviceCount. This compensates for the two threads running at different rate

Signed-off-by: Liam Han <[email protected]>

* braces indentation

Signed-off-by: Mabel Zhang <[email protected]>

* addressing gnu c compiler (gcc) warnings

Signed-off-by: Liam Han <[email protected]>

Signed-off-by: Liam Han <[email protected]>
Signed-off-by: Mabel Zhang <[email protected]>
Co-authored-by: Mabel Zhang <[email protected]>
Signed-off-by: Silvio <[email protected]>
@traversaro traversaro force-pushed the fixigngazeboserveronwin branch from bb924ea to bacff19 Compare October 19, 2022 21:31
@traversaro traversaro requested a review from iche033 as a code owner October 19, 2022 21:31
@traversaro
Copy link
Contributor Author

Sorry, it seems I did something wrong while trying to fix the DCO checks. Probably it is easier for me to open a new PR.

@traversaro
Copy link
Contributor Author

I opened #1764 to have a PR with a clean history. I think we can close this one and continue the discussion on #1764 .

@traversaro traversaro closed this Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🏯 fortress Ignition Fortress needs upstream release Blocked by a release of an upstream library Windows Windows support
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

10 participants