google / cctz

CCTZ is a C++ library for translating between absolute and civil times using the rules of a time zone.
Apache License 2.0
596 stars 166 forks source link

cmake windows shared builds do not export symbols #72

Open sigmoidal opened 6 years ago

sigmoidal commented 6 years ago

When building shared libs on windows, using cmake and -DBUILD_SHARED_LIBS=On, no .lib file is produced, which in turn doesn't allow a consumer to link against cctz.

Ideally, symbols should be properly exported (see https://msdn.microsoft.com/en-us/library/a90k134d.aspx), then a lib file will be generated.

A not-so-elegant, but working solution may be to enable: CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS, which I had success with, however for optimal results you should only export what should be exported.

This workaround is as simple as adding:

set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS YES CACHE BOOL "Export all symbols")

or running the cmake config command with -DCMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=True

thanks.

Sarcasm commented 6 years ago

Until this is officially supported by cctz in general, I think the correct way is to use the CMake command line option: -DCMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=True. Overriding the cache from the CMakeLists.txt should be avoided, if the CMakeLists.txt needs to be touched, it would be slightly cleaner to specify the target property WINDOWS_EXPORT_ALL_SYMBOLS to the targets.

I think the real fix is to introduce some export macros so that cctz is a better Windows citizen. I expect the Bazel build could also benefit from this.

Maybe something along these lines (though I'm not Windows expert):

#if defined(WIN32) || defined(_WIN32)
#define CCTZ_EXPORT __declspec(dllexport)
#else
#define CCTZ_EXPORT [[gnu::visibility("default")]]
#endif

...

class CCTZ_EXPORT time_zone { ... };

By the way, do you have a minimal example to test what is not working here?

sigmoidal commented 6 years ago

I think the solution to export symbols is the right way to go, just like most libraries.

I don't have an example project to offer as I use upstream's example1.cc with a conan recipe, since I am helping package cctz for the conan package manager (https://github.com/bincrafters/conan-cctz), thus we run libs through CI and use them as dependencies for other projects, so problems become quickly apparent.

There isn't anything special I am doing, if you try to compile cctz on windows with -DBUILD_SHARED_LIBS=On you will notice no lib is created, thus one cannot link against it.

I'd be happy to help if necessary.

devbww commented 6 years ago

This "declspec" question has come up before, but perhaps this time there is enough expertise out there to be able to come to some conclusion.

So, my question is, "where would this CCTZ_EXPORT be applied?"

It seems to me that every current external symbol would need to be exported, so I'm not sure why we shouldn't just make that the default, like it is for non-Windows platforms.

sigmoidal commented 6 years ago

I want to clarify that I don't use cctz currently, so I don't have a clear picture of the use cases and respective functions that may be candidates for the exported symbol table.

You can do something like have this in a header file:

#define _CCTZ_EXPORTS_H__

#ifdef _WIN32
    #ifdef CCTZ_EXPORTS
        #define CCTZ_API __declspec(dllexport)
    #else
        #define CCTZ_API __declspec(dllimport)
    #endif
#else
    #define CCTZ_API
#endif

#endif 

a) decorate classes/functions to be exported with CCTZ_API: e.g. class CCTZ_API time_zone { ... }; b) When building the library using CCTZ_EXPORTS the dll export table will be generated. c) consumers won't have to do anything (they don't specify CCTZ_EXPORTS usually), thus functions will be imported.

Sarcasm commented 6 years ago

IIUC, Bradley basically suggested we use WINDOWS_EXPORT_ALL_SYMBOLS. Maybe it makes sense to do that now and use something a finer grained method later if needed.

devbww commented 6 years ago

IIUC, Bradley basically suggested we use WINDOWS_EXPORT_ALL_SYMBOLS.

Right. The direction to "decorate classes/functions to be exported with CCTZ_API" seems brittle and noisy when every external symbol should be, well, external.

I'm also unsure why shared libraries and "non-shared" libraries should be different.

sigmoidal commented 6 years ago

I consider this a solved issue as far as the original problem is concerned which was that no symbols were exported at all.

I don't however agree that we are discussing external symbols here and I also disagree that what I am proposing is brittle and noisy. We are talking about exported symbols.

The discussion as far as I understand is about (a) exporting every single function and class in the library (which the cmake approach offers for simplicity) vs (b) a clean set of functions/classes intended for use by consumer end-user software.

As far as the end users are concerned I doubt what I am proposing will produce libs that are different in functionality between their shared and static versions. Is there something specific about cctz that would cause this?

Since include/cctz is the public interface of cctz, why would the exports table include all the internal functionality from src/*.h, especially when these internal headers are not distributed with the generated lib.

I understand this may be low priority or unnecessary at this point for cctz, but thanks for discussing it.

devbww commented 6 years ago

Sorry for being slow to respond.

I'm happy to admit that my "every external symbol should be, well, external" assertion was incorrect. There are plenty of "internal" symbols that are only external because they are defined-in and referenced-from different translation units. Having them appear to be internal to the final library, be it shared or not, would be fine.

So, your suggestion stands as (given a suitable definition for CCTZ_API) "decorate classes/functions to be exported with CCTZ_API". OK. I'll revamp a previous question then as to whether anyone knows exactly where those decorations need to go.

Thanks.

sigmoidal commented 6 years ago

Thanks for responding and especially for taking this positive look at my suggestion.

olivier-fs commented 1 year ago

Hi!

Bumping this old but still open issue.

Of course using CMake CLI option -DCMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=True works. But it exports tons of symbols that should probably have been kept private/internal. With latest CMake 3.27.4 I get 220 exported symbols in cctz.dll.

define CCTZ_API __declspec(dllexport)

This xxx_API / xxx_EXPORTS approach is used by almost every library that has to deal with the Windows linker "dynamic loading but still static linking" old but forever annoying problem with DLLs.

Useless entries in DLL's symbol table slows down the startup of processes for no reason.

There hasn't been a cctz release for a lonnnng time. I agree that it is a low priority issue, but may be this could be closed now by sprinkling those CCTZ_API.

Thanks!

devbww commented 1 year ago

it is a low priority issue, but may be this could be closed now by sprinkling those CCTZ_API.

Agreed. And I would happily merge a PR from a Windows developer who can determine what to sprinkle and where, and how to test it.