coincurve icon indicating copy to clipboard operation
coincurve copied to clipboard

FEAT: Transition to scikit-build-core CMake-based build process

Open MementoRC opened this issue 10 months ago • 32 comments

This was initally started to transition to Hatchling. However, as I learned more about scikit-build-core, it seems that it handles all the steps required to build coincurve. However, I do not know Hatchling, so let me know your thoughts on how we should integate it.

This PR itself changes some of the original features of coincurve. CMake provides a convenient way to download the SECP256K1 library, so currently, I did not add this feature (though the current UPSTREAM_REF is used)

The other major addition is that the flow creates the CFFI source headers directly from the downloaded SECP256K1 original headers, so it should be quite straightforward to follow SECP256K1 updates.

The rest is fairly similar, create the SECP256K1 library (using the CMake they provide), create the CFFI C-code (rather than compile it with CFFI), then compile the C-Code and link it to SECP256K1 library.

The feature of using SYSTEM_LIB is similar, although I have not tested it as well as for the CMake-based PR.

MementoRC avatar Mar 27 '24 19:03 MementoRC

Codecov Report

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

Project coverage is 84.64%. Comparing base (07d332e) to head (fe052a6).

:exclamation: Current head fe052a6 differs from pull request most recent head 39d204f

Please upload reports for the commit 39d204f to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #162      +/-   ##
==========================================
- Coverage   85.68%   84.64%   -1.05%     
==========================================
  Files          11       12       +1     
  Lines         566      573       +7     
  Branches       67       68       +1     
==========================================
  Hits          485      485              
- Misses         45       52       +7     
  Partials       36       36              
Files Coverage Δ
src/coincurve/__about__.py 0.00% <ø> (ø)

codecov[bot] avatar Mar 27 '24 19:03 codecov[bot]

