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.

Sign up for GitHub

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

Jump to bottom

[FIX] Fix import syntax to enable shutil.which check #1672

Merged
merged 6 commits into from
Apr 26, 2022
Merged

[FIX] Fix import syntax to enable shutil.which check #1672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
991fcb6
[FIX] Use python's sys.version_info.minor to ensure correct library
matthewfeickert Apr 7, 2022
1c885e0
[FIX] Fix import syntax to enable shutil.which check
matthewfeickert Apr 18, 2022
03d1ac8
[FIX] Remove brew install of git for cmake-macos job
matthewfeickert Apr 21, 2022
5791b27
[pip] Remove manual renaming of egg-info to dist-info
matthewfeickert Apr 18, 2022
5088d6e
[CI] Update checkout action to v3
matthewfeickert Apr 7, 2022
3f1340a
[CI] Add ubuntu sdist build workflow
matthewfeickert Apr 7, 2022
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
67 changes: 60 additions & 7 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ jobs:
python3 -m pip --no-cache-dir install --upgrade pip setuptools wheel

- name: Clone repository
uses: actions/checkout@v2
uses: actions/checkout@v3
with:
fetch-depth: 0

Expand Down Expand Up @@ -282,8 +282,8 @@ jobs:
- name: Verify Python bindings
run: |
export LD_LIBRARY_PATH="/usr/local/lib:${LD_LIBRARY_PATH}"
export PYTHON_VERSION_MINOR=$(_tmp=$(find /usr/local/lib/ -type d -iname "python*") && echo ${_tmp: -3}; unset _tmp)
export PYTHONPATH="/usr/local/lib/python${PYTHON_VERSION_MINOR}/site-packages:${PYTHONPATH}"
export PYTHON_VERSION_MINOR=$(python3 -c 'import sys; print(f"{sys.version_info.minor}")')
export PYTHONPATH="/usr/local/lib/python3.${PYTHON_VERSION_MINOR}/site-packages:${PYTHONPATH}"
python3 -m pip list
python3 -m pip show xrootd
python3 -c 'import XRootD; print(XRootD)'
Expand All @@ -305,7 +305,6 @@ jobs:
krb5 \
ossp-uuid \
libxml2 \
git \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needs to be removed as the brew install conflicts with what the GitHub Action runner macOS environment already has installed and the job fails.

openssl@3

- name: Install necessary Python libraries
Expand All @@ -314,7 +313,7 @@ jobs:
python3 -m pip list

- name: Clone repository
uses: actions/checkout@v2
uses: actions/checkout@v3
with:
fetch-depth: 0

Expand Down Expand Up @@ -446,7 +445,7 @@ jobs:
dnf clean all

- name: Clone repository
uses: actions/checkout@v2
uses: actions/checkout@v3
with:
fetch-depth: 0

Expand Down Expand Up @@ -505,7 +504,7 @@ jobs:
sudo apt-get autoclean -y

- name: Clone repository
uses: actions/checkout@v2
uses: actions/checkout@v3
with:
fetch-depth: 0

Expand Down Expand Up @@ -642,3 +641,57 @@ jobs:
python3 -c 'import XRootD; print(XRootD)'
python3 -c 'import pyxrootd; print(pyxrootd)'
python3 -c 'from XRootD import client; print(client.FileSystem("root://someserver:1094"))'

sdist-ubuntu:

# Use of sudo as https://github.com/actions/virtual-environments requires it
runs-on: ubuntu-latest

steps:
- name: Install external dependencies with apt-get
run: |
sudo apt-get update -y
DEBIAN_FRONTEND=noninteractive sudo apt-get install -y \
g++ \
git \
cmake \
uuid-dev \
dpkg-dev \
libssl-dev \
libx11-dev \
python3 \
python3-pip \
python3-venv \
python3-dev \
pkg-config \
tree
sudo apt-get autoclean -y
python3 -m pip --no-cache-dir install --upgrade pip setuptools wheel
python3 -m pip list

- name: Clone repository
uses: actions/checkout@v3
with:
fetch-depth: 0

- name: Build sdist using publishing workflow
run: |
cp packaging/wheel/* .
./publish.sh
python3 -m pip --verbose install ./dist/xrootd-*.tar.gz
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adriansev I just tested this locally in a ubuntu:20.04 contaienr using a --user install and that works as well, as expected. 👍

python3 -m pip list

- name: Show site-pacakges layout for XRootD modules
run: |
find $(python3 -c 'import XRootD; import pathlib; print(str(pathlib.Path(XRootD.__path__[0]).parent))') \
-type d \
-iname "*xrootd*" | xargs tree

- name: Verify Python bindings
run: |
python3 --version --version
python3 -m pip list
python3 -m pip show xrootd
python3 -c 'import XRootD; print(XRootD)'
python3 -c 'import pyxrootd; print(pyxrootd)'
python3 -c 'from XRootD import client; print(client.FileSystem("root://someserver:1094"))'
29 changes: 3 additions & 26 deletions packaging/wheel/install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@ fi
cd ../bindings/python

# 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 was added in Python 3.3, so any version of Python 3 now will have it)
# TODO: Drop support for Python 3.3 and simplify to pip approach
Comment on lines +43 to +44
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 a follow up PR (to keep scope simple) if people are okay with dropping support for Python 3.1 and Python 3.2 (EOL was October 11, 2014) then this can all get simplified down to just using pip. As the wheels are assuming you're using Python 3 anyway this should not be a problem for anyone.

${6} -c 'from shutil import which' &> /dev/null # $6 holds the python sys.executable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the relevant fix.

Wrong behavior

>>> import shutil.which
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'shutil.which'; 'shutil' is not a package

Correct behavior

>>> from shutil import which

shutil_which_available=$?
if [ "${shutil_which_available}" -ne "0" ]; then
${6} setup.py install ${3}
Expand All @@ -57,28 +59,3 @@ 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%.*}"
core="${egginfo_path#$1/}"
sufix="${core#xrootd-*.*.*_-}"
core="${core%_-*}"
if [[ "$core" == "$sufix" ]]
then
distinfo_path="${egginfo_path%.*}.dist-info"
else
distinfo_path="$1/$core-$sufix"
fi
echo $distinfo_path >> /tmp/out.txt
mkdir $distinfo_path
mv $egginfo_path $distinfo_path/METADATA
echo -e "Wheel-Version: 1.0\nGenerator: bdist_wheel (0.35.1)\nRoot-Is-Purelib: true\nTag: py2-none-any\nTag: py3-none-any" > $distinfo_path/WHEEL
touch $distinfo_path/RECORD
distinfo_name=${distinfo_path#"$1"}
find $1/pyxrootd/ -type f -exec sha256sum {} \; | awk '{printf$2 ",sha256=" $1 "," ; system("stat --printf=\"%s\" "$2) ; print '\n'; }' >> $distinfo_path/RECORD
find $1/$distinfo_name -type f -exec sha256sum {} \; | awk '{printf$2 ",sha256=" $1 "," ; system("stat --printf=\"%s\" "$2) ; print '\n'; }' >> $distinfo_path/RECORD
find $1/XRootD/ -type f -exec sha256sum {} \; | awk '{printf$2 ",sha256=" $1 "," ; system("stat --printf=\"%s\" "$2) ; print '\n'; }' >> $distinfo_path/RECORD
find $1/pyxrootd/ -type l | awk '{print$1 ",,"}' >> $distinfo_path/RECORD
Comment on lines -60 to -84
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless there is some specific reason to need to rename things to act like a wheel instead of an egg for Pythons that have old pips then this should get removed to simplify things and to avoid files not existing causing errors during the build.

If you have Python 3.3+ and a modern pip then pip can handle the build and install of a wheel from the sdist. Otherwise (that is you're on Python 3.2 (which is very very EOL so please don't) or using a pip that is quite old) you'll get a totally fine egg and can be used or uninstalled normally, so there's no clear motivation (to me) to go through attempting to renamed its egg-info dir to dist-info.

Copy link
Contributor

Choose a reason for hiding this comment

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

@matthewfeickert i believe that the lowest baseline is centos 7 that have python 3.6.8 and pip 9.0.3 (setuptools 39.2.0 wheel 0.31.1) so anything lower than this can go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! That's what I assumed. CentOS 7 is going to hold people at Python 3.6 for a long time, but I don't know of any real world cases where people are still using any Python 3.X version older than Python 3.6. 👍