Fix Shared library export and don't install GTEST by damiandixon · Pull Request #245 · AngusJohnson/Clipper2

damiandixon · 2022-09-30T08:53:17Z

The shared library export fix is detailed in #244 for non Windows only.

This adds the CLIPPER2_DLL to all classes and methods that are in the headers that have a cpp implementation.

The CMakeLists.txt is updated to ensure that the exports are exported.

The libraries are built so that symbols are hidden unless exported.

Additional updates in CMakeLists.txt:

Added a default CMAKE_BUILD_TYPE of release and set the build type options to make it easier to select.
Turned off the install of GTEST as it makes no sense to install it.

This leaves the Windows build as a static library.

The default on Linux is a Shared library.

This is a work in progress.

Build libraries as shared. Hide classes and methods not exported or fully defined in headers

Set default build to Release

lederernc · 2022-09-30T14:57:10Z

To get around the issue with the current build you will need to either disable the C4251 warning or prohibit the windows build from building a shared library.

To disable the C4251 warning you will need to sprinkle code like this throughout the code base inside class/struct declarations:

#pragma warning(push)
#pragma warning(disable : 4251)  // dllexport warning
...
#pragma warning(pop)

Here is a diff that fixes it:

git diff --staged
diff --git a/CPP/Clipper2Lib/clipper.engine.h b/CPP/Clipper2Lib/clipper.engine.h
index 5d28baa..5d3d647 100644
--- a/CPP/Clipper2Lib/clipper.engine.h
+++ b/CPP/Clipper2Lib/clipper.engine.h
@@ -20,6 +20,8 @@
 #include <functional>
 #include "clipper.core.h"

+#pragma warning(push)
+#pragma warning(disable : 4251)  // dllexport warning
 namespace Clipper2Lib {

        struct Scanline;
@@ -498,5 +500,6 @@ namespace Clipper2Lib {
        };

 }  // namespace
+#pragma warning(pop)

 #endif  // clipper_engine_h
diff --git a/CPP/Clipper2Lib/clipper.offset.h b/CPP/Clipper2Lib/clipper.offset.h
index d5399b4..1f91081 100644
--- a/CPP/Clipper2Lib/clipper.offset.h
+++ b/CPP/Clipper2Lib/clipper.offset.h
@@ -13,6 +13,8 @@

 #include "clipper.core.h"

+#pragma warning(push)
+#pragma warning(disable : 4251)  // dllexport warning
 namespace Clipper2Lib {

 enum class JoinType { Square, Round, Miter };
@@ -105,4 +107,6 @@ public:
 };

 }
+#pragma warning(pop)
+
 #endif /* CLIPPER_OFFSET_H_ */

jdumas · 2022-09-30T15:43:09Z

CPP/CMakeLists.txt

 option(CLIPPER2_EXAMPLES "Build examples" ON)
 option(CLIPPER2_TESTS "Build tests" ON)

+set(CMAKE_CONFIGURATION_TYPES "Debug;Release;RelWithDebInfo" CACHE STRING "Configs" FORCE)


Please don't force the value of this variable, as this can disrupt projects including Clipper2 as a subproject.

jdumas · 2022-09-30T15:43:23Z

CPP/CMakeLists.txt


+set(CMAKE_CONFIGURATION_TYPES "Debug;Release;RelWithDebInfo" CACHE STRING "Configs" FORCE)
+if(DEFINED CMAKE_BUILD_TYPE)
+   SET_PROPERTY(CACHE CMAKE_BUILD_TYPE PROPERTY STRINGS ${CMAKE_CONFIGURATION_TYPES})


Use lowercase.

jdumas · 2022-09-30T15:44:23Z

CPP/CMakeLists.txt

 # 2d version of Clipper2
-add_library(Clipper2 STATIC ${CLIPPER2_INC} ${CLIPPER2_SRC})
-
+add_library(Clipper2 SHARED ${CLIPPER2_INC} ${CLIPPER2_SRC})


It is preferable to omit SHARED or STATIC altogether and let CMake generate a static/shared library based on the value of the BUILD_SHARED option (set by the user).

jdumas · 2022-09-30T16:00:24Z

CPP/Clipper2Lib/clipperdll.h

+* License   :  http://www.boost.org/LICENSE_1_0.txt                            *
+*******************************************************************************/
+
+#ifndef CLIPPER2_DLL


