Fix windows compilation by setting right visibility attributes #550

j-rivero · 2023-11-07T21:35:30Z

🦟 Bug fix

Fixes Windows compilation after #548

Summary

The PR restores the Windows compilation after it was broken on #548. There was a problem using method visibility declaration instead of class visibility that ended up being not easy to solve without warnings with the version of MSVC in the Jenkins nodes. I added a note and a disable a warning on the code.

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: Jose Luis Rivero <[email protected]>

codecov · 2023-11-07T21:36:13Z

Codecov Report

Merging #550 (4b4b80b) into main (09491f5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

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

@@           Coverage Diff           @@
##             main     #550   +/-   ##
=======================================
  Coverage   83.63%   83.64%           
=======================================
  Files          79       79           
  Lines       10211    10214    +3     
=======================================
+ Hits         8540     8543    +3     
  Misses       1671     1671

Files	Coverage Δ
profiler/src/RemoteryProfilerImpl.cc	`84.13% <ø> (ø)`
src/SignalHandler.cc	`87.75% <ø> (ø)`
testing/include/gz/common/testing/TestPaths.hh	`100.00% <ø> (ø)`
testing/src/CMakeTestPaths.cc	`100.00% <100.00%> (ø)`

testing/include/gz/common/testing/CMakeTestPaths.hh

testing/include/gz/common/testing/TestPaths.hh

testing/include/gz/common/testing/CMakeTestPaths.hh

Co-authored-by: Addisu Z. Taddese <[email protected]> Signed-off-by: Michael Carroll <[email protected]>

testing/include/gz/common/testing/CMakeTestPaths.hh

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

testing/include/gz/common/testing/CMakeTestPaths.hh

Signed-off-by: Jose Luis Rivero <[email protected]>

mjcarroll · 2023-11-08T17:59:57Z

The Windows failure here would indicate to me that the base class constructor isn't getting called?

ProjectSourcePath should only return false in the case that this->projectSourcePath is empty:

gz-common/testing/src/CMakeTestPaths.cc

Lines 26 to 37 in 983a1eb

    
           ////////////////////////////////////////////////// 
        
           bool CMakeTestPaths::ProjectSourcePath(std::string &_sourceDir) 
        
           { 
        
             if (!this->projectSourcePath.empty()) 
        
             { 
        
               _sourceDir = this->projectSourcePath; 
        
               return true; 
        
             } 
        
             return false; 
        
           }

scpeters · 2023-11-08T18:35:18Z

The Windows failure here would indicate to me that the base class constructor isn't getting called?

ProjectSourcePath should only return false in the case that this->projectSourcePath is empty:

gz-common/testing/src/CMakeTestPaths.cc

Lines 26 to 37 in 983a1eb

//////////////////////////////////////////////////

bool CMakeTestPaths::ProjectSourcePath(std::string &_sourceDir)

{

if (!this->projectSourcePath.empty())

{

_sourceDir = this->projectSourcePath;

return true;

}

return false;

}

also I'm not sure why that test works at all since it doesnt use the factory:

https://github.com/gazebosim/gz-common/blob/jrivero/fix_hidden_visibility_win/testing/src/CMakeTestPaths_TEST.cc#L62-L67
gz-common/testing/include/gz/common/testing/CMakeTestPaths.hh

Lines 29 to 30 in c604202

/// It is not intended that users will directly construct this, but rather

/// utilize the TestPathFactory.

scpeters · 2023-11-08T18:33:07Z

testing/include/gz/common/testing/TestPaths.hh


-  /// \brief Path to the root of the project source


restore this comment?

j-rivero · 2023-11-08T18:39:40Z

The Windows failure here would indicate to me that the base class constructor isn't getting called?

Printing a message in the constructor displays that it is being called:

57: [----------] 5 tests from CMakeTestPaths
57: [ RUN      ] CMakeTestPaths.TestingProjectSourceDir
57: [       OK ] CMakeTestPaths.TestingProjectSourceDir (0 ms)
57: [ RUN      ] CMakeTestPaths.ProjectSourcePathUnset
57: Calling the constructor in TestPaths
57: [       OK ] CMakeTestPaths.ProjectSourcePathUnset (0 ms)
57: [ RUN      ] CMakeTestPaths.TestBuildType
57: [       OK ] CMakeTestPaths.TestBuildType (0 ms)
57: [ RUN      ] CMakeTestPaths.ProjectSourcePath
57: Calling the constructor in TestPaths
57: C:\Users\vagrant\code\ws_ionic\src\gz-common\testing\src\CMakeTestPaths_TEST.cc(67): error: Value of: testPaths.ProjectSourcePath(sourceDir)
57:   Actual: false
57: Expected: true
57: [  FAILED  ] CMakeTestPaths.ProjectSourcePath (0 ms)
57: [ RUN      ] CMakeTestPaths.TestTmpPath
57: Calling the constructor in TestPaths
57: [       OK ] CMakeTestPaths.TestTmpPath (0 ms)
57: [----------] 5 tests from CMakeTestPaths (0 ms total)

also I'm not sure why that test works at all since it doesnt use the factory:

Maybe the reason is somehow over here.

mjcarroll · 2023-11-08T18:49:21Z

The way that it should work is that we call the CMakeTestPaths constructor, which delegates to the TestPaths constructor here:

gz-common/testing/include/gz/common/testing/CMakeTestPaths.hh

Line 34 in 983a1eb

public: using TestPaths::TestPaths;

The TestPaths constuctor has a default arg of kTestingProjectSourceDir:

gz-common/testing/include/gz/common/testing/TestPaths.hh

Lines 66 to 68 in 983a1eb

    
           public: GZ_COMMON_TESTING_VISIBLE 
        
                   explicit TestPaths(const std::string &_projectSourcePath = 
        
                       kTestingProjectSourceDir);

Which is a constexpr char, set here:

gz-common/testing/include/gz/common/testing/TestPaths.hh

Line 45 in 983a1eb

constexpr char kTestingProjectSourceDir[] = TESTING_PROJECT_SOURCE_DIR;

Ultimately set via:

gz-common/testing/include/gz/common/testing/TestPaths.hh

Lines 29 to 31 in 983a1eb

    
           #ifndef TESTING_PROJECT_SOURCE_DIR 
        
           #define TESTING_PROJECT_SOURCE_DIR "" 
        
           #endif

TESTING_PROJECT_SOURCE_DIR should be set by gz-cmake when using the gz_build_tests macro: https://github.com/gazebosim/gz-cmake/blob/40db6b2f460f753f3065d48472af8f6b5855cf0a/cmake/GzBuildTests.cmake#L147-L148

j-rivero · 2023-11-08T19:19:24Z

Which is a constexpr char, set here:

If I modified:

diff --git a/testing/include/gz/common/testing/TestPaths.hh b/testing/include/gz/common/testing/TestPaths.hh
index b234a97..5348a1d 100644
--- a/testing/include/gz/common/testing/TestPaths.hh
+++ b/testing/include/gz/common/testing/TestPaths.hh
@@ -27,7 +27,7 @@
 #include "gz/common/testing/Export.hh"

 #ifndef TESTING_PROJECT_SOURCE_DIR
-#define TESTING_PROJECT_SOURCE_DIR ""
+#define TESTING_PROJECT_SOURCE_DIR "foo"
 #endif

 namespace gz::common::testing

The test passes (failed a bit below in other ASSERTs as expected) which looks to me like the constructors are doing their job but we have a problem with TESTING_PROJECT_SOURCE_DIR.

Looking at building logs, I see the definition passed by the compiler when building the test:

