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

git actions: fail build if extensions aren't built #412

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

fruch
Copy link

@fruch fruch commented Jan 23, 2025

we run running again and again into situation our wheels are missing some of the part we expect cause of extensions silently failing

with this change we can stop the builds cause of that, and attend to it

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@fruch fruch requested review from Lorak-mmk and dkropachev January 23, 2025 08:25
@fruch fruch mentioned this pull request Jan 23, 2025
8 tasks
PyEval_InitThreads is depricated, since 3.7 it is not necessary to call
it.
Ref: https://docs.python.org/3/c-api/init.html#c.PyEval_InitThreads
Otherwise it fails to convert unicode to 'str':
```
Error compiling Cython file:
------------------------------------------------------------
...
    if metadata_request_timeout is None:
        return stmt
    ms = int(metadata_request_timeout / datetime.timedelta(milliseconds=1))
    if ms == 0:
        return stmt
    return f"{stmt} USING TIMEOUT {ms}ms"
           ^
------------------------------------------------------------

cassandra/util.py:1813:11: Cannot convert Unicode string to 'str' implicitly. This is not portable and requires explicit encoding.
```
@fruch fruch force-pushed the fail_if_not_ext_built branch 6 times, most recently from 1967715 to 0b856da Compare January 23, 2025 22:36
@fruch
Copy link
Author

fruch commented Jan 24, 2025

@dkropachev

See my commits to switch using pyproject.toml seems they are helping with the building situation quite a lot

It's still a work in progress (macos is not targeting correct in some)

Other issues surfaced cause failing extensions:

  • windows doesn't have uint_128 as we declare and use in Linux/macos

  • aarch64 - gcc crashing in one extension complication

We'll need to take a call on those
I don't mind skipping windows (the must compile flag), but aarch64 we must fix

@dkropachev
Copy link
Collaborator

@dkropachev

See my commits to switch using pyproject.toml seems they are helping with the building situation quite a lot

It's still a work in progress (macos is not targeting correct in some)

Other issues surfaced cause failing extensions:

  • windows doesn't have uint_128 as we declare and use in Linux/macos
  • aarch64 - gcc crashing in one extension complication

We'll need to take a call on those I don't mind skipping windows (the must compile flag), but aarch64 we must fix

Thanks @fruch , it fixed ubuntu, working on aarch, it looks like all major building binaries are affected: patchelf, gcc, c++.
Bumping to ubuntu-24.04 did not help, trying to update apt packages.

@fruch
Copy link
Author

fruch commented Jan 25, 2025

@dkropachev

See my commits to switch using pyproject.toml seems they are helping with the building situation quite a lot

It's still a work in progress (macos is not targeting correct in some)

Other issues surfaced cause failing extensions:

  • windows doesn't have uint_128 as we declare and use in Linux/macos
  • aarch64 - gcc crashing in one extension complication

We'll need to take a call on those I don't mind skipping windows (the must compile flag), but aarch64 we must fix

Thanks @fruch , it fixed ubuntu, working on aarch, it looks like all major building binaries are affected: patchelf, gcc, c++.
Bumping to ubuntu-24.04 did not help, trying to update apt packages.

You are missing the point, compilation is done inside docker, we can try updating packing inside the before build command, or find out what the root cause, it's only one extension that crashes

With the new setup cibuildwheel and docker, you can run the aarch64 locally (it would be slow be easier to debug)

@dkropachev
Copy link
Collaborator

@dkropachev
See my commits to switch using pyproject.toml seems they are helping with the building situation quite a lot
It's still a work in progress (macos is not targeting correct in some)
Other issues surfaced cause failing extensions:

  • windows doesn't have uint_128 as we declare and use in Linux/macos
  • aarch64 - gcc crashing in one extension complication

We'll need to take a call on those I don't mind skipping windows (the must compile flag), but aarch64 we must fix

Thanks @fruch , it fixed ubuntu, working on aarch, it looks like all major building binaries are affected: patchelf, gcc, c++.
Bumping to ubuntu-24.04 did not help, trying to update apt packages.

You are missing the point, compilation is done inside docker, we can try updating packing inside the before build command, or find out what the root cause, it's only one extension that crashes

With the new setup cibuildwheel and docker, you can run the aarch64 locally (it would be slow be easier to debug)

It looks like I found cure for aarch https://github.com/scylladb/python-driver/actions/runs/12970378006/job/36175553064?pr=411, ran pass cp310-manylinux_aarch64 wheel

@dkropachev
Copy link
Collaborator

fixes macos as well, making a glimpse at windows.

fruch added 3 commits January 26, 2025 15:09
we run running again and again into situation our wheels are missing
some of the part we expect cause of extensions silently failing

with this change we can stop the builds cause of that, and attend to it
build is the way packages should be built it does build isolation by
default

we also switch to uv, for quicker way of collecting packages

Ref: https://build.pypa.io/en/stable/index.html
Ref: https://peps.python.org/pep-0517/
Ref: https://peps.python.org/pep-0518/
move to pyproject.toml that supports PEP517/518
this would help us use isolated build env, and avoid
all the fragile situation we have with Cython installation dependcy
missing in some CI variation
@fruch fruch force-pushed the fail_if_not_ext_built branch from 0b856da to 47d4409 Compare January 26, 2025 13:10
fruch added 3 commits January 26, 2025 15:30
move the configuration to pyproject.toml, that it would
be easier to use from multiple workflow and for local usage

for example:
```
uv pip install cibuildwheel
CIBW_FREE_THREADED_SUPPORT=True  cibuildwheel --only cp313-manylinux_x86_64
```
for some reason we missing that the path we carfully added
are not actually used
without this, that `c_shard_info.c` fails to compile like the following:

```
  cassandra\c_shard_info.c(3083): error C2065: '__uint128_t': undeclared identifier
  cassandra\c_shard_info.c(3083): error C2146: syntax error: missing ')' before identifier '__pyx_v_biased_token'
  cassandra\c_shard_info.c(3083): error C2059: syntax error: ')'
  cassandra\c_shard_info.c(3083): error C2059: syntax error: ')'
  cassandra\c_shard_info.c(3083): error C2059: syntax error: ')'
```
@fruch fruch force-pushed the fail_if_not_ext_built branch from 47d4409 to b2c6171 Compare January 26, 2025 13:31
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.

2 participants