fix shared on windows

[SpaceIm]

closes https://github.com/flexible-collision-library/fcl/issues/458 closes https://github.com/flexible-collision-library/fcl/issues/516

This big patch comes with several modifications:

• FCL_EXPORT is renamed FCL_API
• FCL_EXPORT (aka FCL_API now) is removed in all template declarations.
• 2 new definitions are introduced to handle explicit template declaration and explicit template definition for all compilers: FCL_EXTERN_TEMPLATE_API and FCL_INSTANTIATION_DEF_API. They are needed because Visual Studio expect dllexport in explicit template definition, while others compilers expect this kind of attribute in explicit template declaration. These macros are defined through an option in generate_export_header (CUSTOM_CONTENT_FROM_VARIABLE) introduced in CMake 3.7. Therefore minimum version of CMake is bumped to 3.7 (maybe undesirable).
• It also adds more exported symbols.
• ~(my IDE has also removed a lot of trailing spaces).~ => not anymore

---

This change is 

[SpaceIm]

---

CMakeLists.txt, line 34 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

If we want to continue to support Trusty, we need minimum 2.8.12, if we want to continue to support Xenial, we need minimum 3.5.

I am hoping to retool the build system for when we drop support for those platforms, but we are not at that stage yet, and I do not want to incrementally bump the CMake minimum version from 3.7 to 3.10 as we add features, since it adds complexity to our testing infrastructure (3.6-3.9 are not in an Ubuntu LTS).

Therefore, I am afraid, we cannot bump the version to 3.7.

The only reason of 3.7 is the option CUSTOM_CONTENT_FROM_VARIABLE I use in generate_export_header in order to inject explicit template instantiation and definition definitions. There should be another way to achieve the same behaviour without this option. For example, a less elegant solution could be to rename export.h, and create a file export.h with macros related to template instantiation which includes the file generated by generate_export_header. Thereby, we can rely of the current include behaviour of export.h in other files.

[SpaceIm]

Honestly, I can't imagine redo all these modifications (357 files!) just to remove modifications related to trailing spaces :/

[jwnimmer-tri]

Without implying any vote on whitespace process, FYI it's not difficult to fix. First, start from master and remove all trailing whitespace in a new commit, PR that, and merge to master. Then, rebase this PR atop that commit, always resolving conflicts by this PR winning (you can even just re-copy a backup of these files during the merge or rebase, don't even need to deal with diffs).

[SpaceIm]

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I didn't quite follow this:

Visual Studio expect dllexport in explicit template definition, while others compilers expect this kind of attribute in explicit template declaration.

Could you elaborate on what an "explicit template definition" is? What came to mind to me was explicit instantiation.

But then I'm not quite sure what "explicit template declaration" is.

ooops sorry, it's a typo: I meant explicit instantiation declaration and explicit instantiation definition.

---

[jwnimmer-tri]

It's simple to git reset --hard OLDSHA to reset this branch after a rebase. It's O(1) effort not O(N).

[jwnimmer-tri]

Rough outline: Part A:

git checkout master
git checkout -b whitespace-only
perl -pi -e 's# *$##g;' $(find . -name '*.hh' -o -name '*.cc')
# git add, git commit
# open a new PR with whitespace-only branch; review, CI, and merge it to master

Part B:

git checkout fix-shared-windows  # this current PR
cp -r src ../src.backup
cp -r include ../include.backup
git checkout master
git checkout -b fix-shared-windows-without-whitespace
rm -rf src include
mv ../src.backup src
mv ../include.backup include
# git add, git commit

Now fix-shared-windows-without-whitespace is identical to this PR, except rebased on whitespace changes.

[sherm1]

That seems very straightforward! An alternate part B could be

# After the part A whitespace is merged into master:
git checkout master
git pull upstream master
git checkout fix-shared-windows  # this current PR
git rebase master  # using your favorite merge tool

Merging should be easy.

[ahoarau]

This PR works perfectly ! Thanks for the hard work. I would love to see that integrated in upstream. Indeed cmake is staying with an old version on xenial, but having the latest 3.20.2 is very easy since it only requires to put the archive's bin in the PATH.

That being said, I also understand the need of having only apt executables. How long do you plan to "support" cmake 3.5 ?

[jamiesnape]

Indeed cmake is staying with an old version on xenial, but having the latest 3.20.2 is very easy since it only requires to put the archive's bin in the PATH.

Also you can use the xenial APT packages from https://apt.kitware.com.

That being said, I also understand the need of having only apt executables. How long do you plan to "support" cmake 3.5 ?

I think we would be dropping support as soon as possible, but it has not been the highest priorities.

[jamiesnape]

Do we have a straightforward solution to advancing this?

At this stage, assuming that CI does not break: rebase, resolve conflicts, merge, and apologize to people using very old CMake (pre 3.7). If CI does break, I guess I could migrate it to GitHub Actions in an hour or two.

[SteB0]

Hi, thanks god that i found this issue! After struggling with fcl inside my windows project i found @SpaceIm´s branch. Is someone working on a solution to fix the master version?

