-
Notifications
You must be signed in to change notification settings - Fork 499
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
Prepare for Pypi installable Ledger Python package #2246
base: feature/pypi
Are you sure you want to change the base?
Conversation
${SKBUILD_PROJECT_NAME} | ||
VERSION ${SKBUILD_PROJECT_VERSION} | ||
LANGUAGES C CXX | ||
) | ||
|
||
list(APPEND CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/doc") | ||
include(LedgerVersion) |
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.
Handling of version numbers could use a clean-up, I'll leave this for future me.
if (HAVE_EDIT) | ||
target_link_libraries(${_target} ${EDIT_LIB}) | ||
target_link_libraries(${_target} PUBLIC ${EDIT_LIB}) |
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.
When using Python_add_library
(see below) CMake complained about use of keyword vs. plain arguments; adding the PUBLIC
and PRIVATE
scope keywords here fixed the error.
ℹ️ ${EDIT_LIB}
needs to be public as the ${LEDGER_CLI_SOURCES}
use it and the linker will fail otherwise.
set_target_properties(${_target} PROPERTIES LINK_FLAGS "-undefined dynamic_lookup") | ||
else() | ||
target_link_libraries(${_target} ${Boost_LIBRARIES} ${Python_LIBRARIES}) | ||
target_link_libraries(${_target} PUBLIC ${Boost_LIBRARIES} ${Python_LIBRARIES}) |
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.
ℹ️ The ${Boost_LIBRARIES}
need to be public as the Util and Math tests use it and the linker will fail otherwise.
if (NOT SKBUILD) | ||
set(SKBUILD_PROJECT_NAME ledger) | ||
set(SKBUILD_PROJECT_VERSION 3.3.2) | ||
endif() |
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.
The SKBUILD
variables are set when building using the scikit-build-core build backend with pip
add_subdirectory(doc) | ||
add_subdirectory(contrib) | ||
add_subdirectory(test) | ||
endif() |
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.
No need to build the supporting stuff for the Python package.
[project.urls] | ||
homepage = "https://ledger-cli.org" | ||
documentation = "https://ledger-cli.org/docs.html" | ||
repository = "https://git.ledger-cli.org/ledger" |
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.
Do the above project metadata seem reasonable to folks?
|
||
[tool.scikit-build.cmake.define] | ||
USE_PYTHON = "ON" | ||
USE_GPGME = "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.
To be most useful I think the Ledger Python Package should provide support for encrypted journals.
If many folks disagree I can look into possibly making it a install-time option, if scikit-build-core supports that.
# FIXME: symlink would be sufficient: | ||
# maybe using install(CODE "...") and | ||
# execute_process(COMMAND "${CMAKE_COMMAND}" -E create_symlink ...). | ||
# Windows will need a special case due to not supporting symlinks. | ||
add_custom_command( | ||
TARGET libledger POST_BUILD | ||
COMMAND ${CMAKE_COMMAND} -E copy_if_different | ||
$<TARGET_FILE:libledger> "${CMAKE_BINARY_DIR}/${_ledger_python_module_name}") | ||
$<TARGET_FILE:libledger> | ||
"${CMAKE_BINARY_DIR}/python/${SKBUILD_PROJECT_NAME}/_${_ledger_python_module_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.
ℹ️ This somewhat unusual destination is necessary in order to make the python related tests pass from CMake's build directory.
I'm on Ubuntu 23.04 For Python 3.9 and 3.10, because apparently my system has only libboost Python binding for Python 3.11
See complete logs: |
Also note #2332 may just render this effort futile. |
This PR adds support for building the Ledger Python package via pip and possibly other Python installation methods supporting
pyproject.toml
using scikit-build-core build backends. 🐍🥳Please note, that this PR focuses on the build setup for Python and non-Python builds. There is another conversation to be had about how to best structure the Ledger Python package, i.e. where the Ledger (binary) Python module should reside, and how to deal with backwards compatibility.
Here are the steps in which this PR has been tested:
python -m venv venv
source venv/bin/activate
pip install -U pip
(optional)/usr/bin/env CMAKE_BUILD_PARALLEL_LEVEL=$(sysctl -n hw.ncpu) pip install --verbose --no-dependencies git+https://github.com/afh/ledger.git@pypi
python ${PATH_TO}/ledger.git/python/demo.py
Folks wanting to test are invited to follow the instructions above and try out any alternative way to install the Ledger Python package. If you do, please your experience, especially if you ran into problems.
I left a few notes as comments below for the reviewers.
ℹ️ Given the potential impact of this PR I've set it up as a draft to have a conversation about the future of the Ledger Python Package.
Looking forward to feedback 🙂
Closes #1934