PROPOSAL: add preload and library type

[MementoRC]

Can we add recording of the version and the type of library coincurve was built upon (external or internal)? In the case of external, this information is needed to try to pre-load the DLL on windows. This is thw main reason why the tests are not working in the othe PR

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.68%. Comparing base (5e46aca) to head (1722f9d).

❗ Current head 1722f9d differs from pull request most recent head f590d8f. Consider uploading reports for the commit f590d8f to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #151   +/-   ##
=======================================
  Coverage   85.68%   85.68%           
=======================================
  Files          11       11           
  Lines         566      566           
  Branches       67       67           
=======================================
  Hits          485      485           
  Misses         45       45           
  Partials       36       36           

[MementoRC]

@ofek Do you accept this proposal, or should I try to find another way. This is what I need to force the loading of libsecp256k1-2.dll for the windows wheel build with the conda install. So far what I observed is that:

• No particular issues with building, we need conda to install libsecp256k1, no issues
• For the test of the wheel, cibuildwheel uses virtualenv to install the wheel, but we need conda and things go south from there, the lib is installed, but virtualenv does not see it, only a manual load make the test work

I think it is valuable to have it in coincurve. The type of build and the libname (-1, -2, ..) is saved in this file and packaged with the library. When a user initialize the package, it does this simple check as to how the _libsecp256k1 was built and requires the dynamic lib if needed

[ofek]

Can you show me another example of an extension module that has runtime logic specifically for Conda?

[MementoRC]

No, I don't think I can. This is not specific to conda though, I just know that I will need it for conda, which is the only known case for using libsecp256k1 shared library so far

I kind of feel that you're not ok with this at all, so the key reason for this is that I needed to verify the wheel built with a shared libsecp256k1 ... and it works ... so what I mean is maybe I should step back:We won't actually release these builds

[ofek]

I'm not inherently against it, just trying to understand because I have limited time to review

[MementoRC]

I thought about it over lunch and it seems that it is only needed in a niche case where (as far as I understand it) an installed library on windows is "lost". This seems to happen only in the additional condition that several unix-like environments are stacked

So, although I am pretty happy with having found and resolved this flaw, it may just seeds an unforeseen issue later that may reduce the popularity of coincurve ... and I wouldn't want to again be the source of an issue

For more context, this is the workflow where this occurs:

  windows-wheels-conda-x86_64:
    name: Build Windows (CONDA) wheels AMD64
    needs:
    - test
    runs-on: windows-latest

    steps:
    - uses: actions/checkout@v4

    - name: Set up Python ${{ env.PYTHON_VERSION }}
      uses: actions/setup-python@v5
      with:
        python-version: '3.12'

    - name: Install Miniconda
      uses: conda-incubator/setup-miniconda@v3

    - name: Build wheels
      uses: pypa/[email protected]
      env:
        CIBW_ENVIRONMENT:
            COINCURVE_IGNORE_SYSTEM_LIB="0"
            COINCURVE_UPSTREAM_REF="__no_tag__"
            COINCURVE_SECP256K1_BUILD='SHARED'
        CIBW_BUILD: "cp312-win_amd64"
        CIBW_ARCHS_WINDOWS: "AMD64"
        CIBW_BEFORE_ALL: >
            conda install -c conda-forge pkg-config libsecp256k1
        CIBW_BUILD_VERBOSITY: 1
        # pytest needs to be defined here to find 'coincurve' package?
        CIBW_TEST_REQUIRES: dlltracer pytest pytest-benchmark
        CIBW_TEST_COMMAND: >
            conda activate test &&
            conda install -c conda-forge libsecp256k1 pkg-config &&
            python {project}/tests/test_dll_tracer.py &&
            python -c
            "from coincurve import PrivateKey;
            a=PrivateKey();
            b=PrivateKey();
            assert a.ecdh(b.public_key.format())==b.ecdh(a.public_key.format())
            "

The failure that this solves occurs in the CIBW_TEST_COMMAND. When from coincurve ... is interpreted it fails with a DLL load for _libsecp256k1, which is due to the fact that it requires libsecp256k1-2.dll and does not find it. But, the test_dll_tracer.py which load the library works (with the full path) fine and loads libsecp256k1-2.dll

I did try to set the %PATH% but it did not seem to work

[ofek]

So do you mean that we should close this for now since it's mostly for debugging an edge case that you have fixed?

[MementoRC]

Well, if you feel that it is risky, or even feel uneasy about it. I'd rather go back to the drawing board and find another solution or simply test in my fork then PR a clean code without this test

I also don't mean to use too much of your time, but I also need to know what is a good idea to pursuit and what is not

Next, I'll focus on just replacing the make with cmake - I need that to get to the native windows wheel, which I know you want

[ofek]

Sorry I was just a bit confused: is this necessary to get something working or is it more for debugging? If the former then I will merge.

[MementoRC]

It is necessary to make the cibuildwheel + coincurve built with shared libsecp256k1 library be able to test that wheel on windows. It is a narrow and niche case need

That need "could" occur IF a user happens to install coincurve in an unconventional way, with libsecp256k1 from conda and source-build package with pip ... maybe.

Also, understand that it is not easy for me to simply say no to something that looks cool and that I found

Your repo only releases coincurve built with a statically linked libsecp256k1. This case will never happen - so from your repo standpoint, no, it is not needed

[MementoRC]

@ofek Are you open to reorganizing coincurve as a src-layout package ... I get a lot of missing coincurve package when a tox -e ... fails

... and separating setup.py into a subpackage like setup_modules

All these PR's are actually kinda ready, btw

[MementoRC]

Well, of course - I am switching to dbg mode on my fork - back when it passes

[ofek]

Let me know when to review what

[MementoRC]

Sure, I still need to work on rebasing this one. Note that I changed the logic, now I only add the extra file to the wheel if it is an EXTERNAL libsecp256k1, then I try/except on its existence to quick-turn if there's no file

[MementoRC]

@ofek Do you understand the codecov error? I added a new test for the init.py logic - Ok, I think I understand, the test cannot excercies all its lines because of the if on the type of build to test, for the 'INTERNAL' build there is not much to actually test, but for the EXTERNAL I want the test to verify that the extra lib info file is correct

[MementoRC]

Ok, so I gave some thoughts to this PR. It is solving a niche case and working around the shortcomings of cibuildwheel. In the meantime, when working on testing the coicurve conda environment, I think that I solved the problem in a different way.

So the only benefit of this PR is for the case where coincurve is built to use a system/conda library, but when it is used that library cannot be found - This is a case that is hard to imagine ever happening in the field, and even so, it would only change one error message for another

In brief, I now think it is at best useless.