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

cwalk: fix compilation when both shared and static is wanted #1566

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

Conversation

antoniovazquezblanco
Copy link
Contributor

This is a follow up of #1387

I am trying to improve on the already merged meson script by allowing compilation of both static and dynamic libs simultaneously with the proper arguments. This tryes to implement what @benoit-pierre suggested...
Also adding @likle from upstream...

From meson maintainers, would this be desirable or would a "def" file provide any other desirable behaviour?

Thanks

releases.json Outdated Show resolved Hide resolved
releases.json Outdated Show resolved Hide resolved
@antoniovazquezblanco
Copy link
Contributor Author

In MS compilers, this is producing a lot of warnings:

warning: 'cwk_path_change_segment' redeclared without 'dllimport' attribute: 'dllexport' attribute added [-Winconsistent-dllimport]

@benoit-pierre
Copy link
Contributor

From meson maintainers, would this be desirable or would a "def" file provide any other desirable behaviour?

AFAIK, that's a Windows only solution, and CWK_SHARED also impact the build on other platforms:

#if defined(_WIN32) || defined(__CYGWIN__)
#define CWK_EXPORT __declspec(dllexport)
#define CWK_IMPORT __declspec(dllimport)
#elif __GNUC__ >= 4
#define CWK_EXPORT __attribute__((visibility("default")))
#define CWK_IMPORT __attribute__((visibility("default")))
#else
#define CWK_EXPORT
#define CWK_IMPORT
#endif

#if defined(CWK_SHARED)
#if defined(CWK_EXPORTS)
#define CWK_PUBLIC CWK_EXPORT
#else
#define CWK_PUBLIC CWK_IMPORT
#endif
#else
#define CWK_PUBLIC
#endif

However, the library(…) call is missing gnu_symbol_visibility: 'hidden' to take full advantage of __attribute__((visibility("default"))) when using clang/gcc.

@benoit-pierre
Copy link
Contributor

redeclared without 'dllimport

That's because -DCWK_EXPORTS is needed too: from CMakeLists.txt, it should be unconditionally added do c_args.

Additionally, CWK_SHARED is part of the public interface:

target_compile_definitions(cwalk PUBLIC CWK_SHARED)

So it should be added to the compile_args of cwalk_dep.

@benoit-pierre
Copy link
Contributor

You'll need to declare 2 dependencies, and use meson.override_dependency(…) with static, so the right compile arguments are used for both static and shared variants.

@benoit-pierre
Copy link
Contributor

And switch to this syntax in the wrap:

[provide]
dependency_names = cwalk

@antoniovazquezblanco
Copy link
Contributor Author

Finally found some time to get back to this but...

You'll need to declare 2 dependencies, and use meson.override_dependency(…) with static, so the right compile arguments are used for both static and shared variants.

I do not know how to do this. Could you provide a detailed description please? Thanks @benoit-pierre !

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.

2 participants