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

FEAT: Transition to CMake #152

Merged
merged 54 commits into from
Mar 24, 2024
Merged

Conversation

MementoRC
Copy link
Sponsor Collaborator

@MementoRC MementoRC commented Mar 9, 2024

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:
#123

Copy link

codecov bot commented Mar 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@           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 MementoRC changed the title (ref) use logging module. add CMake from _build_clib FEAT: Transition to CMake Mar 9, 2024
@MementoRC MementoRC requested a review from ofek March 21, 2024 19:15
@MementoRC
Copy link
Sponsor Collaborator Author

MementoRC commented Mar 21, 2024

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
Copy link
Sponsor Collaborator Author

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
Copy link
Owner

ofek commented Mar 22, 2024

Nice!

@MementoRC
Copy link
Sponsor Collaborator Author

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
Copy link
Sponsor Collaborator Author

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
Copy link
Owner

ofek commented Mar 24, 2024

Very cool

@MementoRC MementoRC merged commit 5288cb8 into ofek:master Mar 24, 2024
14 checks passed
@MementoRC
Copy link
Sponsor Collaborator Author

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

@MementoRC MementoRC deleted the feat/add-cmake-extension branch March 24, 2024 20:34
@ofek
Copy link
Owner

ofek commented Mar 24, 2024

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

@MementoRC
Copy link
Sponsor Collaborator Author

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

@MementoRC
Copy link
Sponsor Collaborator Author

@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
Copy link
Sponsor Collaborator Author

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
Copy link
Sponsor Collaborator Author

MementoRC commented Mar 24, 2024

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
Copy link
Owner

ofek commented Mar 24, 2024

If it's okay with you I'm just going to reset to 5e46aca and then force push

@ofek
Copy link
Owner

ofek commented Mar 24, 2024

Fixed

@MementoRC
Copy link
Sponsor Collaborator Author

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
Copy link
Owner

ofek commented Mar 24, 2024

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

@MementoRC
Copy link
Sponsor Collaborator Author

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
Copy link
Owner

ofek commented Mar 24, 2024

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

@MementoRC
Copy link
Sponsor Collaborator Author

MementoRC commented Mar 24, 2024

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
Copy link
Owner

ofek commented Mar 24, 2024

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

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.

None yet

2 participants