Windows Tests #1109

varunagrawal · 2022-02-17T16:19:26Z

Thanks to the awesome work by @mikesheffler, I was finally able to figure out what was the issue behind all the "undefined" errors.

This PR adds/removes all the necessary GTSAM_EXPORT declarations and updates the CI to run all GTSAM test targets. Unfortunately some targets have test failures due to bugs creeping in over time and the CI not being enabled for them, but those can be tackled later.

I am leaving GTSAM_UNSTABLE test targets for another day.

dellaert

Awesome (if windows CI actually passes :-))

dellaert · 2022-02-17T16:26:50Z

gtsam/geometry/Line3.cpp

dellaert · 2022-02-17T16:27:43Z

gtsam/geometry/Line3.h

@@ -146,7 +146,30 @@ class Line3 {
 */
 Line3 transformTo(const Pose3 &wTc, const Line3 &wL,
                  OptionalJacobian<4, 6> Dpose = boost::none,
-                  OptionalJacobian<4, 4> Dline = boost::none);
+                  OptionalJacobian<4, 4> Dline = boost::none) {


I had trouble with definitions in headers. Should be inline? But I don't understand why we can't just export here and leave definition in .cpp?

dellaert · 2022-02-17T16:29:58Z

gtsam/navigation/tests/testSerializationNavigation.cpp

@@ -39,6 +39,9 @@ BOOST_CLASS_EXPORT_GUID(noiseModel::Unit, "gtsam_noiseModel_Unit")
 BOOST_CLASS_EXPORT_GUID(noiseModel::Isotropic, "gtsam_noiseModel_Isotropic")
 BOOST_CLASS_EXPORT_GUID(SharedNoiseModel, "gtsam_SharedNoiseModel")
 BOOST_CLASS_EXPORT_GUID(SharedDiagonal, "gtsam_SharedDiagonal")
+BOOST_CLASS_EXPORT_GUID(PreintegratedImuMeasurements, "gtsam_PreintegratedImuMeasurements")


should be gtsam::PreintegratedImuMeasurements I think, not _.
We should really standardize (different issue, different PR, different contributor)

varunagrawal · 2022-02-17T17:54:30Z

I forgot to comment out the gtsam_unstable test build. Windows CI will fail for that reason.

dellaert

Changing to "request changes" as I think that function should not be moved out of cpp, unless you can tell me the reason :-)

varunagrawal · 2022-02-17T22:20:31Z

The linker seems to confuse the transformTo method for Line3 with the expressions version, this throwing a linker error. Another solution would be to rename the expression version to transformTo_ but that would be API breaking?

dellaert · 2022-02-17T23:32:52Z

The linker seems to confuse the transformTo method for Line3 with the expressions version, this throwing a linker error. Another solution would be to rename the expression version to transformTo_ but that would be API breaking?

That is very weird. Error message? CI seems to pass, so is it check.geometry that does not pass?

mikesheffler · 2022-02-18T00:55:46Z

Just as a datapoint, I currently have (for fix/windows-tests, and without attempting any gtsam_unstable stuff):

Pass

check.base
check.basis
check.inference
check.navigation
check.sfm
check.symbolic

Fail

check.discrete
check.geometry
check.nonlinear
check.partition
check.sam
check.slam
check.tests

I thought check.discrete was supposed to be in the 'Pass' category, so I'll print what I have on my side. I should be able to look at this later tonight.

Possibly relevant warning:

EXPECT(adds = 54);

results in

Assignment '=' used when equality '==' probably intended

at testAlgebraicDecisionTree.cpp line 321.

`check.discrete` Build Errors

Looks like multiply-defined symbols:

Build started...
1>------ Build started: Project: check_discrete_program, Configuration: Debug x64 ------
1>gtsamDebug.lib(gtsamDebug.dll) : error LNK2005: "private: static class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl gtsam::AlgebraicDecisionTree<unsigned __int64>::DefaultFormatter(unsigned __int64 const &)" (?DefaultFormatter@?$AlgebraicDecisionTree@_K@gtsam@@CA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@AEB_K@Z) already defined in testAlgebraicDecisionTree.obj
1>gtsamDebug.lib(gtsamDebug.dll) : error LNK2005: "public: __cdecl gtsam::AlgebraicDecisionTree<unsigned __int64>::AlgebraicDecisionTree<unsigned __int64>(class std::vector<struct std::pair<unsigned __int64,unsigned __int64>,class std::allocator<struct std::pair<unsigned __int64,unsigned __int64> > > const &,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &)" (??0?$AlgebraicDecisionTree@_K@gtsam@@QEAA@AEBV?$vector@U?$pair@_K_K@std@@V?$allocator@U?$pair@_K_K@std@@@2@@std@@AEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@3@@Z) already defined in testAlgebraicDecisionTree.obj
1>gtsamDebug.lib(gtsamDebug.dll) : error LNK2005: "public: void __cdecl gtsam::AlgebraicDecisionTree<unsigned __int64>::print(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,class std::function<class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl(unsigned __int64)> const &)const " (?print@?$AlgebraicDecisionTree@_K@gtsam@@QEBAXAEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@AEBV?$function@$$A6A?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@_K@Z@4@@Z) already defined in testAlgebraicDecisionTree.obj
1>gtsamDebug.lib(gtsamDebug.dll) : error LNK2005: "public: bool __cdecl gtsam::AlgebraicDecisionTree<unsigned __int64>::equals(class gtsam::AlgebraicDecisionTree<unsigned __int64> const &,double)const " (?equals@?$AlgebraicDecisionTree@_K@gtsam@@QEBA_NAEBV12@N@Z) already defined in testAlgebraicDecisionTree.obj
1>gtsamDebug.lib(gtsamDebug.dll) : error LNK2005: "public: virtual __cdecl gtsam::AlgebraicDecisionTree<unsigned __int64>::~AlgebraicDecisionTree<unsigned __int64>(void)" (??1?$AlgebraicDecisionTree@_K@gtsam@@UEAA@XZ) already defined in testAlgebraicDecisionTree.obj
1>gtsamDebug.lib(gtsamDebug.dll) : error LNK2005: "public: __cdecl gtsam::AlgebraicDecisionTree<unsigned __int64>::AlgebraicDecisionTree<unsigned __int64>(class gtsam::AlgebraicDecisionTree<unsigned __int64> const &)" (??0?$AlgebraicDecisionTree@_K@gtsam@@QEAA@AEBV01@@Z) already defined in testAlgebraicDecisionTree.obj
1>gtsamDebug.lib(gtsamDebug.dll) : error LNK2005: "public: __cdecl gtsam::AlgebraicDecisionTree<unsigned __int64>::AlgebraicDecisionTree<unsigned __int64>(struct std::pair<unsigned __int64,unsigned __int64> const &,double,double)" (??0?$AlgebraicDecisionTree@_K@gtsam@@QEAA@AEBU?$pair@_K_K@std@@NN@Z) already defined in testAlgebraicDecisionTree.obj
1>gtsamDebug.lib(gtsamDebug.dll) : error LNK2005: "public: __cdecl gtsam::AlgebraicDecisionTree<unsigned __int64>::AlgebraicDecisionTree<unsigned __int64>(class std::vector<struct std::pair<unsigned __int64,unsigned __int64>,class std::allocator<struct std::pair<unsigned __int64,unsigned __int64> > > const &,class std::vector<double,class std::allocator<double> > const &)" (??0?$AlgebraicDecisionTree@_K@gtsam@@QEAA@AEBV?$vector@U?$pair@_K_K@std@@V?$allocator@U?$pair@_K_K@std@@@2@@std@@AEBV?$vector@NV?$allocator@N@std@@@3@@Z) already defined in testAlgebraicDecisionTree.obj
1>C:\Mike\Work\Build\gtsam\bin\Debug\check_discrete_program.exe : fatal error LNK1169: one or more multiply defined symbols found
1>Done building project "check_discrete_program.vcxproj" -- FAILED.
2>------ Build started: Project: check.discrete, Configuration: Debug x64 ------
2>Test project C:/Mike/Work/Build/gtsam/gtsam/discrete/tests
2>    Start 1: check_discrete_program
2>Could not find executable C:/Mike/Work/Build/gtsam/bin/Debug/check_discrete_program.exe
2>Looked in the following places:
2>C:/Mike/Work/Build/gtsam/bin/Debug/check_discrete_program.exe
2>C:/Mike/Work/Build/gtsam/bin/Debug/check_discrete_program.exe.exe
2>C:/Mike/Work/Build/gtsam/bin/Debug/Debug/check_discrete_program.exe
2>C:/Mike/Work/Build/gtsam/bin/Debug/Debug/check_discrete_program.exe.exe
2>Debug/C:/Mike/Work/Build/gtsam/bin/Debug/check_discrete_program.exe
2>Debug/C:/Mike/Work/Build/gtsam/bin/Debug/check_discrete_program.exe.exe
2>Unable to find executable: C:/Mike/Work/Build/gtsam/bin/Debug/check_discrete_program.exe
2>1/1 Test #1: check_discrete_program ...........***Not Run   0.00 sec
2>
2>0% tests passed, 1 tests failed out of 1

