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

Prepare python3 spkg-configure.m4 for Python 3.12 (setuptools instead of distutils) #36983

Merged
merged 5 commits into from
Feb 13, 2024

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Dec 31, 2023

This PR does not change what versions are accepted!
To test with Python 3.12, still need to adjust the version bound set in build/pkgs/python3/spkg-configure.m4.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

Matthias Koeppe added 2 commits December 31, 2023 14:27
@tornaria
Copy link
Contributor

tornaria commented Jan 2, 2024

Looks ok to me, but I don't have the means to test this. There's too many in-flight stuff going on. I only got chatting with you about this since I wanted to have a try at #36957 just to see how it works, but not even this PR would break it (I don't have the inclination to work or review 10 prs just to test a thing I don't really care about and don't use / won't use).

I'd be tempted to positive review this PR on the basis of checks passing and you knowing what you are doing.

The situation brings to mind this: https://martinfowler.com/articles/ship-show-ask.html

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 2, 2024

I'd be tempted to positive review this PR on the basis of checks passing and you knowing what you are doing.

Good enough for me.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 2, 2024

Thanks!

@vbraun
Copy link
Member

vbraun commented Jan 5, 2024

Merge conflict

Copy link

github-actions bot commented Feb 3, 2024

Documentation preview for this PR (built with commit 0e54d2c; changes) is ready! 🎉

Comment on lines +23 to +25
AX_COMPARE_VERSION([$python3_version], [ge], [3.12.0], [
conftest_venv_options="--system-site-packages"
distutils_core="setuptools"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move to setuptools unconditionally, instead of only for python 3.12?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, that would require people to have an additional system package installed that isn't going to be used other than for this configure check.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just setuptools. Besides, what makes it ok to require setuptools for python 3.12 but not for python 3.11?

In fact, it seems you don't really need to require the presence of neither distutils nor setuptools at all. If distutils or setuptools is available from system, it can be used, otherwise setuptools will be installed in the venv and everything will be fine.

The only reason one checks for a few modules here is because they are built-in python modules that can't be installed after the fact on top of a system python. But setuptools can (and will) be installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what makes it ok to require setuptools for python 3.12

You know the answer. It's OK because without it we cannot test what we want to test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If distutils or setuptools is available from system, it can be used, otherwise setuptools will be installed in the venv and everything will be fine.

Not really. We stopped using distutils from the system a long time ago (by setting SETUPTOOLS_USE_DISTUTILS=local), and we do not use setuptools from the system (except in the experimental system-site mode, which I'll not comment on).

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 don't understand. All of what I wrote is about SageMath

Copy link
Contributor

Choose a reason for hiding this comment

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

"We had quite a bit of trouble with this on macOS (both plain Xcode and homebrew)."

I was asking for an example of this, i.e. broken systems that cause trouble with sagemath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

macOS Xcode + homebrew was pretty problematic in #31227.

And the link to python/cpython#90539 (comment) that I sent earlier has the pointers to Sage tickets where Python was unusable on Homebrew, Cygwin, and pyenv.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also openSUSE was affected by the same (#33153).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thanks.

vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 11, 2024
    
This is a minimal PR to allow building with python 3.12, given the
circumstances surrounding sagemath#36181.

I've tested this works with system python 3.12 by:
```
./bootstrap
./configure --disable-doc --disable-editable --enable-system-site-
packages
env 'MAKE=make -j36' make
./sage -tp 36 --all
./sage -tp 36 --all --long
```

It only gave the expected failures in
`src/sage/matroids/database_collections.py` from sagemath#37140.

Since scipy 1.12 works ok after sagemath#37123 and it's just a one-liner to
change the required version, I included it here.

No attempt is made at upgrading anything in sage-the-distro.

### 📝 Checklist

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.

EDIT: rebased on top of sagemath#36983 to address reviewer suggestion.
    
URL: sagemath#37270
Reported by: Gonzalo Tornaría
Reviewer(s): Aliaksei Urbanski, Gonzalo Tornaría, Tobias Diez
@vbraun vbraun merged commit 647ec81 into sagemath:develop Feb 13, 2024
63 of 70 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants