check ipython for sphinxext, extend python_package_check macro

[dimpase]

with system-wide ipython, it sometimes happens (e.g. with Fedora's 38 ipython 8.9) that
it does not have sphinxext.

We provide a check here, by extending the sage_python_package_check macro to allow for extra check,
and doing the corresponding check

[mkoeppe]

Can this not just be taken care of by setting a lower version bound?

[dimpase]

No, I don't think so.
It seems that here are gaps in what versions of ipython have the needed feature (might depend on the distro too).

E.g. if a distro doesn't package sphinx in the usual way, or doesn't package it at all, it's not clear why they'd put sphinxext in ipython.

[mkoeppe]

It seems that here are gaps in what versions of ipython have the needed feature (might depend on the distro too).

That would be highly unusual, so I think more investigation is needed.

Note that ALL of Python distribution compatibility is done on the basis of version constraints.
https://peps.python.org/pep-0508/

[dimpase]

It seems that here are gaps in what versions of ipython have the needed feature (might depend on the distro too).

That would be highly unusual, so I think more investigation is needed.

Note that ALL of Python distribution compatibility is done on the basis of version constraints. https://peps.python.org/pep-0508/

Fedora splits ipython into a number of parts, see https://packages.fedoraproject.org/pkgs/ipython/
In particular I'll need to add python-ipython-sphinx to build/pkgs/ipython/distros/fedora.txt.

As we have no control on how much of these is installed, we need a check, and the check in this PR is a generic one (maybe other distros do this, or will do this...)

[mkoeppe]

I'd say this is a distribution bug.

[root@c1233864a514 /]# ls /usr/lib/python3.11/site-packages/IPython/
__init__.py  __main__.py  __pycache__  conftest.py  consoleapp.py  core  display.py  extensions  external  lib  paths.py  py.typed  terminal  testing  utils
[root@c1233864a514 /]# grep sphinxext /usr/lib/python3.11/site-packages/ipython-8.9.0-py3.11.egg-info/SOURCES.txt 
IPython/sphinxext/__init__.py
IPython/sphinxext/custom_doctests.py
IPython/sphinxext/ipython_console_highlighting.py
IPython/sphinxext/ipython_directive.py
docs/source/sphinxext.rst
docs/sphinxext/apigen.py
docs/sphinxext/configtraits.py
docs/sphinxext/github.py
docs/sphinxext/magics.py

Do you happen to know if this has been discussed with upstream?

[dimpase]

I'd say this is a distribution bug.

[root@c1233864a514 /]# ls /usr/lib/python3.11/site-packages/IPython/
__init__.py  __main__.py  __pycache__  conftest.py  consoleapp.py  core  display.py  extensions  external  lib  paths.py  py.typed  terminal  testing  utils
[root@c1233864a514 /]# grep sphinxext /usr/lib/python3.11/site-packages/ipython-8.9.0-py3.11.egg-info/SOURCES.txt 
IPython/sphinxext/__init__.py
IPython/sphinxext/custom_doctests.py
IPython/sphinxext/ipython_console_highlighting.py
IPython/sphinxext/ipython_directive.py
docs/source/sphinxext.rst
docs/sphinxext/apigen.py
docs/sphinxext/configtraits.py
docs/sphinxext/github.py
docs/sphinxext/magics.py

Do you happen to know if this has been discussed with upstream?

I have no idea, but I don't think you'd have much luck convincing them to merge these packages.
After all if you're not going to use sphinx in your project or workflow you won't need sphinxext.

[github-actions]

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

[orlitzky]

This has come up three or four times now, where we need the ability to run some python code in the interpreter that sage will use, and perform different actions based on the result. For example:

1. The tzdata check in spkg-configure.m4 for python: tzdata, pytest, vcversioner #36674 (comment)
2. Before I noticed that PYTHON_MINOR is already defined, it was used in Skip backport packages for newer python #36776
3. This PR

I think now is a good time to refactor the python package check into a free-form if/else check. I whipped up a quick proof-of-concept of a new macro, SAGE_PYTHON_RUN_IFELSE, and then rewrote the existing python package check in terms of that. It worked on the first try, which is extremely suspicious, but here it is:

m4/sage_python_run_ifelse.m4

#
# SYNOPSIS
#
#   SAGE_PYTHON_RUN_IFELSE(code, [action-if-true], [action-if-false])
#
# DESCRIPTION
#
#   Attempt to run the given input using the python that will be used
#   for Sage. The PYTHONUSERBASE variable poisoned in the same manner
#   as sage-env poisons it, to ensure that the configure- and run-time
#   views of the system are as similar as possible.
#
#   The SAGE_SPKG_CONFIGURE_PYTHON3() macro is AC_REQUIRE'd to ensure
#   that $PYTHON_FOR_VENV is available (if we can use the system
#   python). If it is, the check is run inside a new venv; if not, we
#   execute the code directly under "python3".
#
# INPUT
#
#  * code - the python code to run
#
#  * action-if-true - list of commands to execute if the python
#    interpreter exits successfully (with exit code zero)
#
#  * action-if-false - list of commands to execute if the python
#    interpreter exits unsuccessfully (with a non-zero exit code)
#
AC_DEFUN([SAGE_PYTHON_RUN_IFELSE], [

# Create conftest.py containing the user's code
cat >conftest.py <<SAGE__EOF
$1
SAGE__EOF

  # We need to know if we're using the system python or not.
  AC_REQUIRE([SAGE_SPKG_CONFIGURE_PYTHON3])

  # To prevent user-site (pip install --user) packages from affecting
  # the result, we poison PYTHONUSERBASE. The sage-env script also
  # does this at runtime; we mimic that implementation to ensure that
  # the behaviors at configure- and run-time are identical. Beware
  # that (as in sage-env) the poisoning is skipped if PYTHONUSERBASE
  # is non-empty. In particular, if the user points PYTHONUSERBASE to
  # any path (even the default), then his local pip packages will be
  # detected.
  PYTHONUSERBASE_SAVED="${PYTHONUSERBASE}"
  AS_IF([test -z "${PYTHONUSERBASE}"], [
    PYTHONUSERBASE="${HOME}/.sage/local"
  ])

  # Are we using the system python or not? The PYTHON_FOR_VENV
  # variable is only set if we're using the system python.
  AS_IF([test "x$sage_spkg_install_python3" = "xno"],[
    # We run this check inside a python venv, because that's
    # ultimately how the system $PYTHON_FOR_VENV will be used. We use
    # --clear because ./configure typically clobbers its output files.
    # And we pass --system-site-packages to the venv if it was enabled
    # via ./configure --enable-system-site-packages.
    AS_IF([test "${enable_system_site_packages}" = "yes"], [
      _SSP="--system-site-packages"
    ])

    AS_IF(["${PYTHON_FOR_VENV}" build/bin/sage-venv    dnl
                                ${_SSP}                dnl
                                --clear                dnl
                                config.venv            dnl
                                2>&AS_MESSAGE_LOG_FD], [

      AS_IF([PYTHONUSERBASE="${PYTHONUSERBASE}" dnl
               config.venv/bin/python3          dnl
               conftest.py                      dnl
               1>&AS_MESSAGE_LOG_FD             dnl
               2>&AS_MESSAGE_LOG_FD],[$2],[$3])
    ], [
      # Failed to create a venv for some reason?
      $3
    ])

    # Clean up config.venv, but only if we could have created it.
    # (The --clear flag to pyvenv will not clobber a plain file.)
    AS_IF([test -d config.venv], [rm -rf config.venv])
  ],[
    # Not the system python, just run the thing.
    AS_IF([PYTHONUSERBASE="${PYTHONUSERBASE}" dnl
             python3                          dnl
             conftest.py                      dnl
             1>&AS_MESSAGE_LOG_FD             dnl
             2>&AS_MESSAGE_LOG_FD],[$2],[$3])
  ])

  # Un-clobber PYTHONUSERBASE and clean up conftest.py before leaving.
  PYTHONUSERBASE="${PYTHONUSERBASE_SAVED}"
  rm -f conftest.py
])

m4/sage_python_package_check.m4

#
# SYNOPSIS
#
#   SAGE_PYTHON_PACKAGE_CHECK(package)
#
# DESCRIPTION
#
#   Determine if the system copy of a python package can be used by sage.
#
#   This macro uses setuptools.version's pkg_resources to check that the
#   "install-requires.txt" file for the named package is satisfied, and
#   it can typically fail in five ways:
#
#     1. If --enable-system-site-packages was not passed to ./configure,
#
#     2. If we are not using the system python,
#
#     3. If we are unable to create a venv with the system python,
#
#     4. If setuptools is not available to the system python,
#
#     5. If the contents of install-requires.txt are not met (wrong
#        version, no version, etc.) by the system python.
#
#   (The third item takes place in the SAGE_PYTHON_RUN_IFELSE macro.)
#
#   In any of those cases, we set sage_spkg_install_$package to "yes"
#   so that the corresponding SPKG is installed. Otherwise, we do
#   nothing, since the default value of sage_spkg_install_$package
#   is "no" (to use the system copy).
#
#   To avoid suggesting these system packages to users who have not
#   set --enable-system-site-packages, this macro also changes the
#   default for --with-system-foo from "yes" to "no" in that case.
#
AC_DEFUN([SAGE_PYTHON_PACKAGE_CHECK], [
  AC_MSG_CHECKING([if --enable-system-site-packages was used])
  AS_IF([test "${enable_system_site_packages}" = "yes"], [
    AC_MSG_RESULT(yes)

      AC_REQUIRE([SAGE_SPKG_CONFIGURE_PYTHON3])
      AC_MSG_CHECKING([if we're using the system python])
      AS_IF([test "x$sage_spkg_install_python3" = "xno"],[
        AC_MSG_RESULT([yes])

        SAGE_PKG_VERSPEC=$(sed '/^#/d' "./build/pkgs/$1/install-requires.txt")
        AC_MSG_CHECKING([for python package $1 ("${SAGE_PKG_VERSPEC}")])

        SAGE_PYTHON_RUN_IFELSE([
import pkg_resources
pkg_resources.require('${SAGE_PKG_VERSPEC}'.splitlines())
        ], [
          AC_MSG_RESULT(yes)
        ], [
          AC_MSG_RESULT(no)
  	  sage_spkg_install_$1=yes
        ])
      ], [
        dnl Not using the system python
        AC_MSG_RESULT([no])
	sage_spkg_install_$1=yes
      ])
  ], [
    dnl System site packages are disabled.
    AC_MSG_RESULT(no; skipping check)
    sage_spkg_install_$1=yes

    dnl We have to retroactively hack the --with-system-foo={no,yes,force}
    dnl mechanism here because it wasn't designed with the ability to
    dnl disable arbitrary chunks of system packages in mind. The easy cases
    dnl are "no" and "force" which require no action; "no" means we won't
    dnl suggest the package anyway, and "force" will raise an error when
    dnl the system-package check fails.
    dnl
    dnl The default of "yes" is more troubling because it is the default. To
    dnl avoid prompting users to install packages that won't be used, we want
    dnl to ignore "yes" when reporting the "hint: install these packages..."
    dnl at the end of ./configure. To accomplish that, we change "yes" to
    dnl "no" here, essentially changing the default for packages using this
    dnl macro when --enable-system-site-packages is disabled. Packages with
    dnl "no" are not suggested to the user.
    AS_IF([test "${sage_use_system_$1}" = "yes"],[sage_use_system_$1=no])
  ])
])

[dimpase]

can we please have optional args in SAGE_PYTHON_PACKAGE_CHECK ?
I.e. SAGE_PYTHON_PACKAGE_CHECK(_, [stuff_if_OK], [stuff_if_bad])
This makes is easier to chain calls, i.e. one can have skip using side effects (pseudo code):

SAGE_PYTHON_PACKAGE_CHECK(foo, [SAGE_PYTHON_RUN_IFELSE(...)], [AC_MESSAGE(oh well) `pip install foo`])

instead of

SAGE_PYTHON_PACKAGE_CHECK(foo)
AS_IF([test ugly-ugly-ugly cond], 
   [SAGE_PYTHON_RUN_IFELSE(...)], [AC_MESSAGE(oh well) `pip install foo`])

[orlitzky]

The default is sage_spkg_install_foo=no, and the goal of both tests in this case is to potentially set them to yes. So technically no "if" statement is needed:

# default is no
SAGE_PYTHON_PACKAGE_CHECK(foo)
# maybe it's yes
SAGE_PYTHON_RUN_IFELSE(..., [do nothing], [set it to yes])
# it's "no" only if both tests succeeded

The second test is wasteful but it's only slow because of the venv creation. Matthias brought that up once before. I chose to create/destroy the venv each time because the autoconf macros are typically wasteful (i.e. self-contained) like that, but I don't really care about it. (I doubt we need a venv at all when we're allowed to use system packages.)

[orlitzky]

SAGE_PYTHON_PACKAGE_CHECK(_, [stuff_if_OK], [stuff_if_bad])

I forgot to say: I'm not opposed to this, just wondering if it's necessary.

[dimpase]

The default is sage_spkg_install_foo=no, and the goal of both tests in this case is to potentially set them to yes. So technically no "if" statement is needed:

# default is no
SAGE_PYTHON_PACKAGE_CHECK(foo)
# maybe it's yes
SAGE_PYTHON_RUN_IFELSE(..., [do nothing], [set it to yes])
# it's "no" only if both tests succeeded

The second test is wasteful but it's only slow because of the venv creation.

the 2nd test is not needed at all if the 1st one gives yes. How do you check that the 2nd one does not flip the state back to no?
A hidden test?

I don't understand the preference for these hidden states being passed around. It's a recipe for bugs.

[orlitzky]

SAGE_PYTHON_RUN_IFELSE(..., [do nothing], [set it to yes])
How do you check that the 2nd one does not flip the state back to no? A hidden test?

You can never be sure, but I was hoping that the implementation of "do nothing" does not alter variables.

I don't understand the preference for these hidden states being passed around. It's a recipe for bugs.

I agree in principle, but the hidden state is already there. The current behavior of SAGE_PYTHON_PACKAGE_CHECK is to set the variable to yes or no, depending on whether or not the check passed, and every spkg-configure script makes use of that. Adding additional action-if-found and action-if-not-found arguments won't change that unless we go through and rewrite every spkg-configure script to put the sage_spkg_install_foo=yes into the action-if-not-found branch. I think that would make sense for the sake of refactoring, but that doesn't make me want to do it.

The reason I set the variable in the macro in the first place is because it worked for 100+ packages and made the spkg-configure scripts a bit shorter. Now we're probably closer to 200 python packages, and we have one outlier. I'd say it's OK to write the AS_IF([test "x$sage_spkg_install_ipython" = xno]) once for now and revisit it later if more packages need it.