dellaert · 2022-02-18T01:53:23Z

Definitely should be ==

About ADT: could be template instantiation.

mikesheffler · 2022-02-18T11:51:44Z

About ADT: could be template instantiation.

I think so? As the code says, it (ADT) mostly exists to specialize one template argument to DecisionTree as double.

I tried sprinkling inline here and there in the ADT header on the methods that were complaining about already being defined, but it didn't seem to do anything. Between the static methods, the inlines and the inheritance, I can't seem to follow it all at once.

Basically, as soon as you have two ADT instances in testAlgebraicDecisionTree.cpp, you'll get the whole raft of errors, so it's probably something about the whole class. Dunno. 😦

varunagrawal · 2022-02-18T22:06:34Z

@dellaert I am going to park responding to this issue until at least the 24th (RAL+IROS deadline). Don't want you to think I'm being unresponsive. :)

dellaert · 2022-02-18T22:55:38Z

Actually, @mikesheffler , I think the == should be replaced with LONGS_EQUAL(54, adds), and 54 should be changed to the right value ;-)

I think the solution to the ADT linking problem might be to re-add the export, and add a .cpp file with an explicit instantiation on Key. That should create one set of symbols exported to the DLL.

…d of EQUAL -- Please review * Added GTSAM_EXPORT back to to AlgebraicDecisionTree.h and added a .cpp file to accompany the .h. The only contents of the file are the specialization AlgebraicDecisionTree<Key>. This seems to make the linker happy enough to include the symbols that allow the above test to run.

mikesheffler · 2022-02-21T11:01:12Z

tl;dr: check.discrete passes now (in Windows).

I think the == should be replaced with LONGS_EQUAL(54, adds), and 54 should be changed to the right value ;-)

I made the change to LONGS_EQUAL and it looks like the value that the test wants is 49, so I changed it to 49. I haven't made the slightest attempt to understand what's actually going on in the test, so please take a look and decide if that is sensible.

the ADT linking problem might be to re-add the export, and add a .cpp file with an explicit instantiation on Key.

I added back GTSAM_EXPORT and then made the .cpp with just

namespace gtsam {

  template class AlgebraicDecisionTree<Key>;

}  // namespace gtsam

which seems to convince the linker to export the symbols, and just once to boot. I don't have a tight handle on the linker's decision making process here, so it's worth taking a look.

dellaert

Nice!
@mikesheffler, @varunagrawal, I propose to approve this PR and we can address different modules in a different PR? After waiting for @mikesheffler confirmation that he does not have outstanding changes on this PR, I propose @varunagrawal merges.

mikesheffler · 2022-02-22T00:00:40Z

I do not have outstanding changes. I think the idea to chop up the effort makes sense, or this will be unmerged for a while.

dellaert · 2022-02-22T00:26:04Z

Cool. @varunagrawal merge when you're ready

varunagrawal added 13 commits February 17, 2022 03:38

basis module tests pass

6e6c647

update METIS test for _WIN32

b13a219

sfm tests pass

7712158

symbolic tests pass

beeb91a

navigation tests passing

865a7ba

fixed dllexport issues in nonlinear, only tests failing

2e5bdcd

fixed dllexport issues in slam, only tests failing

38425f1

move Line3 transformTo definition to header to resolve ambiguity

1e1e2c2

fixed dllexport issues in geometry, only tests failing

fcd418a

Fix discrete module

566c7b9

documentation updates

770587d

suppress spurious warnings

d860e39

build (almost) all GTSAM test targets in CI

9a234a2

varunagrawal added bugfix Fixes an issue or bug ci Related to the Continuous Integration pipeline labels Feb 17, 2022

varunagrawal requested review from dellaert and mikesheffler February 17, 2022 16:19

varunagrawal self-assigned this Feb 17, 2022

formatting updates

12d1d21

dellaert approved these changes Feb 17, 2022

View reviewed changes

varunagrawal added 2 commits February 17, 2022 14:45

comment out gtsam_unstable test target

6fe55af

Move transformTo declaration to the top to avoid ambiguous linkage

1545d90

dellaert requested changes Feb 17, 2022

View reviewed changes

Assignment accidentally used in place of equality

57a51a7

dellaert approved these changes Feb 21, 2022

View reviewed changes

mikesheffler approved these changes Feb 22, 2022

View reviewed changes

varunagrawal merged commit 267a4ae into develop Feb 22, 2022

varunagrawal deleted the fix/windows-tests branch February 22, 2022 00:27

Tobias-Fischer mentioned this pull request Jul 31, 2022

Fix exports on Windows #1257

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows Tests #1109

Windows Tests #1109

varunagrawal commented Feb 17, 2022

dellaert left a comment

dellaert Feb 17, 2022

dellaert Feb 17, 2022

dellaert Feb 17, 2022

varunagrawal commented Feb 17, 2022

dellaert left a comment

varunagrawal commented Feb 17, 2022

dellaert commented Feb 17, 2022

mikesheffler commented Feb 18, 2022

dellaert commented Feb 18, 2022

mikesheffler commented Feb 18, 2022

varunagrawal commented Feb 18, 2022 •

edited

Loading

dellaert commented Feb 18, 2022

mikesheffler commented Feb 21, 2022 •

edited

Loading

dellaert left a comment

mikesheffler commented Feb 22, 2022

dellaert commented Feb 22, 2022

Windows Tests #1109

Windows Tests #1109

Conversation

varunagrawal commented Feb 17, 2022

dellaert left a comment

Choose a reason for hiding this comment

dellaert Feb 17, 2022

Choose a reason for hiding this comment

dellaert Feb 17, 2022

Choose a reason for hiding this comment

dellaert Feb 17, 2022

Choose a reason for hiding this comment

varunagrawal commented Feb 17, 2022

dellaert left a comment

Choose a reason for hiding this comment

varunagrawal commented Feb 17, 2022

dellaert commented Feb 17, 2022

mikesheffler commented Feb 18, 2022

Pass

Fail

Possibly relevant warning:

check.discrete Build Errors

dellaert commented Feb 18, 2022

mikesheffler commented Feb 18, 2022

varunagrawal commented Feb 18, 2022 • edited Loading

dellaert commented Feb 18, 2022

mikesheffler commented Feb 21, 2022 • edited Loading

dellaert left a comment

Choose a reason for hiding this comment

mikesheffler commented Feb 22, 2022

dellaert commented Feb 22, 2022

`check.discrete` Build Errors

varunagrawal commented Feb 18, 2022 •

edited

Loading

mikesheffler commented Feb 21, 2022 •

edited

Loading