C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\bin\HostX64\x64\CL.exe /c /I"C:\Users\vagrant\code\ws_ionic\src\gz-common\include" /I"C:\Users\vagrant\code\ws_ionic\build\gz-common6\include" /I"C:\Users\vagrant\code\ws_ionic\src\gz-common" /I"C:\Users\vagrant\code\ws_ionic\build\gz-common6" /I"C:\Users\vagrant\code\ws_ionic\build\gz-common6\core\include" /I"C:\Users\vagrant\code\ws_ionic\src\gz-common\testing\include" /I"C:\Users\vagrant\code\ws_ionic\build\gz-common6\testing\include" /Zi /W2 /WX- /diagnostics:column /O2 /Ob2 /GL /D _MBCS /D WIN32 /D _WINDOWS /D NDEBUG /D "TESTING_PROJECT_SOURCE_DIR=\"C:/Users/vagrant/code/ws_ionic/src/gz-common\"" /D WIN32_LEAN_AND_MEAN /D NOMINMAX /D "CMAKE_INTDIR=\"Release\"" /Gm- /EHsc /MD /GS /Gy /fp:precise /Zc:wchar_t /Zc:forScope /Zc:inline /GR /std:c++17 /Fo"UNIT_CMakeTestPaths_TEST.dir\Release\\" /Fd"UNIT_CMakeTestPaths_TEST.dir\Release\vc142.pdb" /external:W0 /Gd /TP /errorReport:queue  /external:I "C:/Users/vagrant/code/ws_ionic/install/gz-utils3/include/gz/utils3" /external:I "C:/Users/vagrant/code/ws_ionic/src/gz-common/test/gtest_vendor/include" "C:\Users\vagrant\code\ws_ionic\src\gz-common\testing\src\CMakeTestPaths_TEST.cc"
  Microsoft (R) C/C++ Optimizing Compiler Version 19.29.30151 for x64
  Copyright (C) Microsoft Corporation.  All rights reserved.
  CMakeTestPaths_TEST.cc
  cl /c /I"C:\Users\vagrant\code\ws_ionic\src\gz-common\include" /I"C:\Users\vagrant\code\ws_ionic\build\gz-common6\include" /I"C:\Users\vagrant\code\ws_ionic\src\gz-common" /I"C:\Users\vagrant\code\ws_ionic\build\gz-common6" /I"C:\Users\vagrant\code\ws_ionic\build\gz-common6\core\include" /I"C:\Users\vagrant\code\ws_ionic\src\gz-common\testing\include" /I"C:\Users\vagrant\code\ws_ionic\build\gz-common6\testing\include" /Zi /W2 /WX- /diagnostics:column /O2 /Ob2 /GL /D _MBCS /D WIN32 /D _WINDOWS /D NDEBUG /D "TESTING_PROJECT_SOURCE_DIR=\"C:/Users/vagrant/code/ws_ionic/src/gz-common\"" /D WIN32_LEAN_AND_MEAN /D NOMINMAX /D "CMAKE_INTDIR=\"Release\"" /Gm- /EHsc /MD /GS /Gy /fp:precise /Zc:wchar_t /Zc:forScope /Zc:inline /GR /std:c++17 /Fo"UNIT_CMakeTestPaths_TEST.dir\Release\\" /Fd"UNIT_CMakeTestPaths_TEST.dir\Release\vc142.pdb" /external:W0 /Gd /TP /errorReport:queue  /external:I "C:/Users/vagrant/code/ws_ionic/install/gz-utils3/include/gz/utils3" /external:I "C:/Users/vagrant/code/ws_ionic/src/gz-common/test/gtest_vendor/include" "C:\Users\vagrant\code\ws_ionic\src\gz-common\testing\src\CMakeTestPaths_TEST.cc"
Link:
  C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\bin\HostX64\x64\link.exe /ERRORREPORT:QUEUE /OUT:"C:\Users\vagrant\code\ws_ionic\build\gz-common6\bin\Release\UNIT_CMakeTestPaths_TEST.exe" /INCREMENTAL:NO /NOLOGO ..\..\lib\Release\gtest.lib ..\..\lib\Release\gtest_main.lib "..\..\lib\Release\gz-common6-testing.lib" ..\..\lib\Release\gtest.lib "..\..\lib\Release\gz-common6.lib" "C:\Users\vagrant\code\ws_ionic\install\gz-utils3\lib\gz-utils3.lib" kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST /MANIFESTUAC:"level='asInvoker' uiAccess='false'" /manifest:embed /PDB:"C:/Users/vagrant/code/ws_ionic/build/gz-common6/bin/Release/UNIT_CMakeTestPaths_TEST.pdb" /SUBSYSTEM:CONSOLE /TLBID:1 /DYNAMICBASE /NXCOMPAT /IMPLIB:"C:/Users/vagrant/code/ws_ionic/build/gz-common6/lib/Release/UNIT_CMakeTestPaths_TEST.lib" /MACHINE:X64  /machine:x64 UNIT_CMakeTestPaths_TEST.dir\Release\CMakeTestPaths_TEST.obj

Could it be that somehow the binary symbols generated by the TestPaths.hh are ended up in the testing library with an empty TESTING_PROJECT_SOURCE_DIR at that point and being linked in the test? Checking the symbols on Linux:

s_ionic_visibility/build/gz-common6 via △ v3.27.7 ❯ nm -D --demangle ./lib/libgz-common6-testing.so | grep TestPath
0000000000004cf0 T gz::common::testing::BazelTestPaths::TestTmpPath(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)
0000000000004de0 T gz::common::testing::BazelTestPaths::ProjectSourcePath(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)
0000000000004cd0 T gz::common::testing::BazelTestPaths::~BazelTestPaths()
0000000000004cb0 T gz::common::testing::BazelTestPaths::~BazelTestPaths()
0000000000004cb0 T gz::common::testing::BazelTestPaths::~BazelTestPaths()
00000000000053c0 T gz::common::testing::CMakeTestPaths::TestTmpPath(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)
0000000000005380 T gz::common::testing::CMakeTestPaths::ProjectSourcePath(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)
0000000000005360 T gz::common::testing::CMakeTestPaths::~CMakeTestPaths()
0000000000005340 T gz::common::testing::CMakeTestPaths::~CMakeTestPaths()
0000000000005340 T gz::common::testing::CMakeTestPaths::~CMakeTestPaths()
0000000000006650 T gz::common::testing::TestPathFactory(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)
00000000000062f0 T gz::common::testing::TestPaths::TestPaths(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)
00000000000062f0 T gz::common::testing::TestPaths::TestPaths(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)
00000000000064c0 T gz::common::testing::TestPaths::~TestPaths()
0000000000006480 T gz::common::testing::TestPaths::~TestPaths()
0000000000006480 T gz::common::testing::TestPaths::~TestPaths()
0000000000006940 W std::unique_ptr<gz::common::testing::TestPaths, std::default_delete<gz::common::testing::TestPaths> >::~unique_ptr()
0000000000006940 W std::unique_ptr<gz::common::testing::TestPaths, std::default_delete<gz::common::testing::TestPaths> >::~unique_ptr()
000000000000ac28 V typeinfo for gz::common::testing::CMakeTestPaths
000000000000abd0 V typeinfo for gz::common::testing::TestPaths
00000000000080a0 V typeinfo name for gz::common::testing::CMakeTestPaths
0000000000008030 V typeinfo name for gz::common::testing::TestPaths
000000000000ac40 V vtable for gz::common::testing::CMakeTestPaths
000000000000acb0 V vtable for gz::common::testing::TestPaths

mjcarroll · 2023-11-08T19:22:30Z

Could it be that somehow the binary symbols generated by the TestPaths.hh are ended up in the testing library with an empty TESTING_PROJECT_SOURCE_DIR at that point and being linked in the test?

maybe? It's just strange that the visibility change caused this to surface.

azeey · 2023-11-09T00:34:08Z

Looking at the build rules from Ninja

build testing\src\CMakeFiles\gz-common6-testing.dir\CMakeTestPaths.cc.obj: CXX_COMPILER__gz-common6-testing_unscanned_Debug C$:\Users\Administrator\ws\ionic\src\gz-common\testing\src\CMakeTestPaths.cc || cmake_object_order_depends_target_gz-common6-testing
  DEFINES = -DNOMINMAX -DWIN32_LEAN_AND_MEAN -Dgz_common6_testing_EXPORTS
  FLAGS = /DWIN32 /D_WINDOWS /W3 /GR /EHsc /MDd /Zi /Ob0 /Od /RTC1 /Gy /W2 /Zi -std:c++17 /EHsc
  INCLUDES = -IC:\Users\Administrator\ws\ionic\src\gz-common\include -IC:\Users\Administrator\ws\ionic\build\gz-common6\include -IC:\Users\Administrator\ws\ionic\src\gz-common\testing\include -IC:\Users\Administrator\ws\ionic\build\gz-common6\testing\include -IC:\Users\Administrator\ws\ionic\build\gz-common6\core\include -external:IC:\Users\Administrator\ws\ionic\install\include\gz\utils3 -external:W0
  OBJECT_DIR = testing\src\CMakeFiles\gz-common6-testing.dir
  OBJECT_FILE_DIR = testing\src\CMakeFiles\gz-common6-testing.dir
  TARGET_COMPILE_PDB = testing\src\CMakeFiles\gz-common6-testing.dir\
  TARGET_PDB = bin\gz-common6-testing.pdb


