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

cython: fix the usage of cython #103

Merged
merged 1 commit into from
Sep 10, 2024
Merged

Conversation

fruch
Copy link
Collaborator

@fruch fruch commented Sep 10, 2024

in the change to make cqlsh platfrom specific, we introduce an option to disable cython compilation phase.
that option was broken and disable cython completly, which in turn broken the cibuildwheel process, since wheel didn't have any compiled parts any more.

this change switch the disable option to be base on environment variable cause of a lack alternatives

Ref: https://discuss.python.org/t/explicit-optional-cythonization-via-config-setting-post-pep517-and-install-option-deprecation/25379

in the change to make cqlsh platfrom specific, we introduce
an option to disable cython compilation phase.
that option was broken and disable cython completly,
which in turn broken the cibuildwheel process, since wheel didn't
have any compiled parts any more.

this change switch the disable option to be base on environment variable
cause of a lack alternatives

Ref: https://discuss.python.org/t/explicit-optional-cythonization-via-config-setting-post-pep517-and-install-option-deprecation/25379
Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. i'd suggest s/CQLSH_NO_CYTHON/NO_CYTHON/ though.

@fruch
Copy link
Collaborator Author

fruch commented Sep 10, 2024

lgtm. i'd suggest s/CQLSH_NO_CYTHON/NO_CYTHON/ though.

It's bit too generic without some prefix, any it might not be needed.
I need to test this with scylla packaging, maybe now we are patch the .so better in a way we are not destroying old path labels

@fruch
Copy link
Collaborator Author

fruch commented Sep 10, 2024

lgtm. i'd suggest s/CQLSH_NO_CYTHON/NO_CYTHON/ though.

It's bit too generic without some prefix, any it might not be needed. I need to test this with scylla packaging, maybe now we are patch the .so better in a way we are not destroying old path labels

it's still needed, since dbuild doesn't have python-dev installed, we can't compile c extensions in it

@fruch fruch merged commit 58a226c into scylladb:master Sep 10, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants