-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
WIP: [ci] [python-package] support setuptools<61 in build-python.sh (fixes #6665) #6681
base: master
Are you sure you want to change the base?
Conversation
echo "" >> ./setup.cfg | ||
echo '[options]' >> ./setup.cfg | ||
echo 'packages = lightgbm' >> ./setup.cfg | ||
echo 'include_package_data = True' >> ./setup.cfg |
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.
WDYT about having a "template" file (say, template_setup.cfg
) with these lines which will be renamed (or its' content will be inserted somewhere) when needed? TBH, these echo
s are really hard to read 😬
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 agree with you, I'm really unhappy with these.
I'd love to avoid checking any more files into source control for this purpose. What about using heredocs?
It would look a little weird in the script because we'd have to left-justify them (I think), but overall they'd be easier to read.
like this:
# use regular-old setuptools for these builds, to avoid
# trying to recompile the shared library
sed -i.bak -e '/start:build-system/,/end:build-system/d' pyproject.toml
echo '[build-system]' >> ./pyproject.toml
echo 'requires = ["setuptools"]' >> ./pyproject.toml
echo 'build-backend = "setuptools.build_meta"' >> ./pyproject.toml
echo "" >> ./pyproject.toml
cat > ./MANIFEST.in <<EOF
recursive-include lightgbm \*.dll \*.dylib \*.so
EOF
cat > ./setup.cfg <<EOF
[options]
packages = lightgbm
include_package_data = True
EOF
mkdir -p ./lightgbm/lib
if test -f ../lib_lightgbm.so; then
echo "found pre-compiled lib_lightgbm.so"
cp ../lib_lightgbm.so ./lightgbm/lib/lib_lightgbm.so
elif test -f ../lib_lightgbm.dylib; then
echo "found pre-compiled lib_lightgbm.dylib"
cp ../lib_lightgbm.dylib ./lightgbm/lib/lib_lightgbm.dylib
elif test -f ../Release/lib_lightgbm.dll; then
echo "found pre-compiled Release/lib_lightgbm.dll"
cp ../Release/lib_lightgbm.dll ./lightgbm/lib/lib_lightgbm.dll
elif test -f ../windows/x64/DLL/lib_lightgbm.dll; then
echo "found pre-compiled windows/x64/DLL/lib_lightgbm.dll"
cp ../windows/x64/DLL/lib_lightgbm.dll ./lightgbm/lib/lib_lightgbm.dll
cp ../windows/x64/DLL/lib_lightgbm.lib ./lightgbm/lib/lib_lightgbm.lib
fi
rm -f ./*.bak
That'd be a short-term fix... longer-term, I think we could use CMake
to get all this stuff out of this script, like having something of this form:
find_library(lightgbm)
if(lightgbm_FOUND)
return()
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.
What about using heredocs?
Looks much better than echo
s 👍
Fixes #6665
As described in #6665 (comment),
build-python.sh install --precompile
is broken on Ubuntu 22.04 because thesetuptools
that gets installed with thepython3-*
packages for that distribution is older than v61.0.0 (the first version that supported pyproject.toml).To fix that, this proposes having
build-python.sh
write asetup.cfg
file when option--precompile
is passed to it.Notes for Reviewers
This is temporary... but should be supported for at least a few years
...or until we can find a better way to support such installs, maybe by using
build
+ more CMake to find the already-builtlib_lightgbm.so
.Per https://ubuntu.com/about/release-cycle, Ubuntu 22.04 will receive "standard support" for 2.5+ more years.
This should be totally forward-compatible
Later
setuptools
versions preferpyproject.toml
tosetup.cfg
, andpyproject.toml
takes precedent when both are found. From the release notes forsetuptools==61.0.0
(link)How I tested this
Ran the following from the root of the repo, on an M2 Mac (arm64).
testing code (click me)
On
master
, that reproduces the error reported in #6665. On this branch, it succeeds.