Consider using https://cmake.org/cmake/help/latest/module/GenerateExportHeader.html rather than defining this by hand (provides flexibility for both static/shared libs with the same CMake target).

I was not aware of this. Tx.

AngusJohnson · 2022-09-30T23:42:50Z

This adds the CLIPPER2_DLL to all classes and methods that are in the headers that have a cpp implementation.

I'm wondering if exporting classes is the best approach for DLLs?
Would it not be better/safer to add additional (admittedly more complex) functions that encapsulate these classes; functions that provide the full feature set of these classes?

damiandixon · 2022-10-01T13:41:33Z

This adds the CLIPPER2_DLL to all classes and methods that are in the headers that have a cpp implementation.

I'm wondering if exporting classes is the best approach for DLLs? Would it not be better/safer to add additional (admittedly more complex) functions that encapsulate these classes; functions that provide the full feature set of these classes?

Personally, I would prefer to keep the classes. All shared libraries in C++ bump into this in one form or another.

…and_shared_lib_linux

…pported

Clean up target properties

Provides versioning and install of headers.

AngusJohnson · 2022-10-02T10:06:27Z

Personally, I would prefer to keep the classes. All shared libraries in C++ bump into this in one form or another.

I appreciate that exporting C++ classes may be very useful in Linux, but ISTM that it's discouraged in Windows.
And is this approach really better than providing a small set of exported functions that will do everything that's done by these classes?

SpaceIm · 2022-10-17T00:42:41Z

CPP/Clipper2Lib/clipperdll.h

+#    define CLIPPER2_DLL
+#  else
+#    if __GNUC__ >= 4
+#      define CLIPPER2_DLL __attribute__((visibility("default")))


CLIPPER2_DLL is a misleading name, since it's designed to be cross-platform. Usually it's called <LIB>_API or <LIB>_EXPORT

I'd be happy to rename it CLIPPER2_EXPORT if there's concensus with that.
Edit: And presumably the file would be clipper.export.h .

Damian Dixon added 3 commits September 30, 2022 09:41

Export classes and methods from shared libraries

8511972

Build libraries as shared. Hide classes and methods not exported or fully defined in headers

Make selection of build type easier.

6650e4f

Set default build to Release

Do not install GTEST by default.

9cff864

damiandixon mentioned this pull request Sep 30, 2022

Shared library #243

Merged

Damian Dixon added 5 commits September 30, 2022 10:00

Windows export issue

b0b18f4

Define _CRT_SECURE_NO_WARNINGS for Windows

07e33f0

Only define _CRT_SECURE_NO_WARNINGS on WIN32

95db9cd

Add clipperdll.h to list of sources so that it is installed.

fd72639

Display the host build system and WIN32 cmake variable

a37c03a

jdumas reviewed Sep 30, 2022

View reviewed changes

damiandixon added 6 commits October 1, 2022 22:30

Merge remote-tracking branch 'origin/main' into dd/fix_cmake_install_…

fb646c9

…and_shared_lib_linux

Remove __declspec as shared libraries on windows are not currently su…

6fcf8cc

…pported

Whitespace changes

ae55807

remove debug messages

6d47780

Enable shared library on non-windows only

290e96c

Clean up target properties

Treat the Clipper2utils library the same as Clipper2

c07266a

Provides versioning and install of headers.

AngusJohnson mentioned this pull request Oct 4, 2022

Export classes and functions from library for Windows & Linux #244

Closed

SpaceIm reviewed Oct 17, 2022

View reviewed changes

AngusJohnson closed this Oct 25, 2022

Conversation

damiandixon commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lederernc commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdumas Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

damiandixon Oct 1, 2022

Choose a reason for hiding this comment

Uh oh!

jdumas Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

jdumas Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

damiandixon Oct 1, 2022

Choose a reason for hiding this comment

Uh oh!

jdumas Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

damiandixon Oct 1, 2022

Choose a reason for hiding this comment

Uh oh!

AngusJohnson commented Sep 30, 2022

Uh oh!

damiandixon commented Oct 1, 2022

Uh oh!

AngusJohnson commented Oct 2, 2022

Uh oh!

SpaceIm Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AngusJohnson Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

damiandixon commented Sep 30, 2022 •

edited

Loading

lederernc commented Sep 30, 2022 •

edited

Loading

SpaceIm Oct 17, 2022 •

edited

Loading

AngusJohnson Oct 17, 2022 •

edited

Loading