ofek / coincurve

Cross-platform Python bindings for libsecp256k1
https://ofek.dev/coincurve/
Apache License 2.0
153 stars 46 forks source link

(feat) Build with CMAKE #150

Closed MementoRC closed 8 months ago

MementoRC commented 8 months ago

This is the framework that helps transition to using CMAKE, however it ties-in to the build_ext. Initially, it wasn't lear CMake could be used for all, so I maintained both CMake and Makefile, thus the use of 2 classes.

Continuous Development is quite hard, it is not clear how for example this type of transition can be made incrementally, at some point, a jump from Makefile to CMake would need to be made, , this is a learning op for me if you have any comment/suggestion or else

Like, do you prefer that I try to make lots of PRs with each 'feat' I think of, or just let you review all the changes bundled in #141?

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.68%. Comparing base (98fed8e) to head (299b903). Report is 1 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #150 +/- ## ======================================= Coverage 85.68% 85.68% ======================================= Files 11 11 Lines 566 566 Branches 67 67 ======================================= Hits 485 485 Misses 45 45 Partials 36 36 ```
MementoRC commented 8 months ago

@ofek this should be cleaner than #141 I know why it is currently failin and fixed itg, but I am trying to resolve a related issue locally, I'll update when it looks good

MementoRC commented 8 months ago

I don't understand why setuptools>=61 is not available, #141 uses the same workflow

ofek commented 8 months ago

It's building Python 3.6 https://github.com/ofek/coincurve/actions/runs/8072129335/job/22053309535?pr=150#step:3:293

MementoRC commented 8 months ago

I don't want to admit that I can no longer read these tiny fonts ... sob. Though, why? setup.py still has the python_requires, #141 has it in its pyproject.toml. This PR is trying to deviate the least from master, do you undestand why this happens?

ofek commented 8 months ago

Appears to be building the correct versions now and failing with a different error

MementoRC commented 8 months ago

Right, I was too eager. I changed the location of the library because it was getting included in the wheel, but then that broke the shared build that needs the library included in the package

MementoRC commented 8 months ago

@ofek, we seem to be getting close. From a coincurve stand-point this is ready (since coincurve uses STATIC only).

The SHARED is a optional feature, that you could decide to switch to, so I want to have all wheels tested

For conda, we need the SHARED build to work without building the lib, I have yet to add the wheel test, it is a bit trickier since conda has to be installed on the docker host used by cibuildwheel (I was only able to test on windows so far). I will add it if all the SHARED pass

ofek commented 8 months ago

If you can find a way to extract the building & linking of the underlying library outside of setuptools/setup.py I would much prefer that if it's easier. Then we could switch to a different backend like Hatchling which I wrote and maintain.

Offering in case it happens to be easier to just run some commands straight up.

MementoRC commented 8 months ago

@ofek Thanks, actually all the builds look fine, both with static and shared library using cibuildwheels, thus native on windows and macos-14 (native ARM).

The last problem I seem to have is that somehow I cannot set the _has_system_lib to True, it is always False - I am forcing the build to fail in this case by giving a non-existing REF, otherwise it would pass and I would have to go see in the log if it was built instead of using the conda library

I was hoping to avoid all these debugging commits, but it is windows again so I have to push.

MementoRC commented 8 months ago

@ofek I don't know how Hatchling works, but build/install of libsecp256k1 is in fact done outside of setuptools since in this PR, we use cmake, and specify everything like install location.

Actually, we could add a CMakeList.txt to build _libsecp256k1 along with libsecp256k1 ... I did not think of that

MementoRC commented 8 months ago

@ofek Okay, I got it! ... little issue of path definitions ... too bad it took me so many useless commits to get there. One day maybe I will be a cleaner commit-er ... though not in the coming few centuries :,(

MementoRC commented 8 months ago

Cool, it works, though coincurve seems incompatible with conda libsecp256k1 ... and ... I have no clue as to why :?

    _c_file_for_extension.obj : error LNK2001: unresolved external symbol secp256k1_context_static
    _c_file_for_extension.obj : error LNK2001: unresolved external symbol secp256k1_nonce_function_default
    _c_file_for_extension.obj : error LNK2001: unresolved external symbol secp256k1_ellswift_xdh_hash_function_prefix
    _c_file_for_extension.obj : error LNK2001: unresolved external symbol secp256k1_ecdh_hash_function_sha256
    _c_file_for_extension.obj : error LNK2001: unresolved external symbol secp256k1_nonce_function_bip340
    _c_file_for_extension.obj : error LNK2001: unresolved external symbol secp256k1_ellswift_xdh_hash_function_bip324
    _c_file_for_extension.obj : error LNK2001: unresolved external symbol secp256k1_ecdh_hash_function_default
    _c_file_for_extension.obj : error LNK2001: unresolved external symbol secp256k1_nonce_function_rfc6979
MementoRC commented 8 months ago

Keep thinking I am so close, no point closing the PR, that was 20 push ago

ofek commented 8 months ago

No luck?

MementoRC commented 8 months ago

It's progressing slowly since I am very bad at windows stuff, I am very close. I added a detection of shared vs static, since pkg-config does not tell as much, but it's tricky with windows, I actually kept the -2 in the conda lib, so it kept failing because of that

I finally got it cornered:

Running command Getting requirements to build wheel
    WARNING:root:DBG: C:/Miniconda/envs/test/Library\bin
    WARNING:root:DBG: secp256k1*.dll
    WARNING:root:[...', 'libsecp256k1-2.dll', ...]

Next push might be the one ... or not, little steps Calling it a day, though, it's quite painful and debilitating, i loaf windows - since this morning I kept thinking about completing the PR and moving to hashling