Support building as a shared library on Windows #147

nre-ableton · 2017-05-25T13:03:23Z

On Windows, it is necessary to add some _declspec() calls to expose
some symbols with different linkage types. This commit adds the
CUKE_API macro, which makes it possible for users to easily build
cucumber-cpp on Windows either as a static or dynamic library by
defining either CUKE_LINKED_AS_SHARED_LIBRARY or
CUKE_CREATE_SHARED_LIBRARY at build time.

muggenhor

This looks promising. I've been meaning to do something similar, to reduce the size of the dynamic symbol table on Linux. What you're doing is an important part of that.

muggenhor · 2017-05-25T13:31:45Z

include/cucumber-cpp/internal/CukeDll.hpp

@@ -0,0 +1,14 @@
+#ifndef CUKE_CUKEDLL_HPP_


I'd appreciate it if you could use CMake's GenerateExportHeader module, or at least make this header match the output that would be generated by that module much closer. To produce this output to get an example, add this to src/CMakeLists.txt and look for the generated files in $BUILDDIR/src/:

include(GenerateExportHeader) set_target_properties(cucumber-cpp cucumber-cpp-nomain PROPERTIES DEFINE_SYMBOL cucumber_cpp_EXPORTS ) generate_export_header(cucumber-cpp)

That should pave the way for symbol visibility on non-Windows OSes too, without having to redo the Windows part later on.

Additionally: to reflect that, please rename this header to not mention DLLs, but the fact that you're declaring which symbols are to be exported. E.g. CukeExport.h.

Sure, I can fix CMakeLists.txt to use GenerateExportHeader. It seems to me though that it would make CukeDll.hpp (or CukeExport.h, or however it should be named) to be obsolete. If it's ok with you, I'll just remove this file and add the necessary include_directories to CMake so that it finds the generated cucumber-cpp_export.h file.

Replacing completely with the generated done is okay with me. Please make that target_include_directories so that transitive build requirements work properly.

@muggenhor ah, I forgot about that, added a fixup just now for that.

muggenhor · 2017-05-25T13:32:20Z

include/cucumber-cpp/internal/CukeDll.hpp

+#ifndef CUKE_CUKEDLL_HPP_
+#define CUKE_CUKEDLL_HPP_
+
+#ifndef CUKE_API_


Don't do this check, it should be considered a bug (and thus reported by the compiler) if it's already defined.

Ok, good point. However I will instead remove this file (see above comment).

muggenhor · 2017-05-25T13:40:22Z

include/cucumber-cpp/internal/CukeDll.hpp

+#define CUKE_CUKEDLL_HPP_
+
+#ifndef CUKE_API_
+#if CUKE_LINKED_AS_SHARED_LIBRARY


Just like GenerateExportHeader, please:

wrap both the export and import case in #ifndef CUCUMBER_CPP_STATIC_DEFINE to detect when we're building or using a static library.

distinguish the export from the import case by the presence of the macros cucumber_cpp_EXPORTS and cucumber_cpp_nomain_EXPORTS (CMake defines those only while building objects for the mentioned target, see the documentation for DEFINE_SYMBOL for more details)

muggenhor · 2017-05-25T13:46:41Z

include/cucumber-cpp/internal/defs.hpp

@@ -1,5 +1,6 @@
 #include "step/StepManager.hpp"
 #include "hook/HookRegistrar.hpp"
 #include "ContextManager.hpp"
+#include "CukeDll.hpp"


Please follow standard "include-what-you-use" rules and don't include what you don't use (in the same file).

Good point, removed.

muggenhor · 2017-05-25T13:48:19Z

include/cucumber-cpp/internal/CukeDll.hpp

+#elif CUKE_CREATE_SHARED_LIBRARY
+#define CUKE_API_ __declspec(dllexport)
+#else
+#define CUKE_API_


FYI: GenerateExportHeader would call this CUCUMBER_CPP_EXPORT (and for Windows #define that to be dllimport when importing).

muggenhor · 2017-05-25T13:51:35Z

include/cucumber-cpp/internal/CukeEngineImpl.hpp

@@ -13,7 +13,7 @@ namespace internal {
 * Currently it is a wrapper around CukeCommands. It will have its own
 * implementation when feature #31 is complete.
 */
-class CukeEngineImpl : public CukeEngine {
+class CUKE_API_ CukeEngineImpl : public CukeEngine {


You forgot to include the header. It probably builds because of an indirect include, but please don't depend on that (include-what-you-use: directly in the file where it's used).

nre-ableton · 2017-05-29T11:54:00Z

@muggenhor thanks for the feedback! I force pushed this branch with some fixes and also cleaned up the commit message to make it more platform-generic. CukeDll.hpp has been completely removed, and now the project relies on the auto-generated header from CMake's GenerateExportHeader module instead.

muggenhor · 2017-05-29T12:10:01Z

src/CMakeLists.txt

+)
+generate_export_header(cucumber-cpp)
+
+include_directories(${CMAKE_CURRENT_BINARY_DIR})


This should no longer be necessary, target_include_directories replaces it.

paoloambrosio · 2017-05-29T15:09:33Z

src/CMakeLists.txt

+set_target_properties(cucumber-cpp cucumber-cpp-nomain PROPERTIES
+    DEFINE_SYMBOL cucumber_cpp_EXPORTS
+)
+generate_export_header(cucumber-cpp)


What about using the EXPORT_FILE_NAME parameter of GENERATE_EXPORT_HEADER (if I read correctly) to match the current file naming conventions for include files? #include "cucumber-cpp_export.h" does not read well

Good point, how does CukeExport.hpp sound (again)? 🙂 fixup pushed.

muggenhor

Except for the alteration to EXPORT_FILE_NAME I suggested, this looks good to go.

I would like to know however whether exporting just parts of a class (as I describe in another comment in this review) works on MSVC. Because that would make it possible to further reduce the exported set of symbols, and thus narrow the exposure of private implementation details further.

muggenhor · 2017-06-02T13:02:23Z

src/CMakeLists.txt

+)
+
+generate_export_header(cucumber-cpp
+    EXPORT_FILE_NAME "CukeExport.hpp"


In order to place this in the same, relative, include directory structure, I think we should use EXPORT_FILE_NAME cucumber-cpp/internal/CukeExport.h (this will affect the required include directives which will need to change to #include <cucumber-cpp/internal/CukeExport.h>). This would make library installs place the headers all in a single location.

muggenhor · 2017-06-02T13:04:16Z

include/cucumber-cpp/internal/ContextManager.hpp

@@ -16,7 +18,7 @@ namespace internal {

 typedef std::vector<shared_ptr<void> > contexts_type;

-class ContextManager {
+class CUCUMBER_CPP_EXPORT ContextManager {


I think it's possible to export only the member function purgeContexts and add a non-template overload for addContext and export that too.

But this can also be done after this PR.

muggenhor · 2017-06-02T13:13:57Z

include/cucumber-cpp/internal/CukeEngine.hpp

@@ -59,7 +61,7 @@ class PendingStepException : public InvokeException {
 * It uses standard types (as much as possible) to be easier to call.
 * Returns standard types if possible.
 */
-class CukeEngine {
+class CUCUMBER_CPP_EXPORT CukeEngine {


I believe all of the members of this class are used internally only, with the exception of the constructor and destructor. So if you explicitly declare those and add (empty) implementations to the .cpp file then exporting just those should be enough.

I know on GNU/Linux that would work. Could you verify that that works on MSVC too? I.e. only exporting some members of a class without exporting the entire class?

AFAIK MSVC should support exporting individual methods and not the entire class. I can test this with dumpbin /EXPORTS (tho I'm not on my Windows machine now, so I'll comment again once I've verified that).

muggenhor · 2017-06-02T13:25:10Z

Oh, before I forget. Could you please add (or modify) an AppVeyor build that will fail to build at least one of the tests or examples if one of the symbols you currently export doesn't get exported? I'd appreciate it if you could demonstrate that failure with an intermediate commit.

This to ensure we don't accidentally break this in the future.

nre-ableton · 2017-07-12T07:29:21Z

@muggenhor Sorry for the delay on finishing up this PR, I had some other work that I had to prioritize in the last few weeks. Anyways, sure, I can adapt the AppVeyor build configuration. I'll be creating another PR so that I can make sure that it fails/succeeds as expected. Please ignore that PR; I'll ping you back in this one once everything is ready to be reviewed again.

coveralls · 2018-05-18T09:22:23Z

Coverage increased (+0.7%) to 62.814% when pulling 9fe13ae on AbletonAppDev:nre_master_shared-lib into 0550fa1 on cucumber:master.

nre-ableton · 2018-05-22T09:06:15Z

@muggenhor Thanks for dusting off and reviving this PR. Do you need any help from me to get it ready to merge? Namely, should I rebase it and fix the merge conflicts?

muggenhor · 2018-05-22T09:32:23Z

I have a rebase ready as e9d074a already. If CI succeeds for d13440a (a commit for CI only that I don't intend to merge) then I intend to push these to this PR. I would appreciate it if you could confirm that it still works as intended for you at that point. That'll probably be about 2 hours from now.

nre-ableton · 2018-05-22T10:16:54Z

@muggenhor I don't have time today to test these changes, but I'm sure they'll be fine. 😄 I did a quick look at the code and I'm sure that it'll work fine for us. We don't use CMake internally and most of the source changes are pretty trivial. Go ahead and merge!

muggenhor · 2018-05-22T17:16:23Z

I intend to merge this tomorrow. It now works on all CI, just minor stuff turned out to be the more difficult things.

On Windows, it is necessary to add some __declspec() calls to expose some symbols with different linkage types. This commit makes it possible for users to easily build cucumber-cpp either as a static or dynamic library by setting CUKE_ENABLE_SHARED_LIBS to ON or OFF at build time.

This makes it possible to install this header at some future point.

Instead of the custom CUKE_ENABLE_SHARED_LIB option prefer the builtin BUILD_SHARED_LIBS option which controls the default library format for add_library.

Making sure GTest/GMock is built as position independent code is necessary when it's linked statically and we try to link it into Cucumber-CPP when that's built as a shared library. Furthermore Boost::unit_test_framework is a direct dependency of cucumber-cpp: we need to link to it when we use it.

Instead of only exporting the GTestDriver, make sure they're all exported from SHARED libraries.

Otherwise how can we execute anything?

This prevents unmarked symbols, which we consider to be internals, from being available on the dynamic library's dynamic symbol table (EXPORT table on Windows). Furthermore we split out a static library. This allows us to expose the library internals to unit tests only, through this static library. This works because symbol hiding only affects dynamic linking, not static linking.

Because this will catch errors with APIs not being properly marked for export. AFAIK this shouldn't fail to catch any errors that a static build would, so I'm not retaining the static build.

nre-ableton · 2018-05-22T18:11:16Z

Great, thanks again for your work on this PR @muggenhor!

This exports all externally used functions and variables to make them accessible from a DLL build. Additionally we now hide all non-marked symbols by default to achieve a similar effect with shared library builds on other platforms.

muggenhor · 2018-05-23T08:48:10Z

This PR has been merged into master with 183c79d. Thanks for your change!

muggenhor requested changes May 25, 2017

View reviewed changes

nre-ableton force-pushed the nre_master_shared-lib branch from b8e685f to cf5ef19 Compare May 29, 2017 11:52

muggenhor reviewed May 29, 2017

View reviewed changes

nre-ableton force-pushed the nre_master_shared-lib branch from 810c656 to b9b2131 Compare May 29, 2017 13:43

paoloambrosio reviewed May 29, 2017

View reviewed changes

muggenhor requested changes Jun 2, 2017

View reviewed changes

nre-ableton force-pushed the nre_master_shared-lib branch from 244a082 to 3de2faa Compare July 12, 2017 07:40

nre-ableton mentioned this pull request Jul 12, 2017

DO NOT MERGE: Testing symbol exporting on AppVeyor #157

Closed

nre-ableton mentioned this pull request Jul 26, 2017

Add support for breaking in the debugger AbletonAppDev/cucumber-cpp#1

Closed

muggenhor force-pushed the nre_master_shared-lib branch from 3de2faa to 051807f Compare May 18, 2018 09:04

muggenhor force-pushed the nre_master_shared-lib branch from 051807f to d7b95d0 Compare May 18, 2018 18:36

muggenhor mentioned this pull request May 19, 2018

Use dependencies via target_link_libraries exclusively #191

Merged

muggenhor force-pushed the nre_master_shared-lib branch from dfb4a0e to 2d0b693 Compare May 22, 2018 17:13

nre-ableton and others added 9 commits May 22, 2018 20:03

Place export header in same include dir namespace as other headers

051503a

This makes it possible to install this header at some future point.

Use CMake standard variable BUILD_SHARED_LIBS

c93b3b9

Instead of the custom CUKE_ENABLE_SHARED_LIB option prefer the builtin BUILD_SHARED_LIBS option which controls the default library format for add_library.

Export all test drivers

3405988

Instead of only exporting the GTestDriver, make sure they're all exported from SHARED libraries.

Export main() from SHARED library

3eef825

Otherwise how can we execute anything?

Hide GMock/GTest internals from cucumber-cpp dynamic symbol table

bd1f9b2

Build as shared library on CI

9fe13ae

Because this will catch errors with APIs not being properly marked for export. AFAIK this shouldn't fail to catch any errors that a static build would, so I'm not retaining the static build.

muggenhor force-pushed the nre_master_shared-lib branch 2 times, most recently from 4e3bedb to 9fe13ae Compare May 22, 2018 18:04

muggenhor approved these changes May 22, 2018

View reviewed changes

muggenhor merged commit 9fe13ae into cucumber:master May 23, 2018

nre-ableton deleted the nre_master_shared-lib branch May 23, 2018 09:09

muggenhor added this to the v0.5 milestone May 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support building as a shared library on Windows #147

Support building as a shared library on Windows #147

nre-ableton commented May 25, 2017

muggenhor left a comment

muggenhor May 25, 2017 •

edited

Loading

nre-ableton May 29, 2017

muggenhor May 29, 2017

nre-ableton May 29, 2017

muggenhor May 25, 2017

nre-ableton May 29, 2017

muggenhor May 25, 2017 •

edited

Loading

muggenhor May 25, 2017

nre-ableton May 29, 2017

muggenhor May 25, 2017

muggenhor May 25, 2017

nre-ableton May 29, 2017

nre-ableton commented May 29, 2017

muggenhor May 29, 2017

paoloambrosio May 29, 2017

nre-ableton May 29, 2017

muggenhor left a comment •

edited

Loading

muggenhor Jun 2, 2017

muggenhor Jun 2, 2017

muggenhor Jun 2, 2017

nre-ableton Jul 12, 2017

muggenhor commented Jun 2, 2017

nre-ableton commented Jul 12, 2017

coveralls commented May 18, 2018 •

edited

Loading

nre-ableton commented May 22, 2018

muggenhor commented May 22, 2018

nre-ableton commented May 22, 2018

muggenhor commented May 22, 2018

nre-ableton commented May 22, 2018

muggenhor commented May 23, 2018

Support building as a shared library on Windows #147

Support building as a shared library on Windows #147

Conversation

nre-ableton commented May 25, 2017

muggenhor left a comment

Choose a reason for hiding this comment

muggenhor May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muggenhor May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nre-ableton commented May 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muggenhor left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muggenhor commented Jun 2, 2017

nre-ableton commented Jul 12, 2017

coveralls commented May 18, 2018 • edited Loading

nre-ableton commented May 22, 2018

muggenhor commented May 22, 2018

nre-ableton commented May 22, 2018

muggenhor commented May 22, 2018

nre-ableton commented May 22, 2018

muggenhor commented May 23, 2018

muggenhor May 25, 2017 •

edited

Loading

muggenhor May 25, 2017 •

edited

Loading

muggenhor left a comment •

edited

Loading

coveralls commented May 18, 2018 •

edited

Loading