[SpaceIm]

Here is an attempt to solve merge conflict.

They are few things to fix which are not seen by merge conflict.

[SpaceIm]

I think the CMake min version issue is resolved since min version is 3.10 in master branch.

I've changed 3 more things:

• hide all symbols by default => there is no reason to not hide private symbols.
• rely on CMake abstraction to hide symbols.
• allow to build tests when private symbols are hidden (I've also added enable_testing() at the root CMakeLists, it's convenient) => it works just fine.

@SeanCurtis-TRI I let you do your trick with rebase, split branch to remove whitespaces. But sorry, I won't solve merge conflicts again, it's too tedious and this PR is opened for 2 years. By the way I still see trailing whitespaces in master, now I had to disable editorconfig. If your team doesn't follow your own .editorconfig (https://github.com/flexible-collision-library/fcl/blob/df2702ca5e703dec98ebd725782ce13862e87fc8/.editorconfig#L41), it would be nice to remove it to avoid to trick external contributors, specially if you are so strict about formatting in PR. Your clang-format doesn't seem to be honored as well in the code base. Fortunately, it was not activated at edit time in my IDE for fcl, otherwise it would have been a big mess to review the modifications.

[sherm1]

I'd like to merge this heroic effort before code-rot sets in again! A few prerequisites

• FCL CI seems broken. Does anyone know how to fix it?
• We need at least one Windows user to verify that the most recent version solves the original problem. @SteB0 can you do that?

cc @SeanCurtis-TRI @jamiesnape @j-rivero @jslee02

[sherm1]

Also cc @SeanCurtis-Drake

[SteB0]

Hi, checked out latest version from @SpaceIm (265928f) and could build and use it without octomap and without Tests. After enabling "Build_Testing" in cmake-configuration i got linker errors e.g. [46/81] Linking CXX executable test\test_fcl_collision.exe FAILED: test/test_fcl_collision.exe

undefined reference to 'fcl::detail::ConvertBVImpl<double, fcl::AABB<double>, fcl::OBB<double> >::run(fcl::AABB<double> const&, Eigen::Transform<double, 3, 1, 0> const&, fcl::OBB<double>&)'

[SpaceIm]

Which compiler? I think it's fine with msvc, I'll try with a GNU compiler.

[SteB0]

mingw810_64 x86_64-w64-mingw32-g++.exe

[SpaceIm]

With apple-clang 13 on macOS, it works fine with FCL_HIDE_ALL_SYMBOLS=ON, FCL_STATIC_LIBRARY=OFF and BUILD_TESTING=ON. I use libccd v2.1 and eigen 3.4.0. While it compiles and links fine, when I run tests, few of them fail in test_fcl_signed_distance, test_gjk_libccd-inl_epa, test_gjk_libccd-inl_gjk_doSimplex2 and test_gjk_libccd-inl_signed_distance. Moreover the last one is stuck at some point (in box_box test)

I'll try with MinGW (but I have gcc 11.2.0 with runtime 9.0.0 on my windows computer, mingw810_64 is pretty old).

[ahoarau]

Don't know if this is relevant for this MR, but I just found out that FCL can be made into a header-only library. Can anyone explain the motivation behind declaring explicit templates in .cc ? Only speed when linking against it ?

[SpaceIm]

I would say speed up compilation of downstream projects, by avoiding common template instantiations in fcl.

[SpaceIm]

Ok so first thing to do to fix msvc shared with tests: turn test_fcl_utility to a static library.

And I also see link errors of test_fcl_collision with Visual Studio, but it's against a constructor of fcl::ContactPoint<double>. I see, fcl::ContactPoint<double> doesn't declare nor define its destructor and copy/move constructors/assignement operators. They are not instantiated by Visual Studio during compilation, but at consume time explicit declaration tells to Visual Studio that they should be defined somewhere else so it doesn't try to instantiate them.

[SpaceIm]

Ok for Visual Studio, there were several missing declaration & definition of destructor, copy/move constructor/assignement in Contact & ContactPoint (though, it might be a bug in msvc). Moreover M_PI is not defined by default in cmath for Visual Studio, compilation of one test was failing.

[SpaceIm]

For MinGW, there are indeed some link issues in tests (test_collision_func_matrix for me) with ComputeBVImpl. I'll see what I can do.

[SpaceIm]

@SteB0 I think MinGW is fixed now with https://github.com/flexible-collision-library/fcl/pull/461/commits/af00cc29bffc9eb9a9523651df8bff9818db3d26 (may be tedious to review). The shared library builds and links, as well as the tests, even when all symbols are hidden.

Indeed order of declaration & definition of ComputeBVImpl templates was not correct. The proper order is:

• template declaration
• explicit instantiation declaration
• template definition
• explicit instantiation definition (in one and only one compilation unit)

[SpaceIm]

@SeanCurtis-TRI @jamiesnape @sherm1 I've fixed all the autoformatting mess, now only remain relevant modifications.