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

Add options for building and installing shared, static libraries #70

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sizeofvoid
Copy link

This PR adds cmake options for building and installing shared and/or static libraries. All combinations were tested on a UNIX-like system. Therefore a test under Windows is missing.

@mbunkus mbunkus requested a review from robUx4 January 2, 2021 19:11
@mbunkus
Copy link
Contributor

mbunkus commented Jan 2, 2021

@robUx4 Can you test this PR on Windows, please?

@robUx4
Copy link
Contributor

robUx4 commented Jan 3, 2021

I get these compilation errors (VS Code with Clang 11 from Visual Studio 2019)

[build] C:\PROGRA~2\MICROS~3\2019\Preview\VC\Tools\Llvm\x64\bin\clang.exe -DEBML_DLL -DEBML_DLL_EXPORT -DHAVE_WINAPIFAMILY_H -I. -I../ -g -Xclang -gcodeview -O0 -D_DEBUG -D_DLL -D_MT -Xclang --dependent-lib=msvcrtd -fvisibility-inlines-hidden -std=gnu++14 -MD -MT CMakeFiles/ebml.dir/src/EbmlDate.cpp.obj -MF CMakeFiles\ebml.dir\src\EbmlDate.cpp.obj.d -o CMakeFiles/ebml.dir/src/EbmlDate.cpp.obj -c ../src/EbmlDate.cpp
[build] ../src/EbmlDate.cpp:40:24: error: definition of dllimport static field not allowed
[build] const uint64 EbmlDate::UnixEpochDelay = 978307200; // 2001/01/01 00:00:00 UTC
[build]                        ^
[build] ..\ebml/EbmlDate.h:46:7: note: attribute is here
[build] class EBML_DLL_API EbmlDate : public EbmlElement {
[build]       ^
[build] .\ebml_export.h:15:39: note: expanded from macro 'EBML_DLL_API'
[build] #      define EBML_DLL_API __declspec(dllimport)
[build]                                       ^
[build] ../src/EbmlDate.cpp:42:11: warning: 'libebml::EbmlDate::EbmlDate' redeclared without 'dllimport' attribute: 'dllexport' attribute added [-Winconsistent-dllimport]
[build] EbmlDate::EbmlDate(const EbmlDate & ElementToClone)
[build]           ^
[build] ..\ebml/EbmlDate.h:49:5: note: previous declaration is here
[build]     EbmlDate(const EbmlDate & ElementToClone);
[build]     ^
[build] ../src/EbmlDate.cpp:48:21: warning: 'libebml::EbmlDate::ReadData' redeclared without 'dllimport' attribute: 'dllexport' attribute added [-Winconsistent-dllimport]
[build] filepos_t EbmlDate::ReadData(IOCallback & input, ScopeMode ReadFully)
[build]                     ^
[build] ..\ebml/EbmlDate.h:80:15: note: previous declaration is here
[build]     filepos_t ReadData(IOCallback & input, ScopeMode ReadFully = SCOPE_ALL_DATA);
[build]               ^
[build] ../src/EbmlDate.cpp:66:21: warning: 'libebml::EbmlDate::RenderData' redeclared without 'dllimport' attribute: 'dllexport' attribute added [-Winconsistent-dllimport]
[build] filepos_t EbmlDate::RenderData(IOCallback & output, bool /* bForceRender */, bool  /* bWithDefault */)
[build]                     ^
[build] ..\ebml/EbmlDate.h:91:15: note: previous declaration is here
[build]     filepos_t RenderData(IOCallback & output, bool bForceRender, bool bWithDefault = false);
[build]               ^
[build] ../src/EbmlDate.cpp:78:16: warning: 'libebml::EbmlDate::IsSmallerThan' redeclared without 'dllimport' attribute: 'dllexport' attribute added [-Winconsistent-dllimport]
[build] bool EbmlDate::IsSmallerThan(const EbmlElement *Cmp) const
[build]                ^
[build] ..\ebml/EbmlDate.h:78:18: note: previous declaration is here
[build]     virtual bool IsSmallerThan(const EbmlElement *Cmp) const;
[build]                  ^
[build] 4 warnings and 1 error generated.

There are 2 errors.

  • the constant cannot be exported as a DLL variable. A define would work but would break API+ABI.
  • the methods defined in the C++ must be set with the export type as well.

@robUx4
Copy link
Contributor

robUx4 commented Jan 3, 2021

Adding the EBML_DLL_API in front of method definitions doesn't help much:

../src/EbmlDate.cpp:42:24: error: dllimport cannot be applied to non-inline function definition

Now given it's built with EBML_DLL_EXPORT I would assume it's building the DLL version. In that case it should be using dllexport and not dllimport. dllimport would be for the user app to import the methods it's using. The problem is in ebml_export.h which is using the ebml_export.h generated by CMake which uses ebml_EXPORTS to define the trigger between dllimport and dllexport instead of EBML_DLL_EXPORT.

Defining ebml_EXPORTS instead of EBML_DLL_EXPORT in the CmakeLists.txt file fixes the issue.

Copy link
Contributor

@robUx4 robUx4 left a comment

Choose a reason for hiding this comment

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

Changes to EBML_DLL_EXPORT needed


target_compile_definitions(ebml
PUBLIC EBML_DLL
PRIVATE EBML_DLL_EXPORT)
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to be ebml_EXPORTS, not EBML_DLL_EXPORT.

Copy link
Contributor

Choose a reason for hiding this comment

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

The libmatroska version doesn't have this and it's exporting the DLL symbols correctly. Removing this target_compile_definitions completely also fixes the issue.

@robUx4
Copy link
Contributor

robUx4 commented Jan 3, 2021

Also it seems both the static and dynamic builds generate a ebml.lib in the same folder. One of which is overwritten by the other when building both targets. They should have different names or should not be buildable at the same time.

There are also an EBMLTarget-debug.cmake and EBMLTarget-release.cmake that are generated that only export the DLL variant. Not sure if this is normal or it's only used when importing as a DLL.

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.

3 participants