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

Use setuptools_scm instead of custom version determination code #180

Merged
merged 5 commits into from
Jul 16, 2022

Conversation

bnavigator
Copy link
Collaborator

This eliminates the need of a custom setup.cfg.in and _version.py.in in our sources. It also removes the custom 4 part versioning scheme with PEP404 compliant schemes.

@roryyorke
Copy link
Collaborator

This looks good. I assume the coverage reduction is a false positive due to the smaller number of total lines, due in turn to code removed from setup.py?

FWIW for anyone else reviewing, setuptools_scm looks like a safe, well-maintained dependency, with 18M downloads per month at time of writing.

Some remarks:

  • should we specify setuptools >= 45 ? (see below)
  • should importlib_metadata be in pyproject.toml, or does setuptools_scm already declare a dependency on it for Python < 3.8?
  • _skbuild tripped me up a few times; it's a pity we can't have multiple of these with each having, say, a venv-specific suffix (easier said than done, I imagine).

I've done a bit of testing; below is what I found on my Ubuntu 20.04 machine. Main issue is that conda in-place build fails.

python3 -m build works fine, executed as follows:

#!/bin/bash

# build with `python -m build`
set -e

outdir=$(mktemp -d -t slycotwheel-XXXXXX)
venvdir=$(mktemp -d -t slycotvenv-XXXXXX)
echo "Wheels to to ${outdir}; venvdir is ${venvdir}"

python3 -m build ~/src/slycot --outdir ${outdir}
python3 -m venv ${venvdir}
source ${venvdir}/bin/activate
pip install ${outdir}/*
python -c "import slycot; print(slycot.__version__)"
pip install scipy pytest
python -c "import slycot; slycot.test()"

A non-conda in-place build works, though the first time I tried it I got this message:

/tmp/slycotvenv-5mwTyB/lib/python3.8/site-packages/setuptools_scm/integration.py:27: RuntimeWarning: 
ERROR: setuptools==44.0.0 is used in combination with setuptools_scm>=6.x

Your build configuration is incomplete and previously worked by accident!
setuptools_scm requires setuptools>=45

I changed my venv creation to have a newer version of setuptools, and all was fine (see below).

#!/bin/bash
set -e

venvdir=$(mktemp -d -t slycotvenv-XXXXXX)
python3 -m venv ${venvdir}
echo "venvdir is ${venvdir}"
source ${venvdir}/bin/activate
pip install "setuptools>=45.0" "setuptools_scm>=6.3" "wheel" "scikit-build>=0.14.1" "cmake" "numpy!=1.23.0"
cd ~/src/slycot

python setup.py build_ext --inplace

However, a conda in-place build did not work; the build is fine, but import slycot results in

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/rory/src/slycot/slycot/__init__.py", line 52, in <module>
    __version__ = imv(__package__)
  File "/home/rory/miniconda3/envs/sd-2/lib/python3.10/importlib/metadata/__init__.py", line 984, in version
    return distribution(distribution_name).version
  File "/home/rory/miniconda3/envs/sd-2/lib/python3.10/importlib/metadata/__init__.py", line 957, in distribution
    return Distribution.from_name(distribution_name)
  File "/home/rory/miniconda3/envs/sd-2/lib/python3.10/importlib/metadata/__init__.py", line 548, in from_name
    raise PackageNotFoundError(name)
importlib.metadata.PackageNotFoundError: No package metadata was found for slycot

conda in-place build:

conda create -n sd-2 --channel conda-forge python numpy scipy pytest gfortran_linux-64 gxx_linux-64 scikit-build setuptools setuptools_scm importlib_metadata ninja
conda activate sd-2
git clean -xfd  # in working tree, obviously
conda develop .
python setup.py build_ext --inplace -- -DNumPy_INCLUDE_DIR=$(python -q -c 'import numpy; print(numpy.get_include())') -DCMAKE_Fortran_COMPILER=x86_64-conda_cos6-linux-gnu-gfortran -DCMAKE_C_COMPILER=x86_64-conda_cos6-linux-gnu-gcc

@bnavigator
Copy link
Collaborator Author

  • should we specify setuptools >= 45 ? (see below)

Let's do that. setuptools_scm should do it themselves, but they only do it for their own build system, not in install_requires.

@bnavigator
Copy link
Collaborator Author

However, a conda in-place build did not work

This needs some further investigation. Maybe it is better to not rely on importlib[._]metadata and use a setuptools_scm generated version.py instead. Do we have any downstream packages relying on the slycot.version / slycot.version API?

Should we include such build schemes into the CI?

@roryyorke
Copy link
Collaborator

Thanks, that fixed the conda in-place build problem, and the other two builds are still good.

Do we have any downstream packages relying on the slycot.version / slycot.version API?

No idea. We have 6 dependents according to libraries.io. We're back to this, I reckon leave as-is.

Should we include such build schemes into the CI?

I'm not sure. The build setup that matters most to me for development is a conda environment with in-place build; that lets me easily test and debug Slycot on different Python versions. The build setup that matters most for use of Slycot is the conda-forge feedstock.

I think we can merge - did you want to change anything else?

@bnavigator
Copy link
Collaborator Author

Can we hold on for a little bit? I am considering to move more metadata from setup.py to pyproject.toml.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants