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

Fixed build on MSVC #897

Merged
merged 4 commits into from
Sep 14, 2023
Merged

Fixed build on MSVC #897

merged 4 commits into from
Sep 14, 2023

Conversation

peci1
Copy link
Contributor

@peci1 peci1 commented Sep 3, 2023

🦟 Bug fix

Summary

This was needed for the MSVC build to succeed. @darksylinc got obviously hit again by the poor MSVC linker that is not able to figure out it doesn't need to lookup defaulted constructors/destructors.

The CMake flag was needed to support building with VS2022. VS2019 shouldn't require it.

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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.

Signed-off-by: Martin Pecka <[email protected]>
@peci1 peci1 requested a review from iche033 as a code owner September 3, 2023 17:33
@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Sep 3, 2023
@peci1
Copy link
Contributor Author

peci1 commented Sep 3, 2023

Please note the buildfarm uses vcpkg for fingind Ogre2 and doesn't find it, so the buildfarm actually won't test the proposed changes on Windows.

@codecov
Copy link

codecov bot commented Sep 3, 2023

Codecov Report

Merging #897 (962dc65) into gz-rendering8 (98c5978) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

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

@@                Coverage Diff                @@
##           gz-rendering8     #897      +/-   ##
=================================================
- Coverage          75.21%   75.18%   -0.04%     
=================================================
  Files                176      177       +1     
  Lines              16839    16846       +7     
=================================================
  Hits               12665    12665              
- Misses              4174     4181       +7     
Files Changed Coverage Δ
include/gz/rendering/base/BaseNativeWindow.hh 0.00% <ø> (ø)
ogre2/src/Ogre2GlobalIlluminationCiVct.cc 0.00% <0.00%> (ø)
ogre2/src/Ogre2RenderEngine.cc 68.18% <ø> (ø)
src/GlobalIlluminationBase.cc 0.00% <0.00%> (ø)
src/GlobalIlluminationCiVct.cc 0.00% <0.00%> (ø)
src/GlobalIlluminationVct.cc 0.00% <0.00%> (ø)
src/NativeWindow.cc 0.00% <0.00%> (ø)
src/base/BaseNativeWindow.cc 0.00% <0.00%> (ø)

@iche033
Copy link
Contributor

iche033 commented Sep 6, 2023

changes look good to me, will wait for CI results from #899

@mjcarroll
Copy link
Contributor

@peci1 I'm now getting around to looking at this locally, I seem to have some issues with gtest const_cast with VS2022. Have you seen anything to this effect?

@mjcarroll
Copy link
Contributor

Specifically:

[1/66] Building CXX object test\gtest_vendor\CMakeFiles\gtest.dir\src\gtest-all.cc.obj
FAILED: test/gtest_vendor/CMakeFiles/gtest.dir/src/gtest-all.cc.obj
C:\Users\carro\buildcache\bin\buildcache.exe C:\PROGRA~1\MICROS~1\2022\COMMUN~1\VC\Tools\MSVC\1436~1.325\bin\Hostx64\x64\cl.exe  /nologo /TP -DNOMINMAX -DWIN32_LEAN_AND_MEAN -IC:\Users\carro\gazebodistro\.pixi\env\Library\include -IC:\Users\carro\gazebodistro\workspace\src\gz-rendering\include -IC:\Users\carro\gazebodistro\workspace\build\gz-rendering8\include -external:IC:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\include -external:IC:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor -external:IC:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\src -external:W0 /DWIN32 /D_WINDOWS /W3 /GR /EHsc /MD /O2 /Ob2 /DNDEBUG /Gy /W2 /Zi /GL -std:c++17 /EHsc /showIncludes /Fotest\gtest_vendor\CMakeFiles\gtest.dir\src\gtest-all.cc.obj /Fdtest\gtest_vendor\CMakeFiles\gtest.dir\gtest.pdb /FS -c C:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\src\gtest-all.cc
cl : Command line warning D9025 : overriding '/W3' with '/W2'
C:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\src/gtest-port.cc(924): error C2440: 'const_cast': cannot convert from 'std::string' to 'char *'
C:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\src/gtest-port.cc(924): note: Conversion requires a constructor or user-defined-conversion operator, which can't be used by const_cast or reinterpret_cast
C:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\src/gtest-port.cc(925): error C2440: 'const_cast': cannot convert from 'std::string' to 'char *'
C:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\src/gtest-port.cc(925): note: Conversion requires a constructor or user-defined-conversion operator, which can't be used by const_cast or reinterpret_cast
C:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\src/gtest-port.cc(930): error C2664: 'bool testing::internal::MatchRegexAnywhere(const char *,const char *)': cannot convert argument 1 from 'const std::string' to 'const char *'
C:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\src/gtest-port.cc(930): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
C:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\src/gtest-port.cc(909): note: see declaration of 'testing::internal::MatchRegexAnywhere'
C:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\src/gtest-port.cc(930): note: while trying to match the argument list '(const std::string, const char *)'
C:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\src/gtest-port.cc(936): error C2664: 'bool testing::internal::MatchRegexAnywhere(const char *,const char *)': cannot convert argument 1 from 'const std::string' to 'const char *'
C:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\src/gtest-port.cc(936): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
C:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\src/gtest-port.cc(909): note: see declaration of 'testing::internal::MatchRegexAnywhere'
C:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\src/gtest-port.cc(936): note: while trying to match the argument list '(const std::string, const char *)'
C:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\src/gtest-port.cc(943): error C2039: 'StrDup': is not a member of 'testing::internal::posix'
C:\Users\carro\gazebodistro\.pixi\env\Library\include\gtest/internal/gtest-port.h(1987): note: see declaration of 'testing::internal::posix'
C:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\src/gtest-port.cc(943): error C3861: 'StrDup': identifier not found
C:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\src/gtest.cc(2439): error C2511: 'void testing::Test::RecordProperty(const std::string &,int)': overloaded member function not found in 'testing::Test'
C:\Users\carro\gazebodistro\.pixi\env\Library\include\gtest/gtest.h(243): note: see declaration of 'testing::Test'
C:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\src/gtest.cc(5118): error C2511: 'void testing::TestEventListeners::SuppressEventForwarding(void)': overloaded member function not found in 'testing::TestEventListeners'
C:\Users\carro\gazebodistro\.pixi\env\Library\include\gtest/gtest.h(1023): note: see declaration of 'testing::TestEventListeners'
C:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\src/gtest.cc(5580): error C2660: 'testing::TestEventListeners::SuppressEventForwarding': function does not take 0 arguments
C:\Users\carro\gazebodistro\.pixi\env\Library\include\gtest/gtest.h(1060): note: see declaration of 'testing::TestEventListeners::SuppressEventForwarding'
C:\Users\carro\gazebodistro\workspace\src\gz-rendering\test\gtest_vendor\src/gtest.cc(5580): note: while trying to match the argument list '()'
ninja: build stopped: subcommand failed.
PS C:\Users\carro\gazebodistro\workspace\build\gz-rendering8>

@mjcarroll
Copy link
Contributor

Oh very strange, this is exactly the same as other packages, but not building here.

@peci1
Copy link
Contributor Author

peci1 commented Sep 14, 2023

I thought tests were disabled on Win. I did not encounter this error.

@mjcarroll
Copy link
Contributor

I thought tests were disabled on Win. I did not encounter this error.

Not if you are brave :) I'll poke at it some more. The gz-cmake fix looked good, so I merged it.

@mjcarroll
Copy link
Contributor

I determined it was some CMake path priority silliness, unrelated to this PR.

@mjcarroll mjcarroll merged commit 0795e09 into gazebosim:gz-rendering8 Sep 14, 2023
@scpeters
Copy link
Member

this has broken the macOS build:

ogre2/src/Ogre2RenderEngine.cc:1348:30: error: use of undeclared identifier 'CGLGetCurrentContext'
      std::to_string((size_t)CGLGetCurrentContext());
                             ^
1 error generated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants