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

sage.env.sage_include_directories: Don't use distutils and SAGE_LIB #33137

Closed
tornaria opened this issue Jan 9, 2022 · 25 comments · Fixed by #35118
Closed

sage.env.sage_include_directories: Don't use distutils and SAGE_LIB #33137

tornaria opened this issue Jan 9, 2022 · 25 comments · Fixed by #35118

Comments

@tornaria
Copy link
Contributor

tornaria commented Jan 9, 2022

  • src/sage/env.py uses distutils.sysconfig.get_python_inc() which is deprecated, it has no replacement in sysconfig.

Suggestion from fbissey: "we could use sysconfig.get_config_var('INCLUDEPY') which should give the same value." #33135 comment:7

We also remove the use of SAGE_LIB in preparation of namespace packages.

This is the last use of distutils in the Sage library, except for uses in src/sage/features/__init__.py and src/sage/misc/cython.py, which are merely fallbacks when setuptools is not present.

Comment from arojas: "I don't see any code in sagelib or cython that would emit this. Is this still needed at all?"

Follow-up: Once these uses of distutils are removed, their modules can be removed from the regex in the call to filterwarnings introduced by #33135.

Part of

Depends on #32873

CC: @antonio-rojas @dimpase

Component: misc

Author: Frédéric Chapoton, Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/33137

@tornaria tornaria added this to the sage-9.6 milestone Jan 9, 2022
@kiwifb
Copy link
Member

kiwifb commented Jan 9, 2022

comment:1

get_config_var as the other advantage to be usable from python 3.8 and 3.9. So we could use it now.

fbissey@tarazed ~ $ python3.9
Python 3.9.9 (main, Dec  1 2021, 10:05:37) 
[GCC 11.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import distutils.sysconfig
>>> distutils.sysconfig.get_python_inc()
'/usr/include/python3.9'
>>> import sysconfig
>>> sysconfig.get_config_var('INCLUDEPY')
'/usr/include/python3.9'
>>> 
fbissey@tarazed ~ $ python3.8
Python 3.8.12 (default, Nov  4 2021, 22:08:42) 
[GCC 11.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sysconfig
>>> sysconfig.get_config_var('INCLUDEPY')
'/usr/include/python3.8'
>>> 

@fchapoton
Copy link
Contributor

Commit: 35d230a

@fchapoton
Copy link
Contributor

Branch: public/ticket-33137

@fchapoton
Copy link
Contributor

comment:2

une tentative.


New commits:

35d230aless distutils in env.py and features

@kiwifb
Copy link
Member

kiwifb commented Jan 10, 2022

comment:3

I'd have done the first part last night [exactly that code] but I couldn't push to trac. Turns out my ssh key is now too old and needs replacing :)

@fchapoton
Copy link
Contributor

comment:4

Salut,

pour le reste, on peut aussi virer le bloc, plutot que de mettre RuntimeError

@kiwifb
Copy link
Member

kiwifb commented Jan 10, 2022

comment:5

Replying to @fchapoton:

Salut,

pour le reste, on peut aussi virer le bloc, plutot que de mettre RuntimeError

Toi, tu vas encore te faire taper sur les doigts parce que tu parle Français sur un système où l'Anglais est la "lingua Franca" (je sais, c'est bizarre d'écrire ça).

Otherwise, I am not too fussed about removing that block. I cannot see a test showing what it is supposed to catch and how. Antonio says it is a dead code path and there is clearly no proof otherwise anywhere.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jan 10, 2022

Dependencies: #32873

@mkoeppe

This comment has been minimized.

@fchapoton
Copy link
Contributor

comment:8

apparemment le changement dans features est en conflit avec #32873

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 11, 2022

Changed commit from 35d230a to 58b0948

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 11, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

58b0948less distutils in env.py

@kiwifb
Copy link
Member

kiwifb commented Jan 11, 2022

comment:10

Replying to @fchapoton:

apparemment le changement dans features est en conflit avec #32873

Yes and I positively reviewed that ticket. I thought we were dealing with something different. In another ticket of mine about doctest failures on distro, Matthias pointed me out to three tickets - one of which I reviewed, again. It seems to me there are far too many tickets of that kind not included in 9.5.rc0. It is hard to figure out what is already on trac.

@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Apr 2, 2022
@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 2, 2022

comment:12

From tornaria #33473 comment:22:
indeed it seems setuptools adds the python include location to the compiler command line, in fact with the following comment in setuptools/_distutils/command/build_ext.py:

      # Put the Python "system" include dir at the end, so that
      # any local include dirs take precedence.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 2, 2022

Changed commit from 58b0948 to 824e9fa

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 2, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

824e9fasage.env.sage_include_directories: Do not append distutils.sysconfig.get_python_inc()

@mkoeppe mkoeppe changed the title Don't use distutils in sage sage.env.sage_include_directories: Don't use distutils and SAGE_LIB Jul 2, 2022
@mkoeppe

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 2, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

fb965ecsage.env.sage_include_directories: Append sysconfig variable INCLUDEPY

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 2, 2022

Changed commit from 824e9fa to fb965ec

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 2, 2022

Author: Frédéric Chapoton, Matthias Koeppe

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 2, 2022

comment:18

Replying to @mkoeppe:

From tornaria #33473 comment:22:
indeed it seems setuptools adds the python include location to the compiler command line, in fact with the following comment in setuptools/_distutils/command/build_ext.py:

      # Put the Python "system" include dir at the end, so that
      # any local include dirs take precedence.

Nevertheless, I have put sysconfig.get_config_var('INCLUDEPY') back in: sage.misc.cython.cython appends some directories to the result of sage_include_directories(), so let's not provoke some subtle changes in the order of looking up libraries when users use the runtime Cython.

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe removed this from the sage-9.7 milestone Aug 31, 2022
@mkoeppe mkoeppe added this to the sage-9.8 milestone Aug 31, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 26, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

9a77a81sage.env.sage_include_directories: Do not append distutils.sysconfig.get_python_inc()
e2688efsage.env.sage_include_directories: Append sysconfig variable INCLUDEPY

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 26, 2022

Changed commit from fb965ec to e2688ef

@mkoeppe mkoeppe modified the milestones: sage-9.8, sage-9.9 Jan 7, 2023
@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 13, 2023

Removed branch from issue description; replaced by PR #35118.

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

Successfully merging a pull request may close this issue.

4 participants