-
Notifications
You must be signed in to change notification settings - Fork 93
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
Enable building Python packages for PyPI distribution #103
Conversation
I'd like to give enormous thanks to @jcfr for his help with this PR. |
setup.py
Outdated
package_dir={ | ||
'createrepo_c': 'src/python' | ||
}, | ||
cmake_args=['-DPYTHON_DESIRED:STRING=3', '-DBUILD_LIBCREATEREPO_C_SHARED:BOOL=OFF'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should pass sys.executable
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in dralley@ee6df5f
setup.py
Outdated
'Topic :: System :: Systems Administration', | ||
'Programming Language :: Python :: 3', | ||
), | ||
python_requires=">=3.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not necessarily
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in dralley@ee6df5f
Follow-up to this PR can be found here: dralley#2 |
Follow-up merged |
4e0134d
to
da85933
Compare
INSTALL(FILES createrepo_c.bash DESTINATION "/etc/bash_completion.d") | ||
message("Bash completion directory: /etc/bash_completion.d") | ||
endif (BASHCOMP_FOUND) | ||
OPTION(ENABLE_BASHCOMP "Install Bash autocompletions?" ON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're being verbose for the other options, should this be named CREATEREPO_C_INSTALL_BASHCOMP
to be more consistent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Ideally, option specific to the project should be prefixed by the project name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this could be done in a different PR along with some other stylistic changes
src/python/__init__.py
Outdated
|
||
# Support running tests from the source tree | ||
if not os.path.exists(DATA): | ||
from skbuild.constants import CMAKE_INSTALL_DIR as SKBUILD_CMAKE_INSTALL_DIR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most users won't be installing an sdist via PyPI, they'll be using the normal RPM or a bdist - which means they won't have scikit-build
available and will hit an ImportError
. So I don't think this will work, unfortunately.
Putting that aside, something we've done has broken running tests using the normal build process documented in the README. I get
ERROR: Failure: ImportError (cannot import name _createrepo_c)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/nose/loader.py", line 418, in loadTestsFromName
addr.filename, addr.module)
File "/usr/lib/python2.7/site-packages/nose/importer.py", line 47, in importFromPath
return self.importFromDir(dir_path, fqname)
File "/usr/lib/python2.7/site-packages/nose/importer.py", line 94, in importFromDir
mod = load_module(part_fqname, fh, filename, desc)
File "/home/vagrant/devel/createrepo_c/tests/python/tests/test_xml_parser.py", line 6, in <module>
import createrepo_c as cr
File "/home/vagrant/devel/createrepo_c/build/src/python/createrepo_c/__init__.py", line 8, in <module>
from . import _createrepo_c
ImportError: cannot import name _createrepo_c
From running ``` PYTHONPATH=readlink -f ./build/src/python/
nosetests -s tests/python/tests/````
despite that the path does have _createrepo_c.so
on it.
[vagrant@pulp3 createrepo_c]$ ls build/src/python/createrepo_c/
_createrepo_c.so __init__.py __init__.pyc
Looking into what might be causing this currently. Tests work fine with the Python packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't rely on this - most users won't be installing via PyPI
This was only to support the case of local installation where the developer would create an editable install using python install -e .
or python setup.py develop
running tests using the normal build process documented in the README has been broken by a change somewhere
More details in this post: https://groups.google.com/d/msg/scikit-build/XYfZFT2iFJM/lFEaEnm3CQAJ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was only to support the case of local installation where the developer would create an editable install using python install -e . or python setup.py develop
I get that part, and I see where you're coming from :)
What I was getting at is that, in a normal, vanilla cmake build, e.g. simply running cmake ..; make
etc. instead of python setup.py bdist_wheel
, the code block isn't really valid, because Python packaging and scikit really aren't/shouldn't be involved in that scenario, and scikit might not even be installed on the machine, which would lead to an ImportError
(which I experienced in testing that process).
I'm pretty sure it's only a problem if you're using Python bindings from a vanilla cmake build (surprisingly, the RPMs don't ship the bindings, apparently), but I don't want to break something that currently works unless a maintainer is ok with it. Also, it might become a problem if the RPMs did start including the bindings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a solution might be to turn __init__.py
into a template and inject the bin path directly from Cmake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To summarize, there are two issues preventing editable from working.
By editable install, I mean the ability to run pip install -e .
and then import createrepo_c
(1) python source should be moved from src/python/*
to src/python/createrepo_c/*
(2) resolving the location of the compiled module should be fixed by either having a fallback to skbuild.constants
or by hardcoding the expected location
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dralley The python bindings are shipped in Fedora, EPEL, Mageia, and openSUSE. Only SUSE Linux Enterprise currently does not ship them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Conan-Kudo Ah, I hadn't noticed it was built as a separate RPM package (python3-createrepo_c
). Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this handles all the corner cases.
Attempting to test it by building and installing an RPM.
src/python/CMakeLists.txt
Outdated
@@ -28,6 +30,14 @@ set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fno-strict-aliasing") | |||
set (CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} -fno-strict-aliasing") | |||
set (CMAKE_C_FLAGS_RELEASE "${CMAKE_C_FLAGS_RELEASE} -fno-strict-aliasing") | |||
|
|||
IF (NOT SKBUILD) | |||
IF (${CMAKE_VERSION} VERSION_LESS "2.8.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be sensible to bump the minimum required version fo CMake ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dralley We can probably bump up to CMake 2.8.12. |
* Added a setup.py and pyproject.toml * Made CMake config file changes to facilitate building in a different context * Updated gitignore
This commit introduces the option BUILD_LIBCREATEREPO_C_SHARED that is ON by default.
…f relevant This is required to support importing the python module where the python interpreter is statically linked against the python library.
…rce directory This commit addresses the following error: ``` $ python setup.py egg_info running egg_info creating createrepo_c.egg-info writing dependency_links to createrepo_c.egg-info/dependency_links.txt writing top-level names to createrepo_c.egg-info/top_level.txt writing createrepo_c.egg-info/PKG-INFO writing manifest file 'createrepo_c.egg-info/SOURCES.txt' error: package directory 'createrepo_c' does not exist ``` It also updates the CMake install rules to ensure the __init__.py file and the python modules are installed in the appropriate package.
This commit generalizes the approach originally introduced in e880bf0 to prevent the development files from being installed when building the python package.
This will ensure that executable will be available after installed the wheel. The approach adapted from scikit-build/cmake-python-distributions and scikit-build/ninja-python-distribution projects. For more details, see https://setuptools.readthedocs.io/en/latest/setuptools.html#automatic-script-creation
This is useful when developing locally.
Thank you again @jcfr , you've really gone above and beyond :) I've added one more commit to automatically parse the version string from VERSION.cmake, so that it will not have to be changed in multiple places with every new release. And I will likely add some instructions to the README later today. Other than that, @Conan-Kudo, @ignatenkobrain I think it's ready for review. |
setup.py
Outdated
# split on lines, remove empty lines | ||
lines = filter(len, version_text.split('\n')) | ||
# parse out digit characters from the line, convert to int | ||
numbers = [int("".join(filter(str.isdigit, line))) for line in lines] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could potentially be done "better" with regex, but I'm pretty weak with regex, and I think it's valid to assume there won't be numeric characters on each line that aren't part of the actual version number.
But I'm open to suggestions on how to do this better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it would be a pure python package, I would suggest to use python-versioneer
But in the case of createrepo_c
, the project can be built either as a standalone project or python package, the version management needs to be available to both.
For now, I suggest to stick with your approach. Later after we standardize approaches like the one done here, this could be revisited.
Glad to see that scikit-build is useful 😄 When the createrepo_c package will be ready, I was thinking we could write a blog featuring createrepo_c and scikit-build, would you be up for that ? |
@jcfr I'm not a createrepo_c maintainer or anything, just a user who needed a feature, so I'll let the others weigh in on that one. I'm not sure I'm familiar enough with the project that I could do it justice and I wouldn't want to speak out of place or anything like that. I wouldn't mind attempting to do so but writing is not really my forte. |
Sounds good.
That is great, i think you should still take part in telling the story. I think it illustrates how community, open-source and transparency lead to great outcome. |
|
||
To create a binary "wheel" distribution, use: | ||
|
||
python setup.py bdist_wheel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should suggest the pip way instead:
pip wheel . -w dist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any specific benefits? I've not seen this way before, but according to stack overflow and github issues, it can be fairly slow because it copies everything into a temp directory before doing the build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving forward, the idea is to build python package using a standard approach that is independent from the build backend. The fact setuptools is used in an implementation details and we should try to move away from it, ideally i think the user experience should be the same for building wheels.
The other advantage is that it keeps the source tree clean of any artifact.
Timing wise, here is quick comparison for this project
$ mkvirtualenv -p python3.5 createrepo_c-pip-wheel
$ time pip wheel . -w dist --no-cache-dir
real 0m5.113s
user 0m13.332s
sys 0m1.104s
$ mkvirtualenv -p python3.5 createrepo_c-setup-bdist-wheel
$ time bash -c "pip install -r requirements-dev.txt && python setup.py bdist_wheel"
real 0m3.947s
user 0m11.996s
sys 0m1.180s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair points. The only objection I have is that w/ Pip v9 (which is what is currently installed on most distributions, including the newest Fedora and Ubuntu), you would also have to install the wheel
package because that functionality isn't included by default. It looks like Pip v10+ does include it however.
I would probably err on the side of consistency, so if there's an analogous pip command for building sdists, we can use those. But otherwise I would probably stick with the "standard" way for now and update it later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so if there's an analogous pip command for building sdists
No
I would probably stick with the "standard" way for now and update it later on.
Agreed.
README.md
Outdated
|
||
python setup.py sdist | ||
|
||
Source distributions require the installer of the package to have all of the build dependencies installed on their system, since they compile the code during installation. Binary distributions are pre-compiled, but they are likely not portable between substantially different systems, e.g. Fedora and Ubuntu. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Source distributions require
-> Installing source distributions require
README.md
Outdated
|
||
Source distributions require the installer of the package to have all of the build dependencies installed on their system, since they compile the code during installation. Binary distributions are pre-compiled, but they are likely not portable between substantially different systems, e.g. Fedora and Ubuntu. | ||
|
||
If you install a source distribution, you will need |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look like this sentence is unfinished
05f424e
to
22fa7c6
Compare
edit: this is a nonissue, see below. @jcfr For some reason libdrpm isn't being properly compiled statically, so building on a system where
It starts working again once I install |
Since the Python bindings don't even expose support for deltarpm yet, I'm just going to disable it entirely - problem solved. See new commit. |
There are a couple of overall issues I have with this:
|
Re: 1 Just to be clear, CMake is doing all of the build work. (Not sure if you were thinking otherwise). The only thing that scikit-build does is to wrap the existing setuptools / Likewise, when you build edit: I address what I think you mean in another comment below. With regards to PEP 517, PEP 517 is only "provisionally accepted" and it hasn't been implemented in Pip yet. Until it is (and becomes widespread enough that its presence on a user system can be assumed), everybody still has to use Re: 2 Most Python packages that include compiled code (e.g. numpy, scipy, tensorflow) are targeted towards an environment called Manylinux. It's basically a CentOS 5 image provided by the Python Packaging Authority (PyPA) upon which they've built tooling which analyzes all the binaries for dynamically linked dependencies and uses PatchELF to inject them into the binaries themselves, effectively turning everything statically-linked. This allows you to build standalone binary Python packages that will work on any linux distro without modifying the actual build scripts to statically-link everything. Unfortunately, since it's CentOS 5, the currently existing tooling can't be used to build So for now the best we can do for now is to distribute a source package (sdist). When pip installs a sdist, it basically just unpacks the source, builds a bdist locally and installs that. This does require the user to have all of the correct build dependencies and libraries installed on their machine (cmake, librpm, libpython, etc.), but it would avoid all of the effort needed to build packages for every target environment. At some point, once the new Manylinux environment is ready, it would make sense to set up a CI pipeline to handle the process of building the binary packages for all of the supported enviornments. But that isn't an immediate concern (or possibility). |
Re: 1, Are you referring to how inside Looking at
To explain the rest of those instances
I hope this explanation makes it sound more reasonable. It basically comes down to the fact that some things do need to be a little bit context sensitive for this specific use case, and the only way to access the context is to have something like scikit-build expose that information to CMake. I don't think PEP 517 avoids this situation either, just makes support for doing this sort of thing standardized and easier to implement. |
setup.py
Outdated
version=version, | ||
license='GPLv2', | ||
author='RPM Software Management', | ||
author_email='', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Email address is rpm-ecosystem@lists.rpm.org
setup.py
Outdated
name='createrepo_c', | ||
description='C implementation of createrepo', | ||
version=version, | ||
license='GPLv2', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is GPLv2+
src/CMakeLists.txt
Outdated
@@ -110,16 +115,34 @@ CONFIGURE_FILE("deltarpms.h.in" "${CMAKE_CURRENT_SOURCE_DIR}/deltarpms.h" @ONLY) | |||
IF (CMAKE_SIZEOF_VOID_P MATCHES "8") | |||
SET (LIB_SUFFIX "64") | |||
ENDIF (CMAKE_SIZEOF_VOID_P MATCHES "8") | |||
|
|||
SET (LIB_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/lib${LIB_SUFFIX}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just go ahead and change this to use GNUInstallDirs
? We're already saying we require at least CMake 2.8, right?
@Conan-Kudo updated |
1a5935c
to
c510f2e
Compare
I've tested that createrepo_c builds fine with this patch. Please note that DNF team has no intention to manage createrepo_c in pypi. |
This will allow Python bindings and executables for createrepo_c to be distributed cross-distribution via PyPI.
To create a source distribution (requires the installer to have build dependencies installed, but not the builder), use
python3 setup.py sdist
.To create a binary distribution (pre-compiled, does not require the installer to have build deps, caveat [0]), use
python3 setup.py bdist_wheel
. Note: you do need to havescikit-build
package installed to build the package.To install the package, use
pip install dist/createrepo_c-*.tar.gz
if it's an sdist package orpip install dist/createrepo_c-*.whl
if it's a bdist. If it is an sdist package, the installer also needs to have thescikit-build
package installed. New versions ofpip
>= 10.0 should handle this for you due to thepyproject.toml
file, butpip
< 10.0 will not.Once uploaded to PyPI the package can be installed with
pip install createrepo-c
Cmake build system changes
ENABLE_BASHCOMP
, default ON, so that installing bash completions can be disabled if necessary.CREATEREPO_C_INSTALL_DEVELOPMENT
, default ON, so that installing header files and pkginfo can be disabled if necessary.CREATEREPO_C_INSTALL_MANPAGES
, default ON, so that installing manual pages can be disabled if necessary.BUILD_LIBCREATEREPO_C_SHARED
, default ON, so that libcreaterepo_c can be built statically if needed, e.g. built directly into the python extension library.OPENSSL_INCLUDE_DIR
variable to enable building against local/custom OpenSSL libraries.Since these are general changes, if you would like me to split these changes out into separate Pull Requests, I can do so. However, they are prerequisites for this PR.
Python specific changes
SKBUILD
is set.SKBUILD
is set by in the context of building a Python package by "scikit-build", which acts as a bridge between CMake and the Python packaging infrastructure.[0] Binary distributions are not generally portable across different systems - a bdist created on Fedora 28 probably will not work on Ubuntu 16.04. However, there is a specific environment provided by Python Packaging Authority called manylinux, from which you can produce widely portable bdists. The current "manylinux1" environment is based on CentOS 5, which uses a GLib version too old to build createrepo_c. A new environment is in the works based on CentOS 6, but it is not officially ready yet. Thus, for the time being (but hopefully not for very long), it won't be possible to create a widely portable bdist package of createrepo_c.