fix shared on windows

[SpaceIm]

closes #458
closes #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 

[SeanCurtis-TRI]

Reviewed 2 of 357 files at r1.
Reviewable status: 2 of 357 files reviewed, 2 unresolved discussions (waiting on @SpaceIm)

---

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

# POSSIBILITY OF SUCH DAMAGE.

cmake_minimum_required(VERSION 3.7)

nit Unilateral changes to the build system are a bit disconcerting.

@jamiesnape can you weigh in on this change?

---

include/fcl/CMakeLists.txt, line 38 at r1 (raw file):

configure_file(config.h.in config.h @ONLY)

set(FCL_EXPORT_MACRO_NAME "FCL_API")

@jamiesnape could you look at this as well.

(NIT: the presence of a bunch of comments is never a good thing.)

[SpaceIm]

Reviewable status: 2 of 357 files reviewed, 2 unresolved discussions (waiting on @jamiesnape)

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit Unilateral changes to the build system are a bit disconcerting.

@jamiesnape can you weigh in on this change?

As I said, it is required if we want to inject custom string in export.h
Otherwise it would require more refactoring in order to avoid breaking include logic.

---

include/fcl/CMakeLists.txt, line 38 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

@jamiesnape could you look at this as well.

(NIT: the presence of a bunch of comments is never a good thing.)

which comments? Are you talking about /* We are building this library */ ? It's just to be consistent with content of export.h, it can be removed.

[SeanCurtis-TRI]

Reviewable status: 2 of 357 files reviewed, 2 unresolved discussions (waiting on @jamiesnape)

---

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

Previously, SpaceIm wrote…

As I said, it is required if we want to inject custom string in export.h
Otherwise it would require more refactoring in order to avoid breaking include logic.

I'm being cautious. Making it work on windows has to be compatible with the other domains that we support. I'm largely CMake-ignorant, so I can't review this change in that context; hence calling in an expert.

---

include/fcl/CMakeLists.txt, line 38 at r1 (raw file):

Previously, SpaceIm wrote…

which comments? Are you talking about /* We are building this library */ ? It's just to be consistent with content of export.h, it can be removed.

Ah....I think I see my confusion:

1. My aforementioned CMake incompetency
2. My superficial glance at the change led me to think lines like: # define FCL_EXTERN_TEMPLATE_API was a CMake comment.
3. The separation between # and define contributes to that. Why isn't it #define?

[jamiesnape]

Reviewable status: 2 of 357 files reviewed, 2 unresolved discussions (waiting on @jamiesnape, @SeanCurtis-TRI, and @SpaceIm)

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I'm being cautious. Making it work on windows has to be compatible with the other domains that we support. I'm largely CMake-ignorant, so I can't review this change in that context; hence calling in an expert.

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.

[jamiesnape]

Note if we are going to remove large numbers trailing spaces, it should be a separate PR that does nothing else.

Reviewable status: 2 of 357 files reviewed, 2 unresolved discussions (waiting on @jamiesnape, @SeanCurtis-TRI, and @SpaceIm)

[jamiesnape]

Reviewable status: 2 of 357 files reviewed, 2 unresolved discussions (waiting on @SeanCurtis-TRI and @SpaceIm)

---

include/fcl/CMakeLists.txt, line 38 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Ah....I think I see my confusion:

1. My aforementioned CMake incompetency
2. My superficial glance at the change led me to think lines like: # define FCL_EXTERN_TEMPLATE_API was a CMake comment.
3. The separation between # and define contributes to that. Why isn't it #define?

FYI Some preprocessors need the # to be in the first column of the line, so indenting preprocessor definitions is usually done after the #.

[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 :/

[SeanCurtis-TRI]

Thanks for the contribution -- the windows side is definitely underserved and it's great that you're helping out your fellow windows users.

I can very much sympathize with your position on the white space. It'd be a real bear to back it out. In appreciation of your contribution, I'm going to slog through all of it so you don't have to change it. But you'd be doing me a huge favor in the future if your PRs don't include whitespace changes (unless, of course, it's nothing but). :)

Curiously, it was interesting to see the geographic patterns that appeared in the whitespace changes. Makes me wonder about the provenance of the clusters.

Reviewed 355 of 357 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jamiesnape and @SpaceIm)

a discussion (no related file):
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.

---

---

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

Previously, SpaceIm wrote…

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.

In this case, we're constrained enough that we'll have to cobble something together out of older technology. What do they say? True creativity arises under constraints? :)

---

include/fcl/CMakeLists.txt, line 38 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

FYI Some preprocessors need the # to be in the first column of the line, so indenting preprocessor definitions is usually done after the #.

Fair enough....in that case, I'd kill the subsequent indentation between "#" and "define".

[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.

---

[SeanCurtis-TRI]

I agree. However, it seemed to be that because every file with white space changes also has a non-whitespace change and then, when there are 350+ of them, separating the wheat from the chaff becomes non-trivial simply by the principle that small and simple times 350 is no longer small and simple. Hence my willingness to swallow it on this one.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jamiesnape and @SpaceIm)

a discussion (no related file):

Previously, SpaceIm wrote…

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

No worries.

I hesitate to ask that but the symbols FCL_EXTERN_TEMPLATE_API and FCL_INSTANTIATION_DEF_API look very different for two concepts that are so closely related. Can we come up with names that better represents their relationship?

FCL_TEMPLATE_INSTANCE_DECLARE_API
FCL_TEMPLATE_INSTANCE_DEFINE_API

or

FCL_EXPLICT_INSTANCE_DECLARE_API
FCL_EXPLICT_INSTANCE_DEFINE_API

If we can come up with some names that appeal to everyone, then a quick search and replace in your IDE should be trivial. But let's not pull the trigger on that unless we come up with names we like more.

---

[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).

[SeanCurtis-TRI]

That part is clear - but from where I'm sitting, all it does is take the PR and put it in "staged". But how do you take the staged changes and curate them into two commits? That's the part I consider to be time consuming and painful.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jamiesnape and @SpaceIm)

[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.

[SeanCurtis-TRI]

I feel foolish...I was limiting myself in considering untangling the commit it in terms of git operations. I hadn't considered duplicating the effect of the white space changes. :) And, as Sherm points out, once you've achieved the effect, a quick rebase and you're done.

I suspect the search and replace would have to be a bit more complex. I believe it's also making a new line character change in several files (a dozen or so). At least, that's my unconfirmed hypothesis.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jamiesnape and @SpaceIm)

[SeanCurtis-TRI]

As I see this, the CMake formulation requiring CMake 3.7 is blocking this moving forward (the whitesapce, while a fun conversation, is resolved).

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jamiesnape and @SpaceIm)

[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.

[SeanCurtis-TRI]

Do we have a straightforward solution to advancing this?

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jamiesnape and @SpaceIm)

[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 (

fcl/.editorconfig

Line 41 in df2702c

trim_trailing_whitespace = true

), 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 af00cc2 (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.