-
Notifications
You must be signed in to change notification settings - Fork 1.1k
build: Refactor visibility logic and add override #1696
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
base: master
Are you sure you want to change the base?
build: Refactor visibility logic and add override #1696
Conversation
fb5a833
to
ad46425
Compare
Yes, we can. |
ad46425
to
82b7412
Compare
LGTM. A few thoughts:
Those two would add up to: option(SECP256K1_ENABLE_VISIBILITY_ATTRIBUTES "Enable visibility attributes when compiling the library" ON)
if (NOT DEFINED CMAKE_C_VISIBILITY_PRESET)
set(CMAKE_C_VISIBILITY_PRESET hidden)
endif()
...
if(NOT SECP256K1_ENABLE_FUNCTION_VISIBILITY_ATTRIBUTES)
target_compile_definitions(secp256k1 PRIVATE SECP256K1_NO_FUNCTON_VISIBILITY_ATTRIBUTES)
endif() But I noticed while testing combinations of the above that it leads to broken outcomes for test binaries when simply disabling the To be robust against that, we'd need something like: option(SECP256K1_ENABLE_VISIBILITY_ATTRIBUTES "Enable visibility attributes when compiling the library" ON)
if (SECP256K1_ENABLE_FUNCTION_VISIBILITY_ATTRIBUTES AND NOT DEFINED CMAKE_C_VISIBILITY_PRESET)
set(CMAKE_C_VISIBILITY_PRESET hidden)
endif() Which of course would defeat the purpose. So I propose that we just skip trying to set tl;dr: I think this is the best solution so far, but propose adding a helper option to make life easier for parent projects: theuni@e22ed8c Core would then set |
True, but we also apply it to variables, e.g., Perhaps |
(edit: nevermind, got it now) |
Right, that's the just same problem as with your #1674.
Sounds good to me. |
I don't get why additional
(Both cmake and libtool set a define symbol when building a shared library. This symbol alone is insufficient to distinguish between three different cases. When using custom defines, the symbols set by those tools is redundant. The approach of using |
I've spent some time re-reading all the other competing PRs, including #1674, #1677, #1695), and discussing the topic offline with @purpleKarrot. Concept ACK on this one.
+1. |
I am failing to reason about all the different paths through that
I am a bit concerned that new libraries might follow the example set here. There should be a big disclaimer that points people to that GCC wiki page. |
include/secp256k1.h
Outdated
/* Consuming libsecp256k1 as a DLL. */ | ||
# define SECP256K1_API extern __declspec (dllimport) | ||
# endif | ||
#if !defined(SECP256K1_API) && defined(SECP256K1_NO_VISIBILITY_ATTRIBUTES) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My apologies if this was discussed earlier and I missed it, but it seems that SECP256K1_NO_VISIBILITY_ATTRIBUTES
should only be considered only when building the library, not when consuming it:
#if !defined(SECP256K1_API) && defined(SECP256K1_NO_VISIBILITY_ATTRIBUTES) | |
#if !defined(SECP256K1_API) && defined(SECP256K1_BUILD) && defined(SECP256K1_NO_VISIBILITY_ATTRIBUTES) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that it hurts to apply it when consuming the library. Or does it have any undesirable effects?
This changes the meaning of Why avoid the need to set
I doubt that's true. As explained by @theuni in #1674, there's no such macro. There's just
Sure, writing it from scratch would lead to simpler code but my point is that the aforementioned arguments are stronger. |
Co-authored-by: Tim Ruffing <me@real-or-random.org>
82b7412
to
203376c
Compare
Renamed to
Cherry-pick (and renamed the option accordingly). It would be nice to get reviews by the end of the week, then we may get this into 0.7.0. |
This is less invasive than #1695. The latter might be the right thing in a new library (and then we'd probably not support autotools in the first place), but any semantic change to this code has the potential to create news bug, or at least breakages for downstream users.
This is different from #1677 in that it does not set
hidden
explicitly. I agree with the comment in #1677 that settinghidden
violates the principle of least surprise.So this similar in spirit to #1674. So I wonder if this should also include
3eef736. I'd say no,
fvisibility
should then set by the user. But can you, in CMake, setCMAKE_C_VISIBILITY_PRESET
from a parent project?