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

Removed windows warnings #58

Merged
merged 5 commits into from
Dec 7, 2020
Merged

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Nov 4, 2020

Signed-off-by: ahcorde [email protected]

@ahcorde ahcorde added enhancement New feature or request 🔮 dome Ignition Dome tests Broken or missing tests / testing infra labels Nov 4, 2020
@ahcorde ahcorde requested a review from iche033 as a code owner November 4, 2020 08:41
@ahcorde ahcorde self-assigned this Nov 4, 2020
@codecov
Copy link

codecov bot commented Nov 4, 2020

Codecov Report

Merging #58 (ac31126) into ign-sensors4 (655ffb2) will increase coverage by 0.01%.
The diff coverage is 85.71%.

Impacted file tree graph

@@               Coverage Diff                @@
##           ign-sensors4      #58      +/-   ##
================================================
+ Coverage         76.00%   76.01%   +0.01%     
================================================
  Files                23       23              
  Lines              2321     2322       +1     
================================================
+ Hits               1764     1765       +1     
  Misses              557      557              
Impacted Files Coverage Δ
include/ignition/sensors/Manager.hh 75.00% <ø> (ø)
include/ignition/sensors/SensorFactory.hh 69.23% <ø> (ø)
src/AirPressureSensor.cc 83.33% <ø> (ø)
src/AltimeterSensor.cc 86.04% <ø> (ø)
src/CameraSensor.cc 73.35% <ø> (ø)
src/GpuLidarSensor.cc 87.01% <ø> (ø)
src/ImageGaussianNoiseModel.cc 80.00% <ø> (ø)
src/ImuSensor.cc 88.46% <ø> (ø)
src/MagnetometerSensor.cc 86.20% <ø> (ø)
src/Noise.cc 73.33% <ø> (ø)
... and 6 more

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 655ffb2...ac31126. Read the comment docs.

@ahcorde
Copy link
Contributor Author

ahcorde commented Nov 4, 2020

