Allow to choose the build command

[dvarrazzo]

Hello,

In #765 I am trying to fix the PATH in the build env to build a packages. The story is actually a bit more complicated than that: in order to build a working psycopg package (outside cibuildwheel), what seems needed (as can be seen in this test run) is:

1. run python setup.py bdist_wheel (or equivalent) with pg_config in the path:

      - name: Build the C wheel
        run: |
          $env:PATH = "C:\Program Files\PostgreSQL\13\bin;" + $env:PATH
          $env:PATH = "C:\Program Files\PostgreSQL\13\lib;" + $env:PATH
          python ./psycopg_c/setup.py bdist_wheel

2. run delvewheel without pg_config in the path. That's why this is a separate step:

      - name: Delocate the C wheel
        run: |
          pip install delvewheel
          &"delvewheel" repair @(Get-ChildItem psycopg_c\dist\*.whl)

3. the package can be then installed and tested

      - name: Install and run tests (C implementation)
        run: |
          pip install ./psycopg/[test]
          &"pip" install @(Get-ChildItem wheelhouse\*.whl)
          pytest --color yes

If 1 and 2 run in the same environment, delvewheel will pull in something wrong, resulting in the error that can be seen in this run:

ValueError: Can't add new section due to trailing data (PE ends at 0xb9e00, file length is 0xca6f7). Usually
this happens for installers, self-extracting archives, etc. Sorry, I can't help you.

During handling of the above exception, another exception occurred:
...
RuntimeError: Unable to rename the dependencies of libintl-8.dll because this DLL has trailing data. If this
DLL was created with MinGW, run the strip utility. Otherwise, include {'libiconv-2.dll'} in the --no-mangle
flag. In addition, if you believe that delvewheel should avoid name-mangling a specific DLL by default, open
an issue at https://github.com/adang1345/delvewheel/issues and include this error message.

So, it looks like postgres includes, in its libs, a library built with MinGW which eclipses an equivalent library available in windows which delvewheel finds easy to digest. In a test I made on a local machine it does seem that --no-mangle works ok, so this is probably not a show-stopper.

However:

I can see that there is an env var CIBW_BUILD_FRONTEND, which can be used to customise the build step, however it only allows a few precanned choices: build and pip. Wouldn't it be useful to allow also to BYOS (bring your own script) and allow the user to choose whatever command solves their problem? In my case I would have set:

CIBW_BUILD_COMMAND_WINDOWS=tools\build\build_wheel.ps1
CIBW_REPAIR_WHEEL_COMMAND_WINDOWS=delvewheel ...

where build_wheel.ps1 would contain just:

          $env:PATH = "C:\Program Files\PostgreSQL\13\bin;" + $env:PATH
          $env:PATH = "C:\Program Files\PostgreSQL\13\lib;" + $env:PATH
          python ./psycopg_c/setup.py bdist_wheel # or whatever works well

The cibuildwheel pipeline can be already configured using CIBW_REPAIR_WHEEL_COMMAND and CIBW_TEST_COMMAND: it feels natural to have CIBW_BUILD_WHEEL_COMMAND too.

No? :)

Thank you!

[joerick]

The idea has been mooted a few times! But generally we find other ways around the problem that don't require modifying the actual build. The actual build invocation seems pretty core to cibuildwheel, everything sorta leads up to that, so in the past we've been slightly resistant to adding a custom option.

In this case it seems to me that the flexibility built into CIBW_REPAIR_WHEEL_COMMAND could probably solve your problem? Could you add a wrapper script, like:

import sys, subprocess, os

# remove the postgres libs from path
path_elements = os.environ['PATH'].split(os.pathsep)
path_elements = [p for p in path_elements if '\\PostgreSQL\\' not in p]
os.environ['PATH'] = os.pathsep.join(path_elements)

subprocess.run(['delvewheel', *sys.argv[1:]], check=True)

Then set CIBW_REPAIR_WHEEL_COMMAND to be python delvewheel_wrapper.py repair -w {dest_dir} {wheel}.

It is a little hacky, but to me this feels more like an oddity in the repair process than the build, so better addressed at that stage? By the way, do you know why PATH is so crucial to delvewheel? Is that a lookup path for dynamic libraries on windows?

[dvarrazzo]

Hi @joerick,

I have solved the problem our side, adding the possibility of specifying which pg_config executable to use via a PG_CONFIG env var. I thought about hacking on the repair command backwards too but it feels much more brittle (there might be the need of a similar hack on the test command too, I don't know about the state of the PATH before my custom dir is added, etc.)

Of course I could work around the problem this way only because I am in control of the upstream project to build. You may want to use my use case as a data point in the decision of whether to add build command configuration or not (I understand how that command might actually be not so easy to expose or to give it a stable interface)

About why delvewheel is so sensitive to the PATH, I am not sure. I think that windows uses the PATH to find dynamic libraries and the github runner has dozens of directories on the path, including one containing another half-baked Postgres installation. I have just noticed something unexpected with the packages I just built: they have been packaged with the wrong libpq library (version 1300xx was expected). I will make a couple of test and let the delvewheel project know about anything interesting I may find.

[dvarrazzo]

Aside: for a preview of the delvewheel horror story, on the CI runner there are:

• a libpq.dll in a php directory, which is probably the one picked up by delvewheel
• a libiconv-2.dll in the git directory, which is also picked by delvewheel, but that's might be the one easy to digest.
• libpq and libiconv dlls in the postgres directories, which are not picked if the dir is not the path

[CAM-Gerlach]

N.B., FWIW python setup.py bdist_wheel has long been discouraged, has been deprecated for some time, as of setuptools 58.3.0 formal DeprecationWarnings are issued, and will eventually be removed completely. The canonical replacement is python -m build --wheel (for this particular case, or just python -m build to build both wheels and sdists), or for an alternative compatible with both modern PEP 517 and legacy packages, pip wheel. These, incidentally, are the two options civuildwheel currently offers, with the latter currently the default and the former will eventually be. See here for more info.

[henryiii]

This is my recommendation:

https://scikit-hep.org/developer/gha_wheels

(And the previous two pages). I'd be happy to have the docs in cibuildwheel look a bit more like a general version of that. :)

[CAM-Gerlach]

Good point; based on that, for a custom command pipx run build --wheel is even simpler as a one-liner (assuming pipx itself is installed) and ensures an isolated, up to date environment is used to build the wheel.

[joerick]

I don't see a strong need for this, and since PEP517 we're increasingly in a world where this shouldn't be necessary. So, for now at least, I'm gonna close this as a non-goal.