ofek / coincurve

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

FEAT: Transition to CMake #152

Closed MementoRC closed 6 months ago

MementoRC commented 6 months ago

Adds the CMake Clib class used in the working full flow. I put the helping functions in setup.py, so only one file is changed, but ideally they would be in setup_support.py

None of the builds uses this class yet. In the working build, I removed make, BUILDING_FOR_WINDOWS, changed Distribution, etc ...

So I'd thought i'd make PRs that should be simpler to review with the building blocks while maintaining the passing builds

This should resolve: https://github.com/ofek/coincurve/issues/123

codecov[bot] commented 6 months ago

Codecov Report

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

Project coverage is 85.68%. Comparing base (5e46aca) to head (5288cb8).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #152 +/- ## ======================================= 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 6 months ago

@ofek This is basically ready, at least you can look at the core idea - This is the Windows NATIVE build/test

Next step will be to clean-up all that is no longer needed, add the demonstration of using system_lib (with CONDA) - Perhaps I should separate the workflow with a build/test one that is not actually meant to publish a wheel

ofek commented 6 months ago

I invited you to be a maintainer, did you receive the email?

MementoRC commented 6 months ago

Thank you. Yes, I did, I saw it just before I had to step off. I accepted, thanks

MementoRC commented 6 months ago

Wow, this is interesting, it finds the library, but somehow still fails by trying to build_clib (good thing I force a inexistent UPSTREAM_REF to trigger) - I will look at it tomorrow.

Looks like I failed once again to separate the PRs, let me clean-up this one for your CMake review, then add the tests for SHARED mode and CONDA environment in a PR on top of this one

MementoRC commented 6 months ago

I started to look into refactoring coincurve with CMake, meaning, not only build libsecp256k1, but doing the CFFI binding with CMake and configure the package with scikit-build-core. It looks promising, but a bit of a steep learning curve. A long time ago I was a decent Makefile engineer, but looking at CMake makes me feel like a dinosaur (which I am, but come don't rub it in my face that hard!)

Point being, maybe we cans start with discussing this PR, which brings Native windows build online!

Update: Getting there

[ 21%] Built target secp256k1
[ 26%] Building C object CMakeFiles/_libsecp256k1.dir/generated_c_file/_cffi_build.c.o
[ 31%] Linking C shared library lib_libsecp256k1.so
[ 31%] Built target _libsecp256k1

then

[ 21%] Built target secp256k1
[ 26%] Linking C shared library _libsecp256k1.so
MementoRC commented 6 months ago

Ok, first milestone:

[  5%] Built target cffi-c-binding
[ 26%] Linking C shared library libsecp256k1.so
[ 26%] Built target secp256k1
[ 31%] Building C object CMakeFiles/_libsecp256k1.dir/generated_c_file/_cffi_build.c.o
[ 36%] Linking C shared module _libsecp256k1.cpython-311-x86_64-linux-gnu.so
ofek commented 6 months ago

Nice!

MementoRC commented 6 months ago

Another milestone: This was not needed, but simplify updating from new versions of SECP256K1 (and would prevent the bug I introduced last time!)

[ 76%] Generating headers for CFFI.
[ 76%] Built target secp256k1.h
[ 80%] Generating headers for CFFI.
[ 80%] Built target secp256k1_ecdh.h
[ 84%] Generating headers for CFFI.
[ 84%] Built target secp256k1_ellswift.h
[ 88%] Generating headers for CFFI.
[ 88%] Built target secp256k1_extrakeys.h
[ 92%] Generating headers for CFFI.
[ 92%] Built target secp256k1_preallocated.h
[ 96%] Generating headers for CFFI.
[ 96%] Built target secp256k1_recovery.h
[100%] Generating headers for CFFI.
[100%] Built target secp256k1_schnorrsig.h
MementoRC commented 6 months ago

It looks like 'Houston, we have lift-off':

(base) coincurve: python -m build .
* Creating virtualenv isolated environment...
...
*** scikit-build-core 0.8.2 (sdist)
...
*** scikit-build-core 0.8.2 using CMake 3.28.1 (wheel)
...
*** Making wheel...
*** Created coincurve-19.0.2.dev65+g5288cb8.d20240324-cp311-cp311-manylinux_2_31_x86_64.whl...
Successfully built coincurve-19.0.2.dev65+g5288cb8.d20240324.tar.gz and coincurve-19.0.2.dev65+g5288cb8.d20240324-cp311-cp311-manylinux_2_31_x86_64.whl

The version is created with the new _version.py, thus its unfamiliarity ...

ofek commented 6 months ago

Very cool

MementoRC commented 6 months ago

Wow, you know that this PR is not the CMake one o.0 - I am still testing it on my fork

ofek commented 6 months ago

Yes I figured because I didn't see any new commits but am generally excited about your work here

MementoRC commented 6 months ago

@ofek BIG MISTAKE: I just committed to MASTER when I thought I would commit to MY fork

MementoRC commented 6 months ago

@ofek I propose to fix it by doing a revert of the 61 commits one after the other with the --no-commit:

git revert ... --no-commit

I am so used to not being able to commit to master that I was not careful, It's only when I did not see the PR message that I realized my mistake My apologies

MementoRC commented 6 months ago

I am going to prepare the list of commands and post them here - I am awaiting your feedback as to whether this is the best way to correct this

MementoRC commented 6 months ago

So it should actually only be the 6 last commits till: 5288cb82552ecaf1e72cd8997684800c82c32f4a

git revert 38060472b1b0578b21ea551f7dd95ff583ff1e1e --no-commit
git revert c5dda378d724766884de0960484d881af5fb9952 --no-commit
git revert 4b99d5bdbe0fa5932b50a3e7f6b98d6570e1c918 --no-commit
git revert 091d0334150e908f3099178642dea9d4674eb526 --no-commit
git revert 826ac861e5d2ae9919a045a196bc3803ef133aca --no-commit
git revert 935ce6528fe5c0e9c3c8c57e459684c622e89b72 --no-commit
ofek commented 6 months ago

If it's okay with you I'm just going to reset to https://github.com/ofek/coincurve/commit/5e46aca9a4745e06eb4a72f4902477dc30e860ec and then force push

ofek commented 6 months ago

Fixed

MementoRC commented 6 months ago

If you feel this is preferable. I have the 6 commits reverted locally - but, do as you feel is best - I am not very savvy in git. Also, I would refer to actually not be able to merge without PR first and your approval - I actually had to force-push a few times my fork because of similar mistakes

ofek commented 6 months ago

I originally copied the wrong one but I used what you said 5288cb82552ecaf1e72cd8997684800c82c32f4a

MementoRC commented 6 months ago

Wonderful, thank you. Right, I wondered about that because the revert of the merge did not work and I edited my previous message with the correct hash. Man, I sweated bullets, like, right after being trusted ... BAAM ... I stab in the back

Can you protect master against me committing without PR first? If not, I'd rather not be able to commit/merge then - I am prone to silliness that snowball into clusterF

ofek commented 6 months ago

Yes I just did protect the master branch from merges without pull requests and they now require one approval.

MementoRC commented 6 months ago

Good, thanks. With many contributors, it only took my arrival to highlight the benefit of branch protection ... sob - Sorry

Also, I tried to good that you fix it, I sonehow messed up my side and am stuck with a Reverting in master message :'( - Going to calm down for a bit, let my dog bite me for a bit

ofek commented 6 months ago

No worries, I should have done that long ago as it's good practice to do so.