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

fix: Use shutil.which over distutils.spawn.find_executable when possible #1585

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,13 @@ jobs:
python3 -c 'import pyxrootd; print(pyxrootd)'
python3 -c 'from XRootD import client; print(client.FileSystem("root://someserver:1094"))'

- name: Build sdist using publishing workflow
run: |
. /opt/rh/devtoolset-7/enable
cp packaging/wheel/* .
./publish.sh
ls -lhtra dist/
Comment on lines +229 to +234
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simonmichal I added in this chunk into the CI after the RPM build so that your comment is now tested against in the case of the RPM install.


sdist-centos7:

runs-on: ubuntu-latest
Expand Down
1 change: 0 additions & 1 deletion bindings/python/setup_pypi.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
from distutils import sysconfig
from os import getenv, walk, path, path, getcwd, chdir
from platform import system
import subprocess

# Remove the "-Wstrict-prototypes" compiler option, which isn't valid for C++.
cfg_vars = sysconfig.get_config_vars()
Expand Down
4 changes: 4 additions & 0 deletions packaging/rhel/xrootd.spec.in
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,17 @@ BuildRequires: json-c-devel

%if %{python2only}
BuildRequires: python2-devel
BuildRequires: python2-setuptools
%endif
%if %{python2and3}
BuildRequires: python2-devel
BuildRequires: python2-setuptools
BuildRequires: python%{python3_pkgversion}-devel
BuildRequires: python%{python3_pkgversion}-setuptools
%endif
%if %{python3only}
BuildRequires: python%{python3_pkgversion}-devel
BuildRequires: python%{python3_pkgversion}-setuptools
%endif

BuildRequires: openssl-devel
Expand Down
17 changes: 15 additions & 2 deletions packaging/wheel/install.sh
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,28 @@ if [ "$res" -ne "0" ]; then
fi

cd ../bindings/python
$6 setup.py install $3 # $6 holds the python sys.executable
res=$?

# Determine if shutil.which is available for a modern Python package install
${6} -c 'import shutil.which' &> /dev/null # $6 holds the python sys.executable
shutil_which_available=$?
if [ "${shutil_which_available}" -ne "0" ]; then
${6} setup.py install ${3}
res=$?
else
${6} -m pip install ${3} .
res=$?
fi
unset shutil_which_available
Comment on lines +42 to +52
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we're checking for shutil.which in packaging/wheel/setup.py then do so here as well. If it exists, then we're able to start modernizing by using python -m pip install to install the sdist. If no, then fall back to using the deprecated python setup.py install for the time being.


if [ "$res" -ne "0" ]; then
exit 1
fi

cd $startdir
rm -r xrootdbuild

# TODO: Remove all of the below and build a wheel using PyPA tools

# convert the egg-info into a proper dist-info
egginfo_path=$(ls $1/xrootd-*.egg-info)
core="${egginfo_path%.*}"
Expand Down
33 changes: 32 additions & 1 deletion packaging/wheel/publish.sh
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,35 @@ version=$(./genversion.sh --print-only)
version=${version#v}
echo $version > bindings/python/VERSION
rm -r dist
python3 setup.py sdist

# Determine if wheel.bdist_wheel is available for wheel.bdist_wheel in setup.py
python3 -c 'import wheel' &> /dev/null
wheel_available=$?
if [ "${wheel_available}" -ne "0" ]; then
python3 -m pip install wheel
wheel_available=$?

if [ "${wheel_available}" -ne "0" ]; then
echo "ERROR: Unable to find wheel and unable to install wheel with '$(command -v python3) -m pip install wheel'."
echo " Please ensure wheel is available to $(command -v python3) and try again."
exit 1
fi
fi
unset wheel_available

# Determine if build is available
python3 -c 'import build' &> /dev/null
build_available=$?
if [ "${build_available}" -ne "0" ]; then
python3 -m pip install build
build_available=$?
fi

if [ "${build_available}" -ne "0" ]; then
echo "WARNING: Unable to find build and unable to install build from PyPI with '$(command -v python3) -m pip install build'."
echo " Falling back to building sdist with '$(command -v python3) setup.py sdist'."
python3 setup.py sdist
else
python3 -m build --sdist .
fi
unset build_available
Comment on lines +9 to +39
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cleaned up the logic here so that the errors @simonmichal was seeing should be eliminated now.

24 changes: 13 additions & 11 deletions packaging/wheel/setup.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
from setuptools import setup, Extension
from setuptools import setup
from setuptools.command.install import install
from setuptools.command.sdist import sdist
from wheel.bdist_wheel import bdist_wheel

# shutil.which was added in Python 3.3
# c.f. https://docs.python.org/3/library/shutil.html#shutil.which
try:
from shutil import which
except ImportError:
from distutils.spawn import find_executable as which
Comment on lines +6 to +11
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In modernizing the Python packaging, it is preferable to use shutil.which, so if it is available use it. If you're on Python 2, fall back to using distutils.spawn.find_executable.


import subprocess
import sys
import getpass

def get_version():
version = subprocess.check_output(['./genversion.sh', '--print-only'])
Expand All @@ -27,11 +33,9 @@ def get_version_from_file():

def binary_exists(name):
"""Check whether `name` is on PATH."""
from distutils.spawn import find_executable
return find_executable(name) is not None
return which(name) is not None

def check_cmake3(path):
from distutils.spawn import find_executable
args = (path, "--version")
popen = subprocess.Popen(args, stdout=subprocess.PIPE)
popen.wait()
Expand All @@ -42,12 +46,11 @@ def check_cmake3(path):

def cmake_exists():
"""Check whether CMAKE is on PATH."""
from distutils.spawn import find_executable
path = find_executable('cmake')
path = which('cmake')
if path is not None:
if check_cmake3(path): return True, path
path = find_executable('cmake3')
return path is not None, path
path = which('cmake3')
return path is not None, path

def is_rhel7():
"""check if we are running on rhel7 platform"""
Expand Down Expand Up @@ -78,9 +81,8 @@ def has_cxx14():

# def python_dependency_name( py_version_short, py_version_nodot ):
# """ find the name of python dependency """
# from distutils.spawn import find_executable
# # this is the path to default python
# path = find_executable( 'python' )
# path = which( 'python' )
# from os.path import realpath
# # this is the real default python after resolving symlinks
# real = realpath(path)
Expand Down