@ofek So this is a scikit-build-core backend build. I try to see how to use hatch instead, but have not understood yet how to link scikit-built-core to hatch. Since this is a cmake build, would it not be simpler to try to develop a hatch-cmake` hook?

MementoRC avatar Mar 28 '24 21:03 MementoRC

Henry (the maintainer) is working on making a plugin for Hatchling. Until then this current approach is fine. Which PR should I review? I don't quite know the plan.

ofek avatar Mar 28 '24 21:03 ofek

https://github.com/ofek/coincurve/pull/159 is ready, it only add verification for shared lib build. Or do you think it should not be added?

MementoRC avatar Mar 28 '24 22:03 MementoRC

This PR still has issues, but I think it is more maintainable than the setuptools+CMake. So the plan was to move to CMake, then to use hatch back-end and create the hooks to setuptools - but then you pointed me to scikit-build-core ... so we can see this PR as a parallel dev and choose how to move forward - I am not so good at plan (or communication for that matter)

Also, I had not realized that you know so much more than me on this subject (looking at how hatch is written), so I admit I am myself a bit confused as to your plans for coincurve - all I needed was to have coincurve on conda-forge, but I thought I could provide help in refactoring so that 1- we test the wheels on windows, 2 we upgrade the build to use CMake, which is where I think SECP256K1 is headed. As I was doing so, I also thought it'd be neat to reuse the framework to readily provide the experimental SECP256K1 that the issue was asking for, like coincurve[zkrp] of sorts

Do we need to better sync-up?

MementoRC avatar Mar 28 '24 22:03 MementoRC

Definitely please move forward just with scikit-build-core!

ofek avatar Mar 28 '24 22:03 ofek

So, if I undersand correctly, Henry will build a plugin for hatch which would use scikit-build-core and be able to interact with hatch, but I think what I thought we would have is a hatch plugin to interact/control scikit-build-core. With the current PR, do you see how coincurve woulb benefit from hatch

I am trying to update the config with something like:

[[tool.hatch.build.hooks.build-scripts.scripts]]
out_dir = "out"
commands = [
    "mkdir -p build_with_cmake",
    "cd build_with_cmake && cmake -S .. -B .",
    "cd build_with_cmake && cmake --build .",
]
artifacts = [

to try to understand how hatch fits/improves the current status

MementoRC avatar Mar 29 '24 23:03 MementoRC

The plugin would live at the build system layer i.e. Hatchling, which would invoke some logic within that other package. Hatch the CLI has nothing to do with this and we don't even have to use it if you don't want to.

ofek avatar Mar 29 '24 23:03 ofek

Right, I meant hatchling, but the backend would be scikit-build-core, with a tools.scikit-built.hatchling.xxx then what would we use it for? what is coincurve missing without hatchling in the current state I am thinking to try to PR a update of the current master with hatchling to better understand the differences and benefits

MementoRC avatar Mar 29 '24 23:03 MementoRC

The plugin would live at the build system layer i.e. Hatchling

I took a brief look and it seems to me that it is similar to the hooks/plugin of hatch. I think what I would prefer is a tools.hatch.hooks.scikit-build-core framework

I was working a little on moving coincurve to using Cython and interestingly I see that Henry has a Cython_cmake project,

MementoRC avatar Mar 29 '24 23:03 MementoRC

For more context, see this issue: https://github.com/PyO3/maturin/issues/1419

Basically, Hatchling will be the backend when there is a plugin and the only thing that other package would do is build the artifacts for Hatchling to consume.

ofek avatar Mar 30 '24 00:03 ofek

Right, I think I understand, though the pyproject.toml should have hatchling as build-backend, then specify the configuration of scikit-build-core with a plug-in, correct?

Hatchling seems more tailored to something like testing a wheel within a pip-free conda environment (right, i don'tlike pip interfering with conda), the plugin would have to pass-through all hatchling config, rather than just the artifacts

Maybe I am still missing it, I'll start looking into a scikit-build-core plugin to try to better understand it

I mean, there's only one 'build-backend' spec in the pyproject.toml, or do you mean that we'll have to also provide a hatch.toml?

MementoRC avatar Mar 30 '24 00:03 MementoRC

In the future scenario Hatchling would be the backend with a plugin configured that would call scikit-build-core. In the final future scenario after that, scikit-build-core would remove its ability to be a backend (i.e. strip out all of the code to build wheels and source distributions) and it would only be used as a plugin for any backend that supported the future plugin architecture.

For now, we have to use only scikit-build-core.

ofek avatar Mar 30 '24 02:03 ofek

What do you think about scikit-build-cli? It may not be too difficult to create a hatch plugin, this way we could move on to using hatchling and have a more finalized packaging, then once scikit-build-core has moved forward we'd use the more integrated interface

MementoRC avatar Mar 30 '24 14:03 MementoRC

Do you mean a Hatchling build hook that invokes something as a subprocess to build the artifacts?

ofek avatar Mar 30 '24 14:03 ofek

Yes, I thought that was the model you were looking for. I dug a bit more on scikit and I think the plugin is intended to be something likebuild-backend = scikit-build-core.hatchling.build_meta, right, but it seems convoluted to me and more complex to engineer

MementoRC avatar Mar 30 '24 15:03 MementoRC

No the future plugin would be a build hook like this https://github.com/ofek/hatch-mypyc so Hatchling would still be the backend, nothing else would be referenced in that section.

Sure if you can make that work that sounds good! You would want a custom build hook:

  • https://hatch.pypa.io/latest/plugins/build-hook/custom/
  • https://hatch.pypa.io/latest/plugins/build-hook/reference/#hatchling.builders.hooks.plugin.interface.BuildHookInterface.initialize
  • https://hatch.pypa.io/latest/plugins/builder/wheel/#build-data
  • https://github.com/ofek/hatch-mypyc/blob/5a198c0ba8660494d02716cfc9d79ce4adfb1442/hatch_mypyc/plugin.py#L340-L343

ofek avatar Mar 30 '24 15:03 ofek

I am too superficial! After more digging, this is exactly what Henry is working on (last commit 4 days ago). He is developing it inside a branch, so I thought he was implementing it the way he wraps setuptools, but looking at his test suite and example, it is what I am looking for:

[tool.hatch.build.targets.wheel.hooks.scikit-build]
dependencies = ["scikit-build-core"]
wheel.install-dir = "hatchling_example"
cmake.source-dir = "cpp"

lOl ... 10s of back-and-forth for me to finally get it :'(

MementoRC avatar Mar 30 '24 15:03 MementoRC

I didn't know it was that far along, nice!

ofek avatar Mar 30 '24 16:03 ofek

Oh, you're not collaborating with him? So, I am copying his progress into a standalone plugin that I will install locally to test out

MementoRC avatar Mar 30 '24 18:03 MementoRC

I am in general but not implementing the plugin directly. For that, I think one of his undergraduate researchers is assisting.

ofek avatar Mar 30 '24 18:03 ofek

Hmmm ... I don't want to be ridiculed by an undergraduate student though ... maybe I should drop him a note before I get entangled

MementoRC avatar Mar 30 '24 18:03 MementoRC

Some progress, though still some hiccups:

-- Install configuration: "Release"
-- Installing: //_libsecp256k1.cpython-311-x86_64-linux-gnu.so
CMake Error at /tmp/tmpyk6rr366/cmake_install.cmake:80 (file):
  file INSTALL cannot copy file
  "/tmp/tmpyk6rr366/cm_python_module/_libsecp256k1.cpython-311-x86_64-linux-gnu.so"
  to "//_libsecp256k1.cpython-311-x86_64-linux-gnu.so": Permission denied.

from:

[tool.hatch.build.targets.wheel.hooks.hatch-scikit-build-core]
dependencies = ['scikit-build-core']
wheel.install-dir = 'build/scikit-build-core'
cmake.source-dir = '.'

# --- scikit-build-core ---
[tool.scikit-build]
# metadata.version.provider = "scikit_build_core.metadata.setuptools_scm"
cmake.verbose = true
# logging.level = 'INFO'
# wheel.packages = ['src/coincurve']
cmake.build-type = 'Release'

I had to extract the plugin from scikit-build-core, I could not figure out how to register a plugin from within another package. But we'll have to await until the 0.9 release since the plugin requires an update in scikit-build-core anyway I have a good working space though with locally installed scikit and plugin, I'll call it a day ... a successful day even

MementoRC avatar Mar 30 '24 23:03 MementoRC

@ofek I invited you to the plugin repo. It is private for now. It is mostly from Henry, but I did not understand why he needed to update scikit-build-core. I implemented a version that seems to work with 0.8.2, but I'd like you to take a look. The build_dir test is failing because I don't quite understand the mypyc that it comes from. But it builds the wheel. I am trying to understand how to control exactly what files are built by scikit and passed to hatchling, but I could use a helping eye and brain ...

MementoRC avatar Apr 02 '24 00:04 MementoRC

@ofek It looks like with the new plugin, scikit-build-core is creating the wheel, the installed coincurve in the tox directory only contains _libsecp256k1.xxx.so. How does hatch retrieve the artifact from scikit-build hook. The hook builds the library correctly:

  -- Installing: /tmp/tmp17u80te9/wheel/platlib/coincurve/_libsecp256k1.cpython-311-x86_64-linux-gnu.so
  Preparing editable metadata (pyproject.toml) ... done

but after that it seems that I am missing something. With the previous implementation, the artifacts were passed in the build_data, I don't see it in this implementation - stuck for now

MementoRC avatar Apr 22 '24 14:04 MementoRC

cc @henryiii

ofek avatar Apr 22 '24 14:04 ofek

With python -m build:


*** Installing project into wheel...
-- Install configuration: "Release"
-- Installing: /tmp/tmpzvu_i59_/wheel/platlib/coincurve/_libsecp256k1.cpython-311-x86_64-linux-gnu.so
Successfully built coincurve-19.0.2.tar.gz and coincurve-19.0.2-cp311-cp311-linux_x86_64.whl

content:

unzip -l dist/coincurve-19.0.2-cp311-cp311-linux_x86_64.whl
Archive:  dist/coincurve-19.0.2-cp311-cp311-linux_x86_64.whl
  Length      Date    Time    Name
---------  ---------- -----   ----
      179  2020-02-02 00:00   coincurve/__about__.py
      297  2020-02-02 00:00   coincurve/__init__.py
     1117  2020-02-02 00:00   coincurve/context.py
     3985  2020-02-02 00:00   coincurve/ecdsa.py
      433  2020-02-02 00:00   coincurve/flags.py
    24235  2020-02-02 00:00   coincurve/keys.py
        0  2020-02-02 00:00   coincurve/py.typed
      310  2020-02-02 00:00   coincurve/types.py
     4183  2020-02-02 00:00   coincurve/utils.py
  1279192  2020-02-02 00:00   coincurve/_libsecp256k1.cpython-311-x86_64-linux-gnu.so
     3977  2020-02-02 00:00   coincurve-19.0.2.dist-info/METADATA
       99  2020-02-02 00:00   coincurve-19.0.2.dist-info/WHEEL
     9748  2020-02-02 00:00   coincurve-19.0.2.dist-info/licenses/LICENSE-APACHE
     1065  2020-02-02 00:00   coincurve-19.0.2.dist-info/licenses/LICENSE-MIT
     1218  2020-02-02 00:00   coincurve-19.0.2.dist-info/RECORD
---------                     -------
  1330038                     15 files

When I install that wheel and test it, it seems to work

Note that I could be missing something quite obvious

MementoRC avatar Apr 22 '24 16:04 MementoRC

I'd recommend pushing the update so we can see the final failure message. Also if the wheel looks fine I don't quite understand what you mean about tox not working correctly. Could it be that editable installations aren't yet supported so we should remove that option in tox?

ofek avatar Apr 22 '24 17:04 ofek

@ofek that was it! Pushing thru

MementoRC avatar Apr 22 '24 19:04 MementoRC

Ready for review?

ofek avatar Apr 30 '24 14:04 ofek