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

RFE: move python bindings to separate project #12696

Closed
kloczek opened this issue May 6, 2023 · 5 comments
Closed

RFE: move python bindings to separate project #12696

kloczek opened this issue May 6, 2023 · 5 comments

Comments

@kloczek
Copy link

kloczek commented May 6, 2023

Just checked sdist tar ball from pypi and looks like its content does not match in few places to what is in git repo.

As I've pointediin comments in #12201 there are spome issue with building protobuf with its python binding from git tree because prptopbuf source tree may be configured using cmake to use off tree tree build (cmake -B <build_directory>).

IMO it woild be better to clone protobuf to for example https://github.com/protocolbuffers/python-protobuf (to preserve python filrs changes history) and remove protobuf files than transform it to standalone build pytjon module with assumption that protobuf library and devele resources are installed in system image.
Because protobuf provides pkgconfig files checking protobuf librarry and its devel resources presence in system image can be done using pkgconfig python module (https://pypi.org/project/pkgconfig/).
IMO building that way protobuf python module will make its buil procedure fully deterministic.

@kloczek kloczek added the untriaged auto added to all issues by default when created. label May 6, 2023
@haberman
Copy link
Member

Just checked sdist tar ball from pypi and looks like its content does not match in few places to what is in git repo.

That is correct, the source dist is built by a Bazel rule here: https://github.com/protocolbuffers/upb/blob/main/python/dist/BUILD.bazel#L204-L227

As I've pointediin comments in #12201 there are spome issue with building protobuf with its python binding from git tree because prptopbuf source tree may be configured using cmake to use off tree tree build (cmake -B <build_directory>).

Why are you wanting to build the Python binding from the git tree instead of using the sdist or binary wheels from PyPI?

If you want to build Python packages from the repo, there are instructions here: https://github.com/protocolbuffers/protobuf/tree/main/python#building-packages-from-this-repo

The Python packages do not require CMake to build.

IMO it woild be better to clone protobuf to for example https://github.com/protocolbuffers/python-protobuf (to preserve python filrs changes history)

We do not want to move Python into a separate repo. Having separate repos increases our maintenance burden, because we would have to create dependencies between the repos and it would be impossible to make atomic changes across repos.

and remove protobuf files than transform it to standalone build pytjon module with assumption that protobuf library and devele resources are installed in system image.

The Python sdist does not require the C++ protobuf library. The Python packages now use upb (C) and not protobuf (C++), more info here: https://github.com/protocolbuffers/protobuf/tree/main/python#implementation-backends

The sdist contains the full upb library. It does not depend on the protobuf library being installed in the system image.

@kloczek
Copy link
Author

kloczek commented May 15, 2023

Just checked sdist tar ball from pypi and looks like its content does not match in few places to what is in git repo.

That is correct, the source dist is built by a Bazel rule here: https://github.com/protocolbuffers/upb/blob/main/python/dist/BUILD.bazel#L204-L227

This procedure is not pep517 compliant.
https://peps.python.org/pep-0517/

As I've pointediin comments in #12201 there are spome issue with building protobuf with its python binding from git tree because prptopbuf source tree may be configured using cmake to use off tree tree build (cmake -B <build_directory>).

Why are you wanting to build the Python binding from the git tree instead of using the sdist or binary wheels from PyPI?

Because I want to have guarantee that if after release and upload to pypi new version on top of the source tree will be possible to integrate any patch added in VCS tree.

If you want to build Python packages from the repo, there are instructions here: https://github.com/protocolbuffers/protobuf/tree/main/python#building-packages-from-this-repo

All modules should be possible to build unsing pep517 build procedure.
That procedure is not pep517 compliant.

The Python packages do not require CMake to build.

IMO it woild be better to clone protobuf to for example https://github.com/protocolbuffers/python-protobuf (to preserve python filrs changes history)

We do not want to move Python into a separate repo. Having separate repos increases our maintenance burden, because we would have to create dependencies between the repos and it would be impossible to make atomic changes across repos.

So how do you want to handle indeterministic build directory name used by cmake?

and remove protobuf files than transform it to standalone build pytjon module with assumption that protobuf library and devele resources are installed in system image.

The Python sdist does not require the C++ protobuf library. The Python packages now use upb (C) and not protobuf (C++), more info here: https://github.com/protocolbuffers/protobuf/tree/main/python#implementation-backends

The sdist contains the full upb library. It does not depend on the protobuf library being installed in the system image.

As long as does not match with VCS content it is kind of useless.

[tkloczko@pers-jacek SPECS]$ ls -1 python-*.spec|wc -l; grep "Source:.*%{VCS}/" python-*spec |wc -l; grep "Patch:.*%{VCS}/" python-*spec |wc -l
1137
1069
603

Abome means that: in my distro I have

  • 1137 spec files used to package python modules
  • amongst those 1137 1069 during build are using tar balls autogenerated from VCS tags
  • on top of those 1069 are added 603 patches taken from VCS which are not integrated in any of sdist tar balls.

That approach is especially usefull and low cost obverhead when it is necessary to fix some issue which alredy has been identified and fixed in VCS and when maintainer refuses to release new module version with that fix.

The same direction of the build procedures changes at the moemtn is more and more used in Fedora.
(We are living in the world in which we are dealing with only not-enough-well-tested software and that requires adjusting build procedures).

@haberman
Copy link
Member

All modules should be possible to build unsing pep517 build procedure.

Can you point to where this "should" language exists re: PEP 517?

So how do you want to handle indeterministic build directory name used by cmake?

I am not sure what you mean. Our Python packages do not use CMake in any way.

Abome means that: in my distro I have

What is your distro? Can you point me to it so I can understand better what you are doing?

If there is a way to make the repo more PEP 517 compliant without splitting it into multiple repos, we may be able to accept patches if they are not too intrusive.

@kloczek
Copy link
Author

kloczek commented May 15, 2023

All modules should be possible to build unsing pep517 build procedure.

Can you point to where this "should" language exists re: PEP 517?

PEP stabds for Python Enhancement Proposals.
That "should" is in that "Proposals".

So how do you want to handle indeterministic build directory name used by cmake?

I am not sure what you mean. Our Python packages do not use CMake in any way.

If you will look onto https://github.com/protocolbuffers/protobuf/blob/main/python/setup.py you can find hardcoded paths to the rest of the this repository source tree with which build module should be linked.

Abome means that: in my distro I have

What is your distro? Can you point me to it so I can understand better what you are doing?

That distro still is not finished and still not publically availaible.

If there is a way to make the repo more PEP 517 compliant without splitting it into multiple repos, we may be able to accept patches if they are not too intrusive.

As I wrore if protobuf module code will be standalone tree with build rpcedure assuming that protobud DSOs nas necessary headsr files are installed in system image it wouild be possible to drop in serup.py hardcoded paths pointing to rest of this repository source tree.

@haberman
Copy link
Member

If you will look onto https://github.com/protocolbuffers/protobuf/blob/main/python/setup.py you can find hardcoded paths to the rest of the this repository source tree with which build module should be linked.

That setup.py file is used by the old C++ implementation, which is number 2 in this list: https://github.com/protocolbuffers/protobuf/tree/main/python#implementation-backends

It is deprecated and no longer distributed by us. It is not present in the sdist nor the binary wheels we distribute on PyPI.

The implementation we now support and distribute has its setup.py here: https://github.com/protocolbuffers/upb/blob/main/python/dist/setup.py

That setup.py has no requirements for anything else to be installed on the system.

@googleberg googleberg removed the untriaged auto added to all issues by default when created. label Feb 17, 2024
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

No branches or pull requests

3 participants