Builds have more than 200 warning build on Windows (for example: https://build.osrfoundation.org/job/ign_sensors-pr-win/477/). This PR remove all of them

chapulina and others added 2 commits November 11, 2020 11:35
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Co-authored-by: Ian Chen <[email protected]>
@chapulina
Copy link
Contributor

Argh, I forgot that #59 still had 4 C4251 warnings coming from ignition::rendering::MeshDescriptor which I couldn't figure out how they slipped through the pragmas.

[92.250s]   Building Custom Rule C:/Jenkins/workspace/ign_sensors-pr-win/ws/ign-sensors/test/integration/CMakeLists.txt
[92.359s]   air_pressure_plugin.cc
[98.156s]   air_pressure_plugin.obj : MSIL .netmodule or module compiled with /GL found; restarting link with /LTCG; add /LTCG to the link command line to improve linker performance
[98.359s]   Generating code
[103.656s]   Finished generating code
[103.875s]   INTEGRATION_air_pressure_plugin.vcxproj -> C:\Jenkins\workspace\ign_sensors-pr-win\ws\build\ignition-sensors4\bin\Release\INTEGRATION_air_pressure_plugin.exe
[105.516s]   Building Custom Rule C:/Jenkins/workspace/ign_sensors-pr-win/ws/ign-sensors/test/integration/CMakeLists.txt
[105.641s]   altimeter_plugin.cc
[111.453s]   altimeter_plugin.obj : MSIL .netmodule or module compiled with /GL found; restarting link with /LTCG; add /LTCG to the link command line to improve linker performance
[111.641s]   Generating code
[117.172s]   Finished generating code
[117.391s]   INTEGRATION_altimeter_plugin.vcxproj -> C:\Jenkins\workspace\ign_sensors-pr-win\ws\build\ignition-sensors4\bin\Release\INTEGRATION_altimeter_plugin.exe
[119.063s]   Building Custom Rule C:/Jenkins/workspace/ign_sensors-pr-win/ws/ign-sensors/src/CMakeLists.txt
[119.234s]   RenderingSensor.cc
[119.234s]   RenderingEvents.cc
[119.234s]   ImageGaussianNoiseModel.cc
[119.234s]   ImageNoise.cc
[120.703s] C:\Jenkins\workspace\ign_sensors-pr-win\ws\install\ignition-rendering4\include\ignition\rendering4\ignition/rendering/MeshDescriptor.hh(69,35): warning C4251: 'ignition::rendering::v4::MeshDescriptor::meshName': class 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>' needs to have dll-interface to be used by clients of struct 'ignition::rendering::v4::MeshDescriptor' (compiling source file C:\Jenkins\workspace\ign_sensors-pr-win\ws\ign-sensors\src\RenderingSensor.cc) [C:\Jenkins\workspace\ign_sensors-pr-win\ws\build\ignition-sensors4\src\ignition-sensors4-rendering.vcxproj]
[120.703s] C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.24.28314\include\xstring(4448): message : see declaration of 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>' (compiling source file C:\Jenkins\workspace\ign_sensors-pr-win\ws\ign-sensors\src\RenderingSensor.cc) [C:\Jenkins\workspace\ign_sensors-pr-win\ws\build\ignition-sensors4\src\ignition-sensors4-rendering.vcxproj]
[120.703s] C:\Jenkins\workspace\ign_sensors-pr-win\ws\install\ignition-rendering4\include\ignition\rendering4\ignition/rendering/MeshDescriptor.hh(73,38): warning C4251: 'ignition::rendering::v4::MeshDescriptor::subMeshName': class 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>' needs to have dll-interface to be used by clients of struct 'ignition::rendering::v4::MeshDescriptor' (compiling source file C:\Jenkins\workspace\ign_sensors-pr-win\ws\ign-sensors\src\RenderingSensor.cc) [C:\Jenkins\workspace\ign_sensors-pr-win\ws\build\ignition-sensors4\src\ignition-sensors4-rendering.vcxproj]
[120.703s] C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.24.28314\include\xstring(4448): message : see declaration of 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>' (compiling source file C:\Jenkins\workspace\ign_sensors-pr-win\ws\ign-sensors\src\RenderingSensor.cc) [C:\Jenkins\workspace\ign_sensors-pr-win\ws\build\ignition-sensors4\src\ignition-sensors4-rendering.vcxproj]

@iche033
Copy link
Contributor

iche033 commented Nov 12, 2020

Argh, I forgot that #59 still had 4 C4251 warnings coming from ignition::rendering::MeshDescriptor which I couldn't figure out how they slipped through the pragmas.

yeah I noticed that too. Looks like it's from RenderingSensor.cc but also didn't see why the pragmas didn't work there. I thought the easy way to fix it is to update ign-rendering..

@chapulina
Copy link
Contributor

I thought the easy way to fix it is to update ign-rendering..

Yeah I also thought this while working on gazebosim/gz-gui#112 and #59, but the number of warnings on ign-rendering looked a bit overwhelming (almost 20k). So I thought that temporarily suppressing warnings from ign-rendering on the downstream libraries would give us more green builds with lower effort. I haven't actually looked at ign-rendering to triage those warnings though. Maybe it's not so difficult to get rid of all 20k of them 😅

@scpeters
Copy link
Member

I resolved conflicts after merging #60, hoping to see the windows build fixed, but it now has 4 new warnings from ign-rendering

@ahcorde
Copy link
Contributor Author

ahcorde commented Dec 7, 2020

Removed Windows warnings in ign-rendering gazebosim/gz-rendering#183

@ahcorde
Copy link
Contributor Author

ahcorde commented Dec 7, 2020

Do we need a ign-rendering release ?

@chapulina
Copy link
Contributor

Removed Windows warnings in ign-rendering

Thanks for picking that up again! 🙇‍♀️

Do we need a ign-rendering release ?

I don't think that's needed, because we don't generate binaries for Windows. Let's retrigger CI

@ahcorde
Copy link
Contributor Author

ahcorde commented Dec 7, 2020

Windows is green!

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.

Windows is green!

Woohoo! ✔️ Glad you were here to witness this moment 😄

@chapulina chapulina merged commit 3dfb50d into ign-sensors4 Dec 7, 2020
@chapulina chapulina deleted the ahcorde/windows/warnings branch December 7, 2020 22:05
chapulina pushed a commit that referenced this pull request Feb 25, 2021
chapulina added a commit that referenced this pull request Mar 2, 2021
Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Alejandro Hernández Cordero <[email protected]>
chapulina added a commit that referenced this pull request Mar 18, 2021
* Prepare fo 3.2.0 release (#94)

Signed-off-by: Nate Koenig <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>

* Backport #60 from ign-sensors4

Fix macOS/windows tests that failed to load library (#60)

* Add workflow for macos-latest
* Set test env to help find plugins

This fixes tests on macOS and windows that were failing to
find and/or load a sensor component library.

In windows CI and the macOS workflow (which runs `make test`
before `make install`) tests were failing with
the message "Unable to find sensor plugin path".
This is fixed by setting the IGN_PLUGIN_PATH in cmake to
the build folder containing the compiled plugins.

In the macOS jenkins build (which runs `make test` after
`make install`) tests were failing with the message
"SDF sensor type does not match template type". It was
difficult to track down, but it appears to be caused by
a failure to properly dlopen all the shared libraries
linked by the component plugins when a test finds an
installed component library, rather than one from the build
folder. It is fixed by setting DYLD_LIBRARY_PATH to include
the location of the installed libraries.

Fixes #4.

* Remove redundant AddPluginPaths calls from tests

They don't work on windows, so just depend on the
environment variables set in cmake instead.

Signed-off-by: Steve Peters <[email protected]>

* 👩‍🌾 Clear Windows warnings (backport #58) (#102)

Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Alejandro Hernández Cordero <[email protected]>

* Master branch updates (#106)

Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome enhancement New feature or request tests Broken or missing tests / testing infra Windows Windows support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants