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

MAINT: use PEP440 vs. distutils #15256

Merged
merged 1 commit into from Dec 22, 2021
Merged

Conversation

tylerjereddy
Copy link
Contributor

Fixes #15254 (eventually)

  • version checking in a PEP440-compliant manner is
    more robust than using distutils for this purpose

  • replace accordingly, mostly because the latest setuptools
    has finally deprecated i.e., LooseVersion

  • I think the debate about whether we should use an external
    package like packaging for this can be deferred since we
    already vendor scipy/_lib/_pep440.py which says at the top:

The LooseVersion and StrictVersion classes that distutils provides don't
work; they don't recognize anything like alpha/beta/rc/dev versions.

* version checking in a PEP440-compliant manner is
more robust than using `distutils` for this purpose

* replace accordingly, mostly because the latest `setuptools`
has finally deprecated i.e., `LooseVersion`

* I think the debate about whether we should use an external
package like `packaging` for this can be deferred since we
already vendor `scipy/_lib/_pep440.py` which says at the top:

```
The LooseVersion and StrictVersion classes that distutils provides don't
work; they don't recognize anything like alpha/beta/rc/dev versions.
```

* that's a bit harsh for some of our purposes, but the CI is failing
with `LooseVersion` now
@tylerjereddy tylerjereddy added the maintenance Items related to regular maintenance tasks label Dec 20, 2021
@@ -234,7 +234,8 @@ def get_build_ext_override():
from pythran.dist import PythranBuildExt
BaseBuildExt = PythranBuildExt[npy_build_ext]
# Version check - a too old Pythran will give problems
if LooseVersion(pythran.__version__) < LooseVersion('0.9.12'):
importlib.import_module('scipy/_lib/_pep440')
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 gruesome, and I'm not sure we are "allowed" to do this, but we cannot import from within scipy directly from within setup.py as far as I can tell (it dumps a bunch of chicken-and-egg type init errors)

Copy link
Member

Choose a reason for hiding this comment

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

There's a linefrom scipy._build_utils.system_info import get_info, NotFoundError inside configuration(), so it's not completely forbidden. But yeah, if it doesn't work then this workaround is fine.

Copy link
Member

Choose a reason for hiding this comment

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

Double checked, this should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

I get build problems using the latest main (as of now):

Traceback (most recent call last):
  File "/home/nicpa/codes/scipy/setup.py", line 530, in <module>
    setup_package()
  File "/home/nicpa/codes/scipy/setup.py", line 514, in setup_package
    generate_cython()
  File "/home/nicpa/codes/scipy/setup.py", line 255, in generate_cython
    if _pep440.parse(pip.__version__) < _pep440.Version('18.0.0'):
NameError: name '_pep440' is not defined

this seems related to this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this line is missing in line 255 as well...

I can make a PR if you want?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wish it were that easy. It will still copy to a tmpdir (slow rebuilds), and hide the build log. I believe the closest equivalent to python setup.py develop is pip install -v --no-build-isolation --no-use-pep517 -e .. Which is a bit sad:(

Hmm... I see... Yeah, this is not really the pip way...

I'm still pondering on how we get an optimal CLI for the Meson build, because pip is unlikely to be the right answer.

And I have been following this closely, I am very much interested in seeing what Meson can do for Python builds. But I kind-of feel that this requires a complete restructure of the build infrastructure of Python if pip can't be patched. And then we go again with build-systems.

Perhaps the infrastructure should be in meson extensions instead?

Just some random thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps the infrastructure should be in meson extensions instead?

Yes, probably. I've also looked at Nox, which does a lot of things right but it seems to have the annoying habit of coupling everything it does with virtualenvs, and it's not clear on how to get the good parts of Nox without that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know nox, but just by having a look at its documentation page, it doesn't seem like it will fix anything?

To me it is important to be able to build against already installed packages. And many of these venv stuff needs customization flags for running custom builds... Hmm....

Copy link
Member

Choose a reason for hiding this comment

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

What I like from a tool like Nox (in principle) is that it'd give building blocks for things that we now do completely by hand in runtests.py and dev.py. We shouldn't need ~600 LoC to implement our own developer CLI.

To me it is important to be able to build against already installed packages.

Yes, this should be the default for any sane interface. You set up a build env once (which can be expensive), and then you build/test/benchmark over and over.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes! :)

There's a reason there are so many alternatives, a stable all-across solution is not easy :-D

@tylerjereddy
Copy link
Contributor Author

I may also have misunderstood when to use _pep440.parse vs. _pep440.Version -- I think I was just copying from the few examples already in the code base..

@tylerjereddy
Copy link
Contributor Author

some Windows pythran problems in CI as well here

@Smit-create
Copy link
Member

Linux tests failures have a workaround and are fixed in gh-15255

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

LGTM, in it goes. Thanks @tylerjereddy

think the debate about whether we should use an external package like packaging for this can be deferred since we already vendor scipy/_lib/_pep440.py which says at the top:

There's no debate I think, we'll always vendor simple utilities like this:)

@rgommers rgommers merged commit 31c8ba4 into scipy:master Dec 22, 2021
@rgommers rgommers added the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Dec 22, 2021
@rgommers rgommers added this to the 1.8.0 milestone Dec 22, 2021
@rgommers
Copy link
Member

Added as backport candidate.

@rgommers rgommers modified the milestones: 1.8.0, 1.9.0 Dec 22, 2021
@tylerjereddy tylerjereddy deleted the treddy_issue_15254 branch December 22, 2021 21:51
@tylerjereddy tylerjereddy modified the milestones: 1.9.0, 1.8.0 Jan 16, 2022
@tylerjereddy tylerjereddy removed the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Jan 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: DeprecationWarning: distutils Version classes are deprecated
4 participants