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

build(python): Add CMake build target for python #1257

Merged
merged 15 commits into from
Dec 6, 2023

Conversation

WillAyd
Copy link
Contributor

@WillAyd WillAyd commented Nov 6, 2023

No description provided.

@WillAyd WillAyd requested review from lidavidm and kou as code owners November 6, 2023 02:21
@lidavidm
Copy link
Member

lidavidm commented Nov 6, 2023

What's the goal here? I was intentionally trying to keep cmake out of the Python toolchain

@WillAyd
Copy link
Contributor Author

WillAyd commented Nov 6, 2023

Ah ok thanks for the context. Happy to close then if there's not an appetite for it, I just figured it was a more convenient way to build the python libraries, which guarantees the shared libraries / targets are always up to date

@lidavidm
Copy link
Member

lidavidm commented Nov 6, 2023

I think having this as an alternative to the pip-and-env-var workflow is fine! The conda packages do depend on the pip workflow, though (they don't bundle the libraries with the Python package, instead relying on Conda dependencies to do the work)

@WillAyd
Copy link
Contributor Author

WillAyd commented Nov 6, 2023

Ah OK thanks - that helps better frame some of the things in CI I wasn't sure of. Will take a few more passes at this

@WillAyd
Copy link
Contributor Author

WillAyd commented Nov 10, 2023

Current approach is less invasive - just builds on top of the environment setting in place.

A bit surprised by the typing errors - need to debug that further

@github-actions github-actions bot added this to the ADBC Libraries 0.9.0 milestone Nov 30, 2023
@WillAyd
Copy link
Contributor Author

WillAyd commented Nov 30, 2023

Don't think CI failure is related - seen on other PRs

@lidavidm
Copy link
Member

Filed #1333

I will try to review this but my bandwidth for OSS is limited for the near future, unfortunately

@WillAyd
Copy link
Contributor Author

WillAyd commented Nov 30, 2023

Sounds good. As always there is no rush on these

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Thanks!

@lidavidm lidavidm merged commit cf3b5c0 into apache:main Dec 6, 2023
53 of 54 checks passed
@WillAyd WillAyd deleted the python-cmake branch December 14, 2023 02:36
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