Skip to content

WIP: visibility example for cmake #1695

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

WIP: visibility example for cmake #1695

wants to merge 1 commit into from

Conversation

purpleKarrot
Copy link
Contributor

@purpleKarrot purpleKarrot commented Jun 30, 2025 •
edited
Loading

Alternative to #1674 and #1677.

@purpleKarrot
Copy link
Contributor Author

The fact that a build called "Windows (VS 2022, static)" is failing with the message "unresolved external symbol __imp_..." is concerning. The fact that private symbols cannot be resolved indicates that visibility is working correctly and that the library is not static. Unfortunately, the compiler command line is not visible on github, so this needs further investigation.

@real-or-random
Copy link
Contributor

The fact that private symbols cannot be resolved indicates (...) that the library is not static.

I don't think so. It builds a ".lib" file: https://github.com/bitcoin-core/secp256k1/actions/runs/15979380201/job/45069998808?pr=1695#step:4:57

See also https://github.com/bitcoin-core/secp256k1/actions/runs/15965328923/job/45024576152#step:5:36, which shows that no ".dll" is created on master. (We should run file also on "*.lib" files...)

@real-or-random
Copy link
Contributor

real-or-random commented Jul 1, 2025 •
edited
Loading

I suspect that building the exes fails because SECP256K1_STATIC is not actually set for some reason.

Unfortunately, the compiler command line is not visible on github, so this needs further investigation.

Yes, adding this to CI would be useful in any case.

@purpleKarrot
Copy link
Contributor Author

The fact that private symbols cannot be resolved indicates (...) that the library is not static.

I don't think so. It builds a ".lib" file: https://github.com/bitcoin-core/secp256k1/actions/runs/15979380201/job/45069998808?pr=1695#step:4:57

I don't find that convincing.

Creating library D:/a/secp256k1/secp256k1/build/lib/RelWithDebInfo/noverify_tests.lib and object D:/a/secp256k1/secp256k1/build/lib/RelWithDebInfo/noverify_tests.exp
  noverify_tests.vcxproj -> D:\a\secp256k1\secp256k1\build\bin\RelWithDebInfo\noverify_tests.exe
  secp256k1.vcxproj -> D:\a\secp256k1\secp256k1\build\lib\RelWithDebInfo\libsecp256k1.lib

This does not indicate that a static library for libsecp256k1 is created, it just means that noverify_tests links against libsecp256k1.lib, which could be a static library or an import library for an associated .dll. Both of these have the same file extension.

@real-or-random
Copy link
Contributor

Ok, maybe this is more convincing:

@purpleKarrot purpleKarrot force-pushed the visibility branch 2 times, most recently from 561934d to 68c7b89 Compare July 1, 2025 10:25
@purpleKarrot
Copy link
Contributor Author

I see the issue. The usage requirements of the secp256k1 target are not applied, because benchmarks and tests don't actually build against that target. They build against secp256k1_precomputed and secp256k1_asm instead.

@purpleKarrot purpleKarrot force-pushed the visibility branch 2 times, most recently from 4777696 to fa71348 Compare July 1, 2025 13:16
@theuni
Copy link
Contributor

theuni commented Jul 1, 2025

A few things:

  1. As discussed in the other PRs, it's clear that this would be much simpler if we only supported CMake, but that's unfortunately not the reality at the moment. The majority of the cruft in the header exists to support the combination of buildsystems.
  2. Could you please split this into refactor vs behavioral change commits for easier review? There's a lot going on here and the some of the rationale isn't obvious at first glimpse.

This was referenced Jul 2, 2025
Merged
Closed
@real-or-random
Copy link
Contributor

If we don't use this approach in the end, we should recycle the CMake snippets code that sets SECP256K1_STATIC for the parent when consuming the library.

real-or-random added a commit that referenced this pull request Jul 21, 2025
@real-or-random
Merge #1696: build: Refactor visibility logic and add override
7c33804 
c82d84b build: add CMake option for disabling symbol visibility attributes (Cory Fields)
ce79238 build: Add SECP256K1_NO_API_VISIBILITY_ATTRIBUTES (Tim Ruffing)
e5297f6 build: Refactor visibility logic (Tim Ruffing)

Pull request description:

  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 setting `hidden` 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, set `CMAKE_C_VISIBILITY_PRESET` from a parent project?

ACKs for top commit:
  hebasto:
    ACK c82d84b, I have reviewed the code and it looks OK.

Tree-SHA512: dad36c32a108d813e8d4e1849260af43f79a9aa8fbfb9a42b07d737e0467924a511110df0a2c6761539a1587b617a1b11123610a3db9d4cdf2b985dfb3eb21da
@real-or-random
Copy link
Contributor

If we don't use this approach in the end, we should recycle the CMake snippets code that sets SECP256K1_STATIC for the parent when consuming the library.

@purpleKarrot
This PR is primarily superseded by #1696 then, but do you think some of the changes here will still make sense?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
build feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@purpleKarrot @real-or-random @theuni