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

Should we "import" symbols by default (Windows environment) ? #1821

Closed
FilipGh opened this issue Jan 3, 2020 · 7 comments
Closed

Should we "import" symbols by default (Windows environment) ? #1821

FilipGh opened this issue Jan 3, 2020 · 7 comments
Labels

Comments

@FilipGh
Copy link

FilipGh commented Jan 3, 2020

In file proj.h, some macro definitions are set in order to import or export symbols in DLL:

#ifndef PROJ_DLL
#ifdef PROJ_MSVC_DLL_EXPORT
#define PROJ_DLL __declspec(dllexport)
#elif defined(PROJ_MSVC_DLL_IMPORT)
#define PROJ_DLL __declspec(dllimport)
#else
#define PROJ_DLL
#endif
#endif

When linking an executable against proj_6_x.dll, it is mandatory to add the following option in the Visual C++ compiling command line:
/D PROJ_MSVC_DLL_IMPORT

Shouldn't it be easier to "import" symbols by default, as:

#ifndef PROJ_DLL
   #ifdef PROJ_MSVC_DLL_EXPORT
      #define PROJ_DLL __declspec(dllexport)
   #else
      #define PROJ_DLL __declspec(dllimport)
   #endif
#endif

at least in a Windows environment ?

@rouault
Copy link
Member

rouault commented Jan 3, 2020

is mandatory to add the following option in the Visual C++ compiling command line:

That's where things are mysterious to me, and there's something odd in our current setup. __declspec(dllimport) shouldn't be mandatory according to https://blogs.msdn.microsoft.com/russellk/2005/03/20/lnk4217/

@QuLogic
Copy link
Contributor

QuLogic commented Jan 4, 2020

linking an executable against proj_6_x.dll,

Shouldn't you be linking against a .lib, which I think would have the list of exported things in it?

@FilipGh
Copy link
Author

FilipGh commented Jan 6, 2020

In bumped into some linking error when compiling PROJ-JNI.
As @QuLogic says, I do link with proj_6_2.lib on the command line.
The weird thing is that I have no link errors with classes/methods/functions. The only missing symbols are :

  • osgeo::proj::common::UnitOfMeasure::SCALE_UNITY (METER, RADIAN, ...)
  • pj_release

The only way I could compile and link PROJ-JNI C++ part was to add the following line in CMakeList.txt :
target_compile_options(proj-bind PUBLIC "/D PROJ_MSVC_DLL_IMPORT")

@rouault
Copy link
Member

rouault commented Jan 6, 2020

when compiling PROJ-JNI.

Ah indeed that makes sense then. The blog post I mentionned above does mention that explicit dllimport is needed for global variables. That said, now that PROJ-JNI is queued for removal (#1825), this ticket is probably obsolete

@kbevers
Copy link
Member

kbevers commented Jan 7, 2020

@rouault PROJ-JNI is this: https://github.com/Kortforsyningen/PROJ-JNI/, not the old JNI-wrapper within PROJ itself.

@FilipGh is trying to make PROJ integrate smoothly with the new bindings on Windows.

@rouault
Copy link
Member

rouault commented Jan 11, 2020

Reading a bit more about that, I'm not sure we can unconditionnaly define PROJ_MSVC_DLL_IMPORT for _MSC_VER when including proj.h, because that would fail for people doing static linking.

And looking for example at src/bin_cct.cmake, we can see that both conditions (MSVC + dll build) are used to determine when PROJ_MSVC_DLL_IMPORT is defined

if(MSVC AND BUILD_LIBPROJ_SHARED)
  target_compile_definitions(cct PRIVATE PROJ_MSVC_DLL_IMPORT=1)
endif()

@stale
Copy link

stale bot commented Mar 11, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

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

No branches or pull requests

4 participants