hosseinmoein / DataFrame

C++ DataFrame for statistical, Financial, and ML analysis -- in modern C++ using native types and contiguous memory storage
https://hosseinmoein.github.io/DataFrame/
BSD 3-Clause "New" or "Revised" License
2.54k stars 313 forks source link

Windows: static lib broken #76

Closed SpaceIm closed 4 years ago

SpaceIm commented 4 years ago

DataFrame is more or less broken if static because it unconditionally adds dllexport/dllimport attributes.

For example this macro -found in VectorPtrView.h- is wrong for a static lib in Visual Studio:

#ifdef _WIN32
#  ifdef LIBRARY_EXPORTS
#    define LIBRARY_API __declspec(dllexport)
#  else
#    define LIBRARY_API __declspec(dllimport)
#  endif // LIBRARY_EXPORTS
#else
#  define LIBRARY_API
#endif // _WIN32

It should be something like this:

#if defined(_WIN32) && defined(DATAFRAME_SHARED)
#  ifdef LIBRARY_EXPORTS
#    define LIBRARY_API __declspec(dllexport)
#  else
#    define LIBRARY_API __declspec(dllimport)
#  endif // LIBRARY_EXPORTS
#else
#  define LIBRARY_API
#endif // _WIN32

If shared:

If static: no definition at build and consume time

A better approach would be to rely on generate_export_header cmake function: https://cmake.org/cmake/help/v3.0/module/GenerateExportHeader.html Therefore you would also be able to properly hide symbols with GNU compilers for shared lib.

hosseinmoein commented 4 years ago

@SpaceIm , I fixed the issue. You can look at it in https://github.com/hosseinmoein/DataFrame/commit/d5eb0a8ba0a4748135563fe7f7e9fd6bc005745e thanks HM

SpaceIm commented 4 years ago

@hosseinmoein In this modification, you didn't add any condition based on BUILD_SHARED_LIBS. For static buid, HMDF_SHARED must not be defined (and it doesn't really matter for LIBRARY_EXPORTS). Moreover, add_definitions is old CMake, prefer target_compile_definitions (PRIVATE for LIBRARY_EXPORTS because you don't want that users define this one at consume time, and PUBLIC for HMDF_SHARED).

Something like:

if(BUILD_SHARED_LIBS)
  target_compile_definitions(${LIBRARY_TARGET_NAME} PRIVATE LIBRARY_EXPORTS PUBLIC HMDF_SHARED)
endif()
hosseinmoein commented 4 years ago

@SpaceIm , The problem is I don't have access to a windows development environment and have to rely on Appveyor which is almost impossible to resolve things. But it seems that the DataFrame code would not work in static windows compilation. I get a lot of errors when compiling DateTime.cc and .h. That is why I left the compilation to be shared unconditionally.