-
Notifications
You must be signed in to change notification settings - Fork 95
Enable building Python packages for PyPI distribution #103
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
Changes from 24 commits
527ca7a
4311a8e
1227830
6f77806
a4eee6b
e47a89a
9280ebe
fe5a306
717b965
399975f
cdf910c
56eb1ca
3b67fc3
58a4505
e804553
8c90040
9b5b5f2
bf5b805
7f1fb86
85a8615
5d6c5e9
c018c70
5a87e04
0cb6fc4
fe04253
2f71436
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,3 +8,4 @@ Jarek Polok <[email protected]> | |
Neal Gompa <[email protected]> | ||
Ralph Bean <[email protected]> | ||
Frank Schreiner <[email protected]> | ||
Daniel Alley <[email protected]> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -102,6 +102,31 @@ cause degradation of performance. | |
|
||
Without git revision specified HEAD is used. | ||
|
||
## Build Python package | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. I think we should suggest the pip way instead:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 commentThe reason will be displayed to describe this comment to others. Learn more.
No
Agreed. |
||
|
||
To create a source distribution, use: | ||
|
||
python setup.py sdist | ||
|
||
Installing 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. | ||
|
||
Note: if you are building a bdist or installing the sdist on a system with an older version of Pip, you may need to install the ```scikit-build``` Python package first. | ||
|
||
To install either of these packages, use: | ||
|
||
pip install dist/{{ package name }} | ||
|
||
To create an "editable" install of createrepo_c, use: | ||
|
||
python setup.py develop | ||
|
||
Note: To recompile the libraries and binaries, you muse re-run this command. | ||
|
||
|
||
## Build RPM package | ||
|
||
Modify createrepo_c.spec and run: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
[build-system] | ||
requires = ["setuptools", "wheel", "scikit-build"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
setuptools | ||
wheel | ||
scikit-build |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
from skbuild import setup | ||
|
||
|
||
with open('VERSION.cmake', 'r+') as version_file: | ||
lines = version_file.read().splitlines() | ||
# 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 commentThe 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 commentThe 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 For now, I suggest to stick with your approach. Later after we standardize approaches like the one done here, this could be revisited. |
||
# build version string | ||
version = '{major}.{minor}.{patch}'.format( | ||
major=numbers[0], | ||
minor=numbers[1], | ||
patch=numbers[2] | ||
) | ||
|
||
setup( | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. It is |
||
author='RPM Software Management', | ||
author_email='', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Email address is |
||
url='https://github.com/rpm-software-management', | ||
classifiers=( | ||
'License :: OSI Approved :: GNU General Public License v2 (GPLv2)', | ||
'Operating System :: POSIX :: Linux', | ||
'Programming Language :: C', | ||
'Topic :: System :: Software Distribution', | ||
'Topic :: System :: Systems Administration', | ||
"Programming Language :: Python :: 2", | ||
'Programming Language :: Python :: 2.7', | ||
'Programming Language :: Python :: 3', | ||
'Programming Language :: Python :: 3.4', | ||
'Programming Language :: Python :: 3.5', | ||
'Programming Language :: Python :: 3.6', | ||
'Programming Language :: Python :: 3.7', | ||
), | ||
packages=['createrepo_c'], | ||
package_dir={ | ||
'createrepo_c': 'src/python/createrepo_c' | ||
}, | ||
cmake_args=[ | ||
'-DBIN_INSTALL_DIR:PATH=src/python/createrepo_c/data/bin', | ||
'-DBUILD_LIBCREATEREPO_C_SHARED:BOOL=OFF', | ||
'-DCREATEREPO_C_INSTALL_DEVELOPMENT:BOOL=OFF', | ||
'-DCREATEREPO_C_INSTALL_MANPAGES:BOOL=OFF', | ||
'-DENABLE_BASHCOMP:BOOL=OFF', | ||
'-DENABLE_DRPM:BOOL=OFF' | ||
], | ||
cmake_languages=['C'], | ||
entry_points={ | ||
'console_scripts': [ | ||
'createrepo_c=createrepo_c:createrepo_c', | ||
'mergerepo_c=createrepo_c:mergerepo_c', | ||
'modifyrepo_c=createrepo_c:modifyrepo_c', | ||
'sqliterepo_c=createrepo_c:sqliterepo_c' | ||
] | ||
}, | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,7 +56,12 @@ SET(headers | |
xml_file.h | ||
xml_parser.h) | ||
|
||
ADD_LIBRARY(libcreaterepo_c SHARED ${createrepo_c_SRCS}) | ||
IF (BUILD_LIBCREATEREPO_C_SHARED) | ||
SET (createrepo_c_library_type SHARED) | ||
ELSE () | ||
SET (createrepo_c_library_type STATIC) | ||
ENDIF () | ||
ADD_LIBRARY(libcreaterepo_c ${createrepo_c_library_type} ${createrepo_c_SRCS}) | ||
TARGET_LINK_LIBRARIES(libcreaterepo_c ${BZIP2_LIBRARIES}) | ||
TARGET_LINK_LIBRARIES(libcreaterepo_c ${CURL_LIBRARY}) | ||
TARGET_LINK_LIBRARIES(libcreaterepo_c ${EXPAT_LIBRARIES}) | ||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Can we just go ahead and change this to use |
||
IF (CREATEREPO_C_INSTALL_DEVELOPMENT OR "${createrepo_c_library_type}" STREQUAL "SHARED") | ||
INSTALL( | ||
TARGETS libcreaterepo_c | ||
RUNTIME DESTINATION ${LIB_INSTALL_DIR} COMPONENT RuntimeLibraries | ||
LIBRARY DESTINATION ${LIB_INSTALL_DIR} COMPONENT RuntimeLibraries | ||
ARCHIVE DESTINATION ${LIB_INSTALL_DIR} COMPONENT Development | ||
) | ||
ENDIF (CREATEREPO_C_INSTALL_DEVELOPMENT OR "${createrepo_c_library_type}" STREQUAL "SHARED") | ||
|
||
IF (CREATEREPO_C_INSTALL_DEVELOPMENT) | ||
INSTALL(FILES ${headers} DESTINATION "include/createrepo_c") | ||
INSTALL(FILES "createrepo_c.pc" DESTINATION "${LIB_INSTALL_DIR}/pkgconfig") | ||
ENDIF (CREATEREPO_C_INSTALL_DEVELOPMENT) | ||
|
||
INSTALL(FILES ${headers} DESTINATION "include/createrepo_c") | ||
INSTALL(FILES "createrepo_c.pc" DESTINATION "${LIB_INSTALL_DIR}/pkgconfig") | ||
INSTALL(TARGETS libcreaterepo_c LIBRARY DESTINATION ${LIB_INSTALL_DIR}) | ||
INSTALL(TARGETS createrepo_c DESTINATION bin/) | ||
INSTALL(TARGETS mergerepo_c DESTINATION bin/) | ||
INSTALL(TARGETS modifyrepo_c DESTINATION bin/) | ||
INSTALL(TARGETS sqliterepo_c DESTINATION bin/) | ||
IF (NOT DEFINED BIN_INSTALL_DIR) | ||
SET(BIN_INSTALL_DIR "bin/") | ||
ENDIF (NOT DEFINED BIN_INSTALL_DIR) | ||
INSTALL( | ||
TARGETS | ||
createrepo_c | ||
mergerepo_c | ||
modifyrepo_c | ||
sqliterepo_c | ||
RUNTIME DESTINATION ${BIN_INSTALL_DIR} COMPONENT Runtime | ||
) | ||
|
||
IF (ENABLE_PYTHON) | ||
ADD_SUBDIRECTORY(python) | ||
ADD_SUBDIRECTORY(python) | ||
ENDIF (ENABLE_PYTHON) |
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