build testing\src\CMakeFiles\UNIT_CMakeTestPaths_TEST.dir\CMakeTestPaths_TEST.cc.obj: CXX_COMPILER__UNIT_CMakeTestPaths_TEST_unscanned_Debug C$:\Users\Administrator\ws\ionic\src\gz-common\testing\src\CMakeTestPaths_TEST.cc || cmake_object_order_depends_target_UNIT_CMakeTestPaths_TEST
  DEFINES = -DNOMINMAX -DTESTING_PROJECT_SOURCE_DIR=\"C:/Users/Administrator/ws/ionic/src/gz-common\" -DWIN32_LEAN_AND_MEAN
  FLAGS = /DWIN32 /D_WINDOWS /W3 /GR /EHsc /MDd /Zi /Ob0 /Od /RTC1 /Gy /W2 /Zi -std:c++17 /EHsc
  INCLUDES = -IC:\Users\Administrator\ws\ionic\src\gz-common\include -IC:\Users\Administrator\ws\ionic\build\gz-common6\include -IC:\Users\Administrator\ws\ionic\src\gz-common -IC:\Users\Administrator\ws\ionic\build\gz-common6 -IC:\Users\Administrator\ws\ionic\build\gz-common6\core\include -IC:\Users\Administrator\ws\ionic\src\gz-common\testing\include -IC:\Users\Administrator\ws\ionic\build\gz-common6\testing\include -external:IC:\Users\Administrator\ws\ionic\install\include\gz\utils3 -external:IC:\Users\Administrator\ws\ionic\src\gz-common\test\gtest_vendor\include -external:W0
  OBJECT_DIR = testing\src\CMakeFiles\UNIT_CMakeTestPaths_TEST.dir
  OBJECT_FILE_DIR = testing\src\CMakeFiles\UNIT_CMakeTestPaths_TEST.dir
  TARGET_COMPILE_PDB = testing\src\CMakeFiles\UNIT_CMakeTestPaths_TEST.dir\
  TARGET_PDB = bin\UNIT_CMakeTestPaths_TEST.pdb

When CMakeTestPaths.cc is built, TESTING_PROJECT_SOURCE_DIR is not set (because it's not a test). And UNIT_CMakeTestPaths_TEST.exe is built from CMakeTestPaths_TEST.cc and linkded against lib\gz-common6-testing.lib. When calling the CMakeTestPaths constructor, it's going to be calling the one in the library, which doesn't have TESTING_PROJECT_SOURCE_DIR defined. The question is how did this work before the visibility fixes? Is there some sort of inlining happening?

scpeters · 2023-11-09T05:09:22Z

for now I've proposed to revert #548 until we can figure out the windows CI issues: see #553

mjcarroll · 2023-11-09T14:26:08Z

The question is how did this work before the visibility fixes? Is there some sort of inlining happening?

It seems to work on other platforms as well, so maybe there is a difference in the way that inlining is happening.

mjcarroll · 2023-11-09T14:28:51Z

When calling the CMakeTestPaths constructor, it's going to be calling the one in the library,

When i put this together, I think the goal was for the constructor to be only defined in the header such that when it was called, it would use the preprocessor definition at the point that it was used. This seems to have worked up until now, maybe there is a way to make this more robust?

azeey · 2023-11-09T15:18:04Z

When i put this together, I think the goal was for the constructor to be only defined in the header such that when it was called, it would use the preprocessor definition at the point that it was used. This seems to have worked up until now, maybe there is a way to make this more robust?

Yeah, this is puzzling. My understanding was that default arguments are handled at compile time, not at link time, so I would have thought that the TESTING_PROJECT_SOURCE_DIR from CMakeTestPaths_TEST.cc's flags would have been used when calling the CMakeTestPaths constructor. I wonder if the issue is with kTestingProjectSourceDir, because that's a symbol. I'm going to try TestPaths(const std::string &_projectSourcePath = TESTING_PROJECT_SOURCE_DIR);

mjcarroll · 2023-11-09T15:23:03Z

TestPaths(const std::string &_projectSourcePath = TESTING_PROJECT_SOURCE_DIR);

This seems like a reasonable approach, not sure why I didn't do it this way before (maybe because it just worked with the symbol?)

Signed-off-by: Jose Luis Rivero <[email protected]>

azeey · 2023-11-09T15:51:29Z

This seems like a reasonable approach, not sure why I didn't do it this way before (maybe because it just worked with the symbol?)

Well, that didn't work.

The Windows failure here would indicate to me that the base class constructor isn't getting called?

I added a print statement in the base constructor and it didn't get called, so that explains some things.

Signed-off-by: Jose Luis Rivero <[email protected]> Signed-off-by: Michael Carroll <[email protected]> Co-authored-by: Jose Luis Rivero <[email protected]>

Signed-off-by: Michael Carroll <[email protected]>

scpeters · 2023-11-09T23:16:08Z

how do you feel about cherry-picking #549 and reapplying #548 in this branch?

testing/include/gz/common/testing/TestPaths.hh

Signed-off-by: Michael Carroll <[email protected]>

This reverts commit 7eb756f.

Signed-off-by: Jose Luis Rivero <[email protected]>

Signed-off-by: Michael Carroll <[email protected]>

mjcarroll · 2023-11-10T15:00:46Z

how do you feel about cherry-picking #549 and reapplying #548 in this branch?

Done

mjcarroll · 2023-11-10T15:03:12Z

Setting DCO to pass, I think enough merge/rebase/cherry-pick has happened here to confuse the checker.

j-rivero · 2023-11-10T16:02:57Z

Good to go. Thanks @mjcarroll @azeey

j-rivero added 3 commits November 6, 2023 20:02

Remove broken visibility attributes. Fix Windows

1af6703

Signed-off-by: Jose Luis Rivero <[email protected]>

Fix Windows visibility for Testing class

44f77e4

Signed-off-by: Jose Luis Rivero <[email protected]>

Ignore warning for the current version of MSVC

c2cbfbf

Signed-off-by: Jose Luis Rivero <[email protected]>

j-rivero requested a review from marcoag as a code owner November 7, 2023 21:35

mjcarroll reviewed Nov 7, 2023

View reviewed changes

mjcarroll mentioned this pull request Nov 8, 2023

port: 8 to main gazebosim/gz-gui#593

Merged

azeey reviewed Nov 8, 2023

View reviewed changes

testing/include/gz/common/testing/CMakeTestPaths.hh Outdated Show resolved Hide resolved

Apply suggestions from code review

f4b52f8

Co-authored-by: Addisu Z. Taddese <[email protected]> Signed-off-by: Michael Carroll <[email protected]>

mjcarroll force-pushed the jrivero/fix_hidden_visibility_win branch from 662af67 to f4b52f8 Compare November 8, 2023 16:21

iche033 mentioned this pull request Nov 8, 2023

Use sdf FindElement API to avoid const_cast gazebosim/gz-sim#2231

Merged

8 tasks

iche033 reviewed Nov 8, 2023

View reviewed changes

testing/include/gz/common/testing/CMakeTestPaths.hh Outdated Show resolved Hide resolved

fix pragma warning pop

b0394a5

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

mjcarroll reviewed Nov 8, 2023

View reviewed changes

testing/include/gz/common/testing/CMakeTestPaths.hh Outdated Show resolved Hide resolved

Add MSVC_VER for pragma pop

c604202

Signed-off-by: Jose Luis Rivero <[email protected]>

scpeters reviewed Nov 8, 2023

View reviewed changes

testing/include/gz/common/testing/TestPaths.hh

/// \brief Path to the root of the project source

Copy link

Member

scpeters Nov 8, 2023

Choose a reason for hiding this comment

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

restore this comment?

Use TestFactory for CMakeTestPath

34455da

Signed-off-by: Jose Luis Rivero <[email protected]>

mjcarroll and others added 2 commits November 9, 2023 16:41

Fix hidden visibility win fix test (#557)

93f2da7

Signed-off-by: Jose Luis Rivero <[email protected]> Signed-off-by: Michael Carroll <[email protected]> Co-authored-by: Jose Luis Rivero <[email protected]>

Merge branch 'main' into jrivero/fix_hidden_visibility_win

50cfc6b

mjcarroll requested review from azeey and scpeters November 9, 2023 22:43

Minimize diff

e61ebc1

Signed-off-by: Michael Carroll <[email protected]>

azeey reviewed Nov 9, 2023

View reviewed changes

testing/include/gz/common/testing/TestPaths.hh Outdated Show resolved Hide resolved

mjcarroll and others added 5 commits November 9, 2023 17:30

Remove deleted destructor

7eb756f

Signed-off-by: Michael Carroll <[email protected]>

Revert "Remove deleted destructor"

24826b7

This reverts commit 7eb756f.

Enable HIDE_SYMBOLS_BY_DEFAULT

52a7816

Signed-off-by: Jose Luis Rivero <[email protected]>

Add missing visibility declarations (#548)

56bd6fd

Signed-off-by: Jose Luis Rivero <[email protected]>

Change pure virtual to virtual destructor

097ee0b

Signed-off-by: Michael Carroll <[email protected]>

Merge branch 'main' into jrivero/fix_hidden_visibility_win

6d81bba

azeey approved these changes Nov 10, 2023

View reviewed changes

mjcarroll merged commit 75277ae into main Nov 10, 2023
13 checks passed

mjcarroll deleted the jrivero/fix_hidden_visibility_win branch November 10, 2023 16:03

mjcarroll mentioned this pull request Nov 10, 2023

Enable HIDE_SYMBOLS_BY_DEFAULT #549

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix windows compilation by setting right visibility attributes #550

Fix windows compilation by setting right visibility attributes #550

j-rivero commented Nov 7, 2023

codecov bot commented Nov 7, 2023 •

edited

Loading

mjcarroll commented Nov 8, 2023 •

edited

Loading

scpeters commented Nov 8, 2023

scpeters Nov 8, 2023

j-rivero commented Nov 8, 2023

mjcarroll commented Nov 8, 2023 •

edited

Loading

j-rivero commented Nov 8, 2023

mjcarroll commented Nov 8, 2023

azeey commented Nov 9, 2023

scpeters commented Nov 9, 2023

mjcarroll commented Nov 9, 2023

mjcarroll commented Nov 9, 2023

azeey commented Nov 9, 2023

mjcarroll commented Nov 9, 2023

azeey commented Nov 9, 2023

scpeters commented Nov 9, 2023

mjcarroll commented Nov 10, 2023

mjcarroll commented Nov 10, 2023

j-rivero commented Nov 10, 2023

Fix windows compilation by setting right visibility attributes #550

Fix windows compilation by setting right visibility attributes #550

Conversation

j-rivero commented Nov 7, 2023

🦟 Bug fix

Summary

Checklist

codecov bot commented Nov 7, 2023 • edited Loading

Codecov Report

mjcarroll commented Nov 8, 2023 • edited Loading

scpeters commented Nov 8, 2023

scpeters Nov 8, 2023

Choose a reason for hiding this comment

j-rivero commented Nov 8, 2023

mjcarroll commented Nov 8, 2023 • edited Loading

j-rivero commented Nov 8, 2023

mjcarroll commented Nov 8, 2023

azeey commented Nov 9, 2023

scpeters commented Nov 9, 2023

mjcarroll commented Nov 9, 2023

mjcarroll commented Nov 9, 2023

azeey commented Nov 9, 2023

mjcarroll commented Nov 9, 2023

azeey commented Nov 9, 2023

scpeters commented Nov 9, 2023

mjcarroll commented Nov 10, 2023

mjcarroll commented Nov 10, 2023

j-rivero commented Nov 10, 2023

codecov bot commented Nov 7, 2023 •

edited

Loading

mjcarroll commented Nov 8, 2023 •

edited

Loading

mjcarroll commented Nov 8, 2023 •

edited

Loading