Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DLL export/import using BUILD_SHARED_LIBS #406

Merged
merged 12 commits into from
Mar 7, 2023

Conversation

pierre-aimi
Copy link
Contributor

@pierre-aimi pierre-aimi commented Feb 6, 2023

This is a working version of Windows DLL imports/exports.
I've tested using CMake only currently, but it should work with the following #defines:
#define SQLITECPP_COMPILE_DLL is necessary when creating DLLs
#define SQLITECPP_DLL_EXPORT will export symbols, otherwise it will import symbols

Using CMake, all that is necessary is BUILD_SHARED_LIBS turned on. All that CMake does is turn on exporting when building the DLL and then use importing when compiling the example.

Happy to make modifications, just thought I'd have this first pass reviewed first.

@pierre-aimi
Copy link
Contributor Author

I just successfully built a VS 2022 Debug x86 shared libs build by the way. Looks like the AppVeyor build got a 504 anyway.
I haven't got the meson build working properly yet (it's not part of my PR). I managed to build a DLL but none of the symbols are exported. This is probably down to my lack of meson build script experience.

@SRombauts SRombauts self-assigned this Feb 8, 2023
@SRombauts
Copy link
Owner

SRombauts commented Feb 8, 2023

I restarted the ci build
I'll try to take a look, but don't hesitate to ping me!

@UnixY2K
Copy link
Contributor

UnixY2K commented Feb 12, 2023

I just successfully built a VS 2022 Debug x86 shared libs build by the way. Looks like the AppVeyor build got a 504 anyway. I haven't got the meson build working properly yet (it's not part of my PR). I managed to build a DLL but none of the symbols are exported. This is probably down to my lack of meson build script experience.

let me check it, I had another solution which was to make a define in the CPP files so we did not depend on the build system, but this is also a good solution, I will try to check your solution and post the suggested meson changes.

/* Windows DLL export/import */
#if defined(WIN32) && defined(SQLITECPP_COMPILE_DLL)
#if SQLITECPP_DLL_EXPORT
#define DLL_API __declspec(dllexport)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#define DLL_API __declspec(dllexport)
#define SQLITECPP_DLL_API __declspec(dllexport)

I think it should be better if something like SQLITECPP_API instead of DLL_API in order to avoid accidental macro collisions with another libraries, but this should require a quick find and replace of it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I was wondering what would be the best naming convention here...happy to change this

@UnixY2K
Copy link
Contributor

UnixY2K commented Feb 12, 2023

the meson.build would require the following changes:

Diff File
diff --git a/meson.build b/meson.build
index e270d56..dba3c49 100644
--- a/meson.build
+++ b/meson.build
@@ -65,6 +65,8 @@ sqlitecpp_deps = [
 ]
 ## used to override the default sqlitecpp options like cpp standard
 sqlitecpp_opts = []
+## used to set required macros when using sqlitecpp
+sqlitecpp_dep_args = []
 
 ## tests
 
@@ -97,11 +99,6 @@ if host_machine.system() == 'windows'
     sqlitecpp_opts += [
         'cpp_std=c++14',
     ]
-    # check that we are not trying to build as dynamic library
-    if get_option('default_library') != 'shared'
-        message('warning: SQLiteCpp does not support shared library on Windows, the library will be built as static')
-    endif
-    
 endif
 # Options relative to SQLite and SQLiteC++ functions
 
@@ -199,33 +196,36 @@ if get_option('b_coverage')
     ]
 endif
 
-## Workarround for windows: if building on windows we will build the library as static
-if host_machine.system() == 'windows'
-    libsqlitecpp = static_library(
-        'sqlitecpp',
-        sqlitecpp_srcs,
-        include_directories: sqlitecpp_incl,
-        cpp_args: sqlitecpp_args,
-        dependencies: sqlitecpp_deps,
-        # override the default options
-        override_options: sqlitecpp_opts,)
-else
-    libsqlitecpp = library(
-        'sqlitecpp',
-        sqlitecpp_srcs,
-        include_directories: sqlitecpp_incl,
-        cpp_args: sqlitecpp_args,
-        dependencies: sqlitecpp_deps,
-        # override the default options
-        override_options: sqlitecpp_opts,
-        install: true,
-        # API version for SQLiteCpp shared library.
-        version: '0',)
+sqlitecpp_static_args = sqlitecpp_args
+sqlitecpp_static_dep_args = sqlitecpp_dep_args
+# if windows and shared library
+if host_machine.system() == 'windows' and get_option('default_library') == 'shared'
+    # compile with SQLITECPP_COMPILE_DLL and SQLITECPP_DLL_EXPORT=1
+    sqlitecpp_args += [
+        '-DSQLITECPP_COMPILE_DLL',
+        '-DSQLITECPP_DLL_EXPORT=1',
+    ]
+    sqlitecpp_dep_args += [
+        # we just need to define SQLITECPP_COMPILE_DLL
+        '-DSQLITECPP_COMPILE_DLL',
+    ]
 endif
 
+libsqlitecpp = library(
+    'sqlitecpp',
+    sqlitecpp_srcs,
+    include_directories: sqlitecpp_incl,
+    cpp_args: sqlitecpp_args,
+    dependencies: sqlitecpp_deps,
+    # override the default options
+    override_options: sqlitecpp_opts,
+    install: true,
+    # API version for SQLiteCpp shared library.
+    version: '0',)
+
 if get_option('SQLITECPP_BUILD_TESTS')
     # for the unit tests we need to link against a static version of SQLiteCpp
-    if host_machine.system() == 'windows' or get_option('default_library') == 'static'
+    if get_option('default_library') == 'static'
         # we do not need to recomplile the library
         libsqlitecpp_static = libsqlitecpp
     else
@@ -233,7 +233,7 @@ if get_option('SQLITECPP_BUILD_TESTS')
             'sqlitecpp_static',
             sqlitecpp_srcs,
             include_directories: sqlitecpp_incl,
-            cpp_args: sqlitecpp_args,
+            cpp_args: sqlitecpp_static_args,
             dependencies: sqlitecpp_deps,
             # override the default options
             override_options: sqlitecpp_opts,)
@@ -247,13 +247,14 @@ install_subdir(
 sqlitecpp_dep = declare_dependency(
     include_directories: sqlitecpp_incl,
     link_with: libsqlitecpp,
+    compile_args: sqlitecpp_dep_args,
 )
 if get_option('SQLITECPP_BUILD_TESTS')
     ## make the dependency static so the unit tests can link against it
-    ## (mainly for windows as the symbols are not exported by default)
     sqlitecpp_static_dep = declare_dependency(
         include_directories: sqlitecpp_incl,
         link_with: libsqlitecpp_static,
+        compile_args: sqlitecpp_static_dep_args,
     )
 endif
 
@@ -264,7 +265,7 @@ if get_option('SQLITECPP_BUILD_TESTS')
                 fallback: ['gtest', 'gtest_main_dep'])
     sqlitecpp_test_dependencies = [
         gtest_dep,
-        sqlitecpp_static_dep,
+        sqlitecpp_dep,
         sqlite3_dep,
     ]

if required I can do the changes on another PR

#pragma message("Importing symbols")
#endif
#else
#define DLL_API

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also check if the compiler supports GNUC, and set up:

__attribute__ ((visibility ("default")))

This then allows you to specify on unix that the software should be compiled with e.g. meson's gnu_symbol_visibility: 'hidden', which allows avoiding accidental leakage of private symbols, and also allows better codegen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks for the suggestion

@pierre-aimi
Copy link
Contributor Author

@SRombauts I think we might be good to go now. Meson build works, plus a few things fixed up.

@SRombauts
Copy link
Owner

Hello,
I merged other PRs and since this branch has been in conflict, would you be able to rebase, please?

@pierre-aimi
Copy link
Contributor Author

All done!

@SRombauts
Copy link
Owner

Oh, this is not working for MinGW: https://github.com/SRombauts/SQLiteCpp/actions/runs/4341176692/jobs/7580682901

../src/Column.cpp:21:25: error: external linkage required for symbol 'SQLite::INTEGER' because of 'dllexport' attribute 21 | SQLITECPP_API const int INTEGER = SQLITE_INTEGER;

@pierre-aimi
Copy link
Contributor Author

@SRombauts I just installed MinGW and fixed it.
Because gcc is defined, I had to add
#if defined(_WIN32)&& !defined(__GNUC__) && defined(SQLITECPP_COMPILE_DLL)
in order to exclude MinGW from the Windows import/export defines.

I also added -Wno-attributes to the Meson build because the exported const items already have internal linkage and therefore generate loads of warnings.
See this for more details - https://stackoverflow.com/questions/26025708/warning-visibility-attribute-ignored-symbol-visibility-c-gcc

@SRombauts SRombauts merged commit 3ef5b1d into SRombauts:master Mar 7, 2023
@SRombauts
Copy link
Owner

Thanks a lot for your contribution!
It's awesome to finally have this in, good job

@pierre-aimi
Copy link
Contributor Author

Happy to help!

This was referenced Apr 1, 2023
@SRombauts SRombauts mentioned this pull request Aug 18, 2023
@SRombauts SRombauts changed the title Dllexport import Dllexport import using BUILD_SHARED_LIBS Aug 18, 2023
@SRombauts SRombauts changed the title Dllexport import using BUILD_SHARED_LIBS DLL export/import using BUILD_SHARED_LIBS Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants