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

GH-45576: [Python] Support building C++ library automatically #45580

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mgorny
Copy link
Contributor

@mgorny mgorny commented Feb 19, 2025

What changes are included in this PR?

Add a PYARROW_BUILD_ARROW_CPP environment variable and a corresponding --build-arrow-cpp option that enables automatically building the Arrow C++ libraries as part of setup.py invocation and using them to build PyArrow afterwards. This is a prototype / proof-of-concept.

Are these changes tested?

This change introduces an additional build path. I've tested it locally.

Are there any user-facing changes?

It introduces a new option, primarily aimed at downstream packagers — though I suppose some users building from source may also want to use it. If the approach is sound, updating https://arrow.apache.org/docs/developers/python.html#build-and-test may make sense.

Add a `PYARROW_BUILD_ARROW_CPP` environment variable and a corresponding
`--build-arrow-cpp` option that enables automatically building the Arrow
C++ libraries as part of `setup.py` invocation and using them to build
PyArrow afterwards.  This is a prototype / proof-of-concept.
Copy link

⚠️ GitHub issue #45576 has been automatically assigned in GitHub to PR creator.

@tiran
Copy link

tiran commented Feb 20, 2025

I have tested @mgorny 's PR in our downstream build pipeline. It was as intended and makes our downstream builds much easier to maintain.

@pitrou
Copy link
Member

pitrou commented Feb 24, 2025

I'm frankly not sure we want to support anything like this. Our setup.py is already tedious to maintain and we probably don't want to add any complexity there. @raulcd @jorisvandenbossche Thoughts?

@raulcd
Copy link
Member

raulcd commented Feb 25, 2025

I think the current plan, or at least the latest conversations, were to move our build-backend to scikit-build-core. Adding more custom options to our setup.py seems that will make that migration even more complex.
I understand the appeal but this will still fail for people building from source from pypi for example and only helps downstream package maintainers that want to build pyarrow from source cloning the repo, right? which at that point requires a couple commands to build Arrow C++:

cmake -S arrow/cpp -B arrow/cpp/build \
      -DCMAKE_INSTALL_PREFIX=$ARROW_HOME \
      --preset ninja-release-python
cmake --build arrow/cpp/build --target install

@tiran when you say your downstream builds are much easier to maintain, could you share a link on how are you currently building Arrow C++? Is that you would like to get rid of the manual cmake commands?

@pitrou
Copy link
Member

pitrou commented Feb 25, 2025

I think the current plan, or at least the latest conversations, were to move our build-backend to scikit-build-core.

Yes, IMHO that's very much where we want to go.

@mgorny
Copy link
Contributor Author

mgorny commented Feb 25, 2025

I understand the appeal but this will still fail for people building from source from pypi for example

Well, that's definitely something that could be fixed as a followup, or at least to some degree.

were to move our build-backend to scikit-build-core

Well, I'm all for that. I suppose a similar solution (i.e. ability to build Arrow C++ libraries from the Python build) could also be implemented at CMake level, and actually that's how I initially wanted to do it — but I've given up since it required a lot of added complexity. I've noted the problems in #45576. I'm not saying it's impossible, just would probably involve more changes to CMake files and more work, and I'm not sure if that is desired.

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.

4 participants