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

Updated library exports #8198

Merged
merged 2 commits into from Dec 7, 2017
Merged

Updated library exports #8198

merged 2 commits into from Dec 7, 2017

Conversation

mshabunin
Copy link
Contributor

@mshabunin mshabunin commented Feb 13, 2017

replaces #8073
build with opencv/opencv_contrib#996

This pullrequest changes

  • use GenerateExportHeader
  • fixed problems with exports and imports
build_world:Win64 MSVS2017 OpenCL=ON
#build_shared:Win64 OpenCL=OFF
allow_multiple_commits=1

@mshabunin mshabunin changed the title Cmakeex Updated library exports Feb 13, 2017
@mshabunin mshabunin force-pushed the cmakeex branch 5 times, most recently from cc678f4 to a2c3f75 Compare February 20, 2017 09:17
@vpisarev
Copy link
Contributor

vpisarev commented May 3, 2017

@mshabunin, looks like the patch needs to be updated; sorry for not merging it earlier :(

@mshabunin
Copy link
Contributor Author

@vpisarev , this patch is not compatible with #8461, because I've removed all CV_EXPORTS statements in the ts module. First we need to choose either #8461 (build and install ts module in separate package during OpenCV build) either #8468 (separately build standalone ts library). After that I will update this patch.

@@ -879,6 +878,8 @@ macro(_ocv_create_module)
COMPILE_PDB_OUTPUT_DIRECTORY ${LIBRARY_OUTPUT_PATH}
LIBRARY_OUTPUT_DIRECTORY ${LIBRARY_OUTPUT_PATH}
RUNTIME_OUTPUT_DIRECTORY ${EXECUTABLE_OUTPUT_PATH}
DEFINE_SYMBOL CVAPI_EXPORTS
POSITION_INDEPENDENT_CODE TRUE
Copy link
Member

Choose a reason for hiding this comment

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

"PIC" is needed for Python/Java bindings only.
For using OpenCV with applications it is not required at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a port of block removed from OpenCVCompilerOptions.cmake. We always enable PIC because static libraries can be used to build some shared libraries, I wanted to preserve this ability.

@@ -925,7 +921,7 @@ macro(_ocv_create_module)
if(OPENCV_MODULE_${the_module}_HEADERS AND ";${OPENCV_MODULES_PUBLIC};" MATCHES ";${the_module};")
foreach(hdr ${OPENCV_MODULE_${the_module}_HEADERS})
string(REGEX REPLACE "^.*opencv2/" "opencv2/" hdr2 "${hdr}")
if(NOT hdr2 MATCHES "private" AND hdr2 MATCHES "^(opencv2/?.*)/[^/]+.h(..)?$" )
if(NOT hdr2 MATCHES "opencv2/${the_module}/private.*" AND hdr2 MATCHES "^(opencv2/?.*)/[^/]+.h(..)?$" )
Copy link
Member

Choose a reason for hiding this comment

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

Need to revert this change.

#include "opencv2/cv_exports.h"

// fix for Android FAT_JAVA_LIB: symbols should have default visibility even in static libraries
#ifdef __ANDROID__
Copy link
Member

Choose a reason for hiding this comment

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

Comment is not consistent with following code.
Android application native library should not export OpenCV API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We build native static libraries and "fat Java lib" for Android in one build (-DBUILD_SHARED_LIBS=OFF -DBUILD_FAT_JAVA_LIB=ON). We need to export all symbols for the second artifact. Currently all symbols are exported in static builds on all platforms, this PR changes this behavior for all platforms except Android. I can only suggest reworking "fat Java lib" artifact, so that it will be produced same way as opencv_world module (-DBUILD_SHARED_LIBS=ON -DBUILD_opencv_world=ON -DBUILD_FAT_JAVA_LIB=ON), but it cat take more time to investigate.

Copy link
Member

Choose a reason for hiding this comment

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

Fat Java library is artifact for Android manager which is deprecated.
If application doesn't use Java API (doesn't require libopencv_java.so), so there is no reason to bundle whole OpenCV code into Android application.
More accurate way is no link with application JNI library with static OpenCV libraries - libopencv_core.a/etc. So we need to drop global exported symbols from these OpenCV static libraries.

So, there are 2 different builds:

  • static libopencv_<>.a with global exported symbols => libopencv_java.so
  • static libopencv_<>.a without global exported symbols and without any libopencv_java.so. Useful for using OpenCV API from native C++ code only (without any OpenCV Java bindings).

#ifdef _MSC_VER
#define CV_EXPORTS_TEMPLATE
#else
#define CV_EXPORTS_TEMPLATE CV_EXPORTS
Copy link
Member

Choose a reason for hiding this comment

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

CV_EXPORTS_TEMPLATE is not expected by bindings parser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently bindings generators do not support templates, so we don't need it for now. We can easily add CV_EXPORTS_TEMPLATE_W macro if such functionality will be implemented.

@alalek
Copy link
Member

alalek commented Dec 4, 2017

I still do not understand what kind of problem should be resolved by this patch.
It removes OpenCV's CV_EXPORTS (and removes ability to adjust/customize this macro) and replaces with some CMake stuff (which probably depends on used CMake version). I believe this approach has very limited usage. Android "block" is an example that CMake can't target anything, especially cross-platform cases with custom toolchains.

@mshabunin
Copy link
Contributor Author

This patch does not remove CV_EXPORTS macro, it changes the macro behavior depending on build configuration: dynamic builds will have same definitions as current version does, static builds will set it to empty.
The GenerateExportHeader module from cmake is rather simple and we can replace it with our own generated header when we need it, but I think that at this moment we can use it as-is. Problem with Android block takes its' roots in opencv_java module and "non-trivial" "fat Java lib" feature implementation. We can remove it if we will rework the Android package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants