Fix MSVC / Windows Build of GTSAM_UNSTABLE #1102
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… , classes with no methods defined in a .cpp file shouldn't have the GTSAM_EXPORT or GTSAM_UNSTABLE_EXPORT modifier. This was causing problems with the building of gtsam_unstable on MSVC / Windows.
dellaert
approved these changes
Feb 15, 2022
There was a problem hiding this comment.
Great!
@varunagrawal please approve and you merge if you think it's OK. It does follow the guidelines in that markdown file.
|
PS, could you paste the errors for discrete? We can maybe tackle one at a time? |
|
Oh, huh. I guess I don't get email for stuff posted here? Anyway, here's the output for building I can't really parse too much out of that, but here's the offending block: /**
* Convert labels from type M to type L.
*
* @param other: The AlgebraicDecisionTree with label type M to convert.
* @param map: Map from label type M to label type L.
*/
template <typename M>
AlgebraicDecisionTree(const AlgebraicDecisionTree<M>& other,
const std::map<M, L>& map) {
// Functor for label conversion so we can use `convertFrom`.
std::function<L(const M&)> L_of_M = [&map](const M& label) -> L {
return map.at(label);
};
std::function<double(const double&)> op = Ring::id;
this->root_ = this->template convertFrom(other.root_, L_of_M, op);
}I think the specific complaint has to do with |
Ha! A mere compile error :-) Seems compilers disagree. I tried locally with MacOS/clang and the following works as well: Maybe that will make discrete pass on MSVC? |
|
Flabbergasted that so many targets fail even in gtsam. Let me know if you get tired of this, but we could play the same game for |
|
It does make it compile! Unfortunately, that means that there are now link errors, which might not be obvious. I haven't looked to see if these are easy or hard to resolve. If it were just things that had been left out of a list in CMake, I would expect problems on the Linux side as well, so it's probably something more 'fun' related to MSVC, specifically.
I'm game. I'm going to be away from the computer for a little bit, but let me know if anything obvious jumps out at you from the linker problems above. I'll take a look later as well. In case we do want to chase those other two targets, here are the current errors, just an FYI.
|
|
Okay, this time I'm really leaving, but before that, looking at the first error ... I think it's actually right to complain? It's in both testDecisionTreeFactor.cpp and testDiscreteFactorGraph.cpp and it's not the same test.
|
|
A little bird pointed out to me that #91 is probably wrapped in, out, and all around the Windows build issues I'm seeing. We should probably ...
|
|
Mike, as I was looking into this it was straightforward (though still some work) to fix it myself. Added you as reviewer on #1107 |
|
As far as this PR goes, still waiting on @varunagrawal to weigh in. |
This is interesting, I didn't even realize it was happening. Great find @mikesheffler! |
varunagrawal
approved these changes
Feb 17, 2022
There was a problem hiding this comment.
Sweeeet. Thanks for fixing all my stupid mistakes @mikesheffler! Having a second pair of eyes really helped me understand the core of this issue. 😄
|
I'm going ahead and merging this (since I am waiting on CI for other things). We can tackle the rest of the targets in a new PR that builds on this and #1107. |
Third set! Someone else put it together and prodded me to point it out. 🙂 |
gchenfc
added a commit
to gchenfc/gtsam
that referenced
this pull request
Apr 21, 2022
borglab#1102 correctly removed "GTSAM_EXPORT" from NoiseModelFactorX
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Per https://github.com/borglab/gtsam/blob/develop/Using-GTSAM-EXPORT.md , classes with no methods defined in a .cpp file shouldn't have the GTSAM_EXPORT or GTSAM_UNSTABLE_EXPORT modifier. This was causing problems with the building of gtsam_unstable on MSVC / Windows.
What I don't like about this PR is that it is reverses several changes @varunagrawal made in #904. I think that the way it is done in this PR is probably correct, but I think @varunagrawal should weigh in, just in case I'm missing something.
This is in service of #1087, but it isn't a resolution, since that issue is about MATLAB. I started this little cul-de-sac as a waypoint on the way to examining the MATLAB build, and I'll take a look at that soon.
Two requests from @dellaert are ignored here:
Silence some / all of the C4834 warnings about ignored returns from [[nodiscard]] functions: Doing this file-by-file with
#pragma warning(disable : 4834)resulted in, I think, 85 files with changes. I made a patch of that change just in case we do want it, but I thought that changing that many files was probably a sign it was not the right approach.Add more actions to the Windows CI: There are six targets in
.github/workflows/build-windows.yml:and it turns out that that is all of the the
checks that will pass right now. I can modify the list to include more targets, but they will fail. I haven't tried to look at them and see why they're failing.