Opened 20 months ago

Closed 20 months ago

Last modified 20 months ago

#29038 closed enhancement (fixed)

Python package sage_conf: Provides optional configuration information for sagelib

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.1
Component: build Keywords:
Cc: fbissey, arojas, isuruf, embray, infinity0, gh-timokau, jdemeyer, dimpase, jhpalmieri, vbraun Merged in:
Authors: Matthias Koeppe Reviewers: Dima Pasechnik, Erik Bray
Report Upstream: N/A Work issues:
Branch: af18a01 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

This ticket introduces a new Python package sage_conf.

  • sage-the-distribution will generate this Python package at ./configure time and install it at build time before starting to build/install sagelib using src/setup.py.
  • Distributions will generate and install their own sage_conf by a method of their choice before starting to build/install sagelib.

A console_script allows to query individual variable values from the shell, or output all variables in .env format:

$ local/bin/sage-config MAXIMA
/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/bin/maxima
$ local/bin/sage-config
VERSION=9.1.beta0
MAXIMA=/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/bin/maxima
SAGE_LOCAL=/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local
SAGE_ROOT=/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring

The module and the script are used as follows:

  1. It provides configuration information to sagelib at installation time (src/setup.py).

Ultimately we want to be able to install sagelib with pip. To support installation by pip from PyPI, from an URL, etc., we cannot expect to configure the build like we do now, by writing the configuration to the file src/sage-env-config. Moreover, if pip is running in --isolated mode, also no environment variables are passed. Then the only information flow is through arguments to pip build and through the installed Python packages. By installing sage_conf first, we make the configuration available to the pip install of sagelib.

  1. It provides configuration information to the docbuild, in particular about the install locations of documentation of external packages.
  1. It provides configuration information to the runtime of sagelib, making sagelib more independent from the environment variables set by src/bin/sage-env (local/bin/sage-env).
  1. By providing SAGE_LOCAL as one the configuration variables, it removes assumptions regarding install locations of sagelib relative to $SAGE_LOCAL. This enables the following:
  • Installing an experimental version of sagelib in other install locations, such as in a user site packages directory.
  • Making sagelib available in a user's venv, as in the following example:

Without this ticket:

$ sage -python -m venv --system-site-packages ~/personal-sage-venv/
$ source ~/personal-sage-venv/bin/activate
(personal-sage-venv) $ python
>>> import sage.env
>>> sage.env.SAGE_LOCAL
'/Users/mkoeppe/personal-sage-venv'                                     # wrong
>>> import sage.all
RuntimeError: You must get the file local/bin/sage-maxima.lisp

With this ticket:

>>> sage.env.SAGE_LOCAL
'/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local'
>>> import sage.all
>>> sage.all.maxima('1')
1
  • #29013/#27824: Using the system python3 via spkg-configure.m4
  1. For a broader context: see #21707 Task: Split sage-env into 5

Change History (74)

comment:1 Changed 20 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Description modified (diff)

comment:2 Changed 20 months ago by mkoeppe

  • Branch set to u/mkoeppe/python_package_sage_conf__provides_optional_configuration_information_for_sagelib

comment:3 Changed 20 months ago by mkoeppe

  • Cc fbissey arojas isuruf embray infinity0 gh-timokau jdemeyer dimpase jhpalmieri added
  • Commit set to fe5c890e66a7dd37395720b6c8e08a3837e5a74d
  • Status changed from new to needs_review

New commits:

fe5c89029038: Python package sage_conf: Provides configuration information for sagelib

comment:4 Changed 20 months ago by git

  • Commit changed from fe5c890e66a7dd37395720b6c8e08a3837e5a74d to aefd827981481f015c696328402278f2f8d30912

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

aefd827build/pkgs/sage_conf/spkg-install: Fix up path

comment:5 Changed 20 months ago by dimpase

why are you checking in files which apparently are meant to be generated from .in templates?

comment:6 Changed 20 months ago by mkoeppe

A mistake, which I will fix in a moment. Thanks!

comment:7 Changed 20 months ago by git

  • Commit changed from aefd827981481f015c696328402278f2f8d30912 to 88fd03c7784eb82eeb8cb06102ed4e93f5f1d758

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

981e864Move .gitignore to the right place, remove generated files
88fd03cAdd console_scripts entry_point sage-config

comment:8 Changed 20 months ago by mkoeppe

  • Description modified (diff)

comment:9 Changed 20 months ago by mkoeppe

  • Description modified (diff)

comment:10 Changed 20 months ago by mkoeppe

  • Description modified (diff)

comment:11 Changed 20 months ago by mkoeppe

  • Description modified (diff)

comment:12 follow-up: Changed 20 months ago by dimpase

the example with sage --python -m venv - is it going to work if Sage is using venv on its own?

comment:13 in reply to: ↑ 12 Changed 20 months ago by mkoeppe

Replying to dimpase:

the example with sage --python -m venv - is it going to work if Sage is using venv on its own?

Not without installing sage packages into the new venv. As far as I can see, one can only control whether system-packages are made available but not whether the originating venv's packages are made available. But I'd expect that this option will be invented at some point in Python.

comment:14 Changed 20 months ago by mkoeppe

  • Description modified (diff)

comment:15 follow-up: Changed 20 months ago by dimpase

is sage_conf also meant to be used while building Sage's dependencies?

comment:16 in reply to: ↑ 15 Changed 20 months ago by mkoeppe

Replying to dimpase:

is sage_conf also meant to be used while building Sage's dependencies?

No.

comment:17 Changed 20 months ago by dimpase

author ought to be either "The Sage Developers" (cf https://wiki.sagemath.org/Publications_using_SageMath?action=show&redirect=Publications_using_SAGE) or yourself...

comment:18 follow-up: Changed 20 months ago by mkoeppe

I copied the metadata from src/setup.py

comment:19 in reply to: ↑ 18 Changed 20 months ago by dimpase

Replying to mkoeppe:

I copied the metadata from src/setup.py

that line is 5.5 years old. With all respect, William has not contributed a line of code to sagemath since 2017.

Last edited 20 months ago by dimpase (previous) (diff)

comment:20 Changed 20 months ago by git

  • Commit changed from 88fd03c7784eb82eeb8cb06102ed4e93f5f1d758 to f2863760bea085d285709ef848c53e6ce17a65fa

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

f286376build/pkgs/sage_conf/src/setup.cfg.in: Change author to 'The Sage Developers'

comment:21 Changed 20 months ago by mkoeppe

  • Description modified (diff)

comment:22 follow-up: Changed 20 months ago by dimpase

I don't seem to get local/bin/sage-config - it is not on the branch?

comment:23 Changed 20 months ago by dimpase

otherwise the branch builds and passes tests on a Debian system

comment:24 in reply to: ↑ 22 Changed 20 months ago by mkoeppe

Replying to dimpase:

I don't seem to get local/bin/sage-config - it is not on the branch?

Strange. Can you post the output of "sage -f sage_conf"?

comment:25 Changed 20 months ago by git

  • Commit changed from f2863760bea085d285709ef848c53e6ce17a65fa to a4205a776e9563a9d1db72541df7fef2d7a9f8c0

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

a4205a7build/pkgs/sage_conf/src/setup.cfg.in: Use py_modules instead of packages

comment:26 Changed 20 months ago by dimpase

with the latest commit I get an error from sage -f sage_conf

make[1]: Entering directory '/home/dimpase/sage/build/make'
cd '/home/dimpase/sage' && source '/home/dimpase/sage/src/bin/sage-env' && sage-logger -p '/home/dimpase/sage/build/pkgs/sage_conf/spkg-install' '/home/dimpase/sage/logs/pkgs/sage_conf.log'
[sage_conf] /home/dimpase/sage/build/pkgs/sage_conf/spkg-install: 4: /home/dimpase/sage/build/pkgs/sage_conf/spkg-install: source: not found
[sage_conf] Error: failed to source /home/dimpase/sage/build/bin/sage-dist-helpers
[sage_conf] Is /home/dimpase/sage the correct SAGE_ROOT?
make[1]: *** [Makefile:2240: sage_conf] Error 1
make[1]: Leaving directory '/home/dimpase/sage/build/make'

real	0m0.137s
user	0m0.076s
sys	0m0.066s
***************************************************************
Error building Sage.

The following package(s) may have failed to build (not necessarily
during this run of 'make sage_conf'):

* package: sage_conf
  log file: /home/dimpase/sage/logs/pkgs/sage_conf.log
  build directory: /home/dimpase/sage/local/var/tmp/sage/build/sage_conf

comment:27 Changed 20 months ago by dimpase

simply due to Debian's /bin/sh not knowing what source is:

$ /bin/sh
$ source 
/bin/sh: 1: source: not found

comment:28 Changed 20 months ago by dimpase

I suppose the 1st line got to be

#!/usr/bin/env bash

Trying this now.

comment:29 Changed 20 months ago by dimpase

OK, with the latter fix, it completes, and also

$ ./local/bin/sage-config
VERSION=9.1.beta0
MAXIMA=/home/dimpase/sage/local/bin/maxima
SAGE_LOCAL=/home/dimpase/sage/local
SAGE_ROOT=/home/dimpase/sage

works.

\begin{lament}

An annoying thing is that sage -f sage_conf did cause ./configure to be run like 4 or 5 times, I really lost count. This is a by-product of sage -f/-i changing the configuration. See #27373.

I really think we should get rid of these snowflake sage -f/-i and do ./configure && make :-)

\end{lament}

comment:30 Changed 20 months ago by dimpase

$ sage -python -m venv --system-site-packages ~/personal-sage-venv/ also works as advertised by this ticket.

comment:31 Changed 20 months ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to needs_work
  • Work issues set to use bash, not /bin/sh

comment:32 Changed 20 months ago by git

  • Commit changed from a4205a776e9563a9d1db72541df7fef2d7a9f8c0 to 747b05a5364d1e939533648dbae6dde9fbc75363

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

747b05abuild/pkgs/sage_conf/spkg-install: Use bash, not sh

comment:33 Changed 20 months ago by mkoeppe

  • Status changed from needs_work to needs_review
  • Work issues use bash, not /bin/sh deleted

comment:34 Changed 20 months ago by dimpase

  • Status changed from needs_review to positive_review

OK.

comment:35 Changed 20 months ago by mkoeppe

Thank you!

comment:36 follow-up: Changed 20 months ago by embray

  • Status changed from positive_review to needs_work

For the argument parsing please just use argparse.

comment:37 follow-up: Changed 20 months ago by embray

I don't understand this line:

MAXIMA = "@prefix@/bin/maxima"

Why assume that's the location of maxima? It might be if using SAGE_LOCAL, but this should instead come from a path to maxima determined by configure.

comment:38 in reply to: ↑ 37 ; follow-ups: Changed 20 months ago by dimpase

Replying to embray:

I don't understand this line:

MAXIMA = "@prefix@/bin/maxima"

Why assume that's the location of maxima? It might be if using SAGE_LOCAL, but this should instead come from a path to maxima determined by configure.

As long as we don't have spkg-configure.m4 for maxima, it's OK.

comment:39 follow-ups: Changed 20 months ago by embray

The idea of making this type=script make sense to me, but that isn't used very often so it hasn't been well tested. If you look at what happens with a type=script package, it's installed like this:

# ============================= script packages ==============================
# Generate build rules for 'script' packages; this template is used to generate
# two rules in the form:
#
# <pkgname>: <dependencies>
#     $(AM_V_at)cd '$SAGE_ROOT' && \\
#         source '$SAGE_ROOT/src/bin/sage-env' && \\
#         sage-logger -p '$SAGE_ROOT/build/pkgs/<pkgname>/spkg-install' '$(SAGE_LOGS)/<pkgname>.log'

So it already sources sage-env, but not sage-dist-helpers.

Looking at the handful of other spkg-install scripts for the type=script packages, they might not work properly anyways since they don't get the "wrapper" treatment that other spkg-install scripts now get from sage-spkg (that didn't used to be the case.

I would make the splitting up of sage-env a prerequisite to this, and at a minimum ensure that sage-dist-helpers is already sourced in the build env. Then you could do away with the extra boilerplate at the top of the spkg-install for this.

comment:40 in reply to: ↑ 38 Changed 20 months ago by embray

Replying to dimpase:

Replying to embray:

I don't understand this line:

MAXIMA = "@prefix@/bin/maxima"

Why assume that's the location of maxima? It might be if using SAGE_LOCAL, but this should instead come from a path to maxima determined by configure.

As long as we don't have spkg-configure.m4 for maxima, it's OK.

Yes, I suppose, though this would have to be changed later if it were added.

comment:41 follow-up: Changed 20 months ago by embray

In the setup.cfg this should not be necessary:

+setup_requires =
+    setuptools

The standard now for specifying build-time dependencies is to add a pyproject.toml file.

comment:42 follow-ups: Changed 20 months ago by embray

Not to sound like a broken record, but I don't understand the purpose of making this a whole Python module instead of a plain text file. In some ways this makes things even worse for packagers, because now to package sage_config they have to hand-patch this Python module.

All of the data provided by this module can go in a config file, similarly to Numpy's site.cfg. It can even go directly in setup.cfg under a Sage-specific section, but probably better a separate file that doesn't need to be patched.

If you want to install the data along with the sage package it can be added to package_data. If you look at Sage's setup.py it already has one other package_data file. Adding one more is not really a problem, issues like #22655 nonwithstanding. The file can be generated by the setup.py, which is not that unusual a thing to do. If all this is to just avoid questions of how to update setup.py, I can help with that.

comment:43 Changed 20 months ago by embray

This fix (adding the MAXIMA variable) is definitely good anything else in this ticket aside, and I'd approve right away as a separate ticket:

@@ -186,6 +192,7 @@ var('MTXLIB',                        join(SAGE_SHARE, 'meataxe'))
 var('THREEJS_DIR',                   join(SAGE_SHARE, 'threejs'))
 var('SINGULARPATH',                  join(SAGE_SHARE, 'singular'))
 var('PPLPY_DOCS',                    join(SAGE_SHARE, 'doc', 'pplpy'))
+var('MAXIMA',                        'maxima')
 var('MAXIMA_FAS')
 var('SAGE_NAUTY_BINS_PREFIX', '')
 
diff --git a/src/sage/interfaces/maxima.py b/src/sage/interfaces/maxima.py
index eacbc4c..d6d37fe 100644
--- a/src/sage/interfaces/maxima.py
+++ b/src/sage/interfaces/maxima.py
@@ -472,7 +472,7 @@ import pexpect
 
 from random import randrange
 
-from sage.env import DOT_SAGE, SAGE_LOCAL
+from sage.env import DOT_SAGE, SAGE_LOCAL, MAXIMA
 from sage.misc.misc import ECL_TMP
 
 from .expect import (Expect, ExpectElement, gc_disabled)
@@ -546,7 +546,7 @@ class Maxima(MaximaAbstract, Expect):
         Expect.__init__(self,
                         name = 'maxima',
                         prompt = r'\(\%i[0-9]+\) ',
-                        command = 'maxima -p "{0}"'.format(STARTUP),
+                        command = '"{0}" -p "{1}"'.format(MAXIMA, STARTUP),
                         env = {'TMPDIR': str(ECL_TMP)},
                         script_subdirectory = script_subdirectory,
                         restart_on_ctrlc = False,

comment:44 in reply to: ↑ 42 ; follow-ups: Changed 20 months ago by dimpase

Replying to embray:

Not to sound like a broken record, but I don't understand the purpose of making this a whole Python module instead of a plain text file.

it is easier to make data available in Python if it is packaged as a Python module. Otherwise you need to read a plain text file from somewhere etc etc.

comment:45 in reply to: ↑ 44 ; follow-up: Changed 20 months ago by dimpase

Replying to dimpase:

Replying to embray:

Not to sound like a broken record, but I don't understand the purpose of making this a whole Python module instead of a plain text file.

it is easier to make data available in Python if it is packaged as a Python module. Otherwise you need to read a plain text file from somewhere etc etc.

perhaps needless to say, but sysconfig is a Python package, and your objections -- why it's not a text file -- apply to it just as well.

comment:46 in reply to: ↑ 45 Changed 20 months ago by embray

Replying to dimpase:

Replying to dimpase:

Replying to embray:

Not to sound like a broken record, but I don't understand the purpose of making this a whole Python module instead of a plain text file.

it is easier to make data available in Python if it is packaged as a Python module. Otherwise you need to read a plain text file from somewhere etc etc.

perhaps needless to say, but sysconfig is a Python package, and your objections -- why it's not a text file -- apply to it just as well.

Have you looked at the sysconfig module? None of it is generated. In fact, Python installs the Makefile it was built with, and sysconfig.py reads it. I'm not advocating that approach either, but something closer to it.

That said, having a generated Python file for this configuration is something I'd bend on. I don't think it needs to be a .py module but shrug. I added a comment on #29022 which I hope clarifies what I think this should be.

I do like that addition of a sage-config script. That could just be part of the sage package itself though; no need for an module external to sage that I can see.

comment:47 in reply to: ↑ 44 Changed 20 months ago by embray

Replying to dimpase:

Replying to embray:

Not to sound like a broken record, but I don't understand the purpose of making this a whole Python module instead of a plain text file.

it is easier to make data available in Python if it is packaged as a Python module. Otherwise you need to read a plain text file from somewhere etc etc.

# sage/env.py
...

SAGE_ENV_FILE = os.path.join(os.path.dirname(__file__), 'sage.env')
with open(SAGE_ENV_FILE) as sage_env:
    for line in sage_env:
        key, value = line.split('=', 1)
        os.environ[key] = value

...

This format has the advantage that it can also be sourced by a shell script as it's valid shell syntax.

This is just a suggestion though; the .py equivalent is almost the same except with spaces around the =.

Another version, if you require that the values are quoted (so that it's simultaneously valid shell syntax and Python syntax):

with open(SAGE_ENV_FILE) as sage_env:
    exec(sage_env.read(), os.environ)

Or, if we prefer not to override things in os.environ we can use globals() instead.

Point being, it's a few lines.

I like this because even if $SAGE_LOCAL is not "activated" (sage-env not sourced), you can run $SAGE_LOCAL/bin/python -c 'import sage' and have a working environment. The necessary environment variables can then also be passed to Sage's external dependencies, without any requirement of modifying the outer shell environment. To me, that's the primary motivation. That motivation can still be achieved even if the generated file is a .py module, which is why I'm more open to that; I'm just trying to show that it doesn't gain all that much in simplicity either.

comment:48 Changed 20 months ago by dimpase

IMHO the internal design of sysconfig is a technical detail - given Python's historical dislike of non-Python tooling, no wonder they do it their way.

comment:49 Changed 20 months ago by dimpase

globals(), exec(), etc are really measures of last resort.

And a plain text file is much more vulnerable to all sorts of problems, too, e.g. it's not checked for syntax by Python...

comment:50 in reply to: ↑ 39 Changed 20 months ago by mkoeppe

Replying to embray:

I would make the splitting up of sage-env a prerequisite to this

I have laid out a plan for splitting of sage-env in #21707. It is more complex. The present ticket is an actionable step toward it.

comment:51 in reply to: ↑ 38 Changed 20 months ago by mkoeppe

Replying to dimpase:

Replying to embray:

I don't understand this line:

MAXIMA = "@prefix@/bin/maxima"

Why assume that's the location of maxima? It might be if using SAGE_LOCAL, but this should instead come from a path to maxima determined by configure.

As long as we don't have spkg-configure.m4 for maxima, it's OK.

Yes, of course the intention is that a follow-up ticket will add spkg-configure.m4.

I picked this invocation of maxima as an illustrating example for this ticket - one of many places where the Python code assumes that externally to the Python process, an environment has been set up. In the systematic of #21707 ("Split sage-env into 5"), this example belongs to category 5.

comment:52 Changed 20 months ago by git

  • Commit changed from 747b05a5364d1e939533648dbae6dde9fbc75363 to 5f7adc62326b63292f798fb7b6f276f49b2229d3

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

5f7adc6build/pkgs/sage_conf/src/setup.cfg.in: Remove unnecessary setup_requires

comment:53 in reply to: ↑ 41 Changed 20 months ago by mkoeppe

Replying to embray:

In the setup.cfg this should not be necessary:

+setup_requires =
+    setuptools

Thanks, I've removed it.

The standard now for specifying build-time dependencies is to add a pyproject.toml file.

Our present tooling does not need that file either. I'd suggest we do pyproject.toml business after #21508.

comment:54 Changed 20 months ago by git

  • Commit changed from 5f7adc62326b63292f798fb7b6f276f49b2229d3 to 8f8d8e3976d1c891b501e64ecdc094da65cf8239

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

8f8d8e3build/pkgs/sage_conf/spkg-install: Don't source sage-env, as it is sourced already by the build/make/Makefile rule

comment:55 in reply to: ↑ 39 Changed 20 months ago by mkoeppe

Replying to embray:

The idea of making this type=script make sense to me, but that isn't used very often so it hasn't been well tested. If you look at what happens with a type=script package, [...] it already sources sage-env, but not sage-dist-helpers.

Thanks! I've simplified spkg-install accordingly.

comment:56 in reply to: ↑ 42 ; follow-up: Changed 20 months ago by mkoeppe

Replying to embray:

In some ways this makes things even worse for packagers, because now to package sage_config they have to hand-patch this Python module.

No, to the contrary. This ticket defines a functional interface to configuration: to the shell, by sage-config, to Python, by importing the module sage_conf and reading its variables.

The version of sage_conf provided in build/pkgs is just a reference implementation of the functional interface that sage-the-distribution uses.

Packagers will just be able to generate a sage_conf module in any way they like.

I have chosen this design (external to the src directory, a Python module rather than a configuration file) for the following reasons:

  • committing to a functional interface is more flexible than (1) committing to configuration by a static file written at installation time; (2) (less importantly) to a particular file format for that.
  • for example, a binary distribution may decide to ship a version of sage_conf that is more dynamic: It could discover some settings at its runtime.
  • it is not part of the src directory; in this way it is working towards the goals of (a) distributors being able to use the whole unmodified src tree; (b) pip installability of the unmodified src tree.
Last edited 20 months ago by mkoeppe (previous) (diff)

comment:57 Changed 20 months ago by git

  • Commit changed from 8f8d8e3976d1c891b501e64ecdc094da65cf8239 to 7e34340291bce50f341a047f32f018ae97af6f08

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

7e34340build/pkgs/sage_conf/src/sage_conf.py.in: Reimplement _main using argparse

comment:58 in reply to: ↑ 36 Changed 20 months ago by mkoeppe

Replying to embray:

For the argument parsing please just use argparse.

Thanks for the suggestion. Done.

comment:59 Changed 20 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:60 Changed 20 months ago by mkoeppe

  • Reviewers changed from Dima Pasechnik to Dima Pasechnik, Erik Bray

comment:61 follow-up: Changed 20 months ago by dimpase

from __future__ import print_function
+del print_function

by the way, can you explain these 2 lines

comment:62 Changed 20 months ago by git

  • Commit changed from 7e34340291bce50f341a047f32f018ae97af6f08 to af18a01e25b70406cf5ea948f550493f065282ae

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

af18a01Use sys.stdout.write instead of print

comment:63 in reply to: ↑ 61 Changed 20 months ago by mkoeppe

Replying to dimpase:

from __future__ import print_function
+del print_function

by the way, can you explain these 2 lines

Without the del print_function, sage-config would print a line print_function=_Feature((2, 6, 0, 'alpha', 2), (3, 0, 0, 'alpha', 0), 65536).

But I have simplified the module by avoiding print altogether.

comment:64 Changed 20 months ago by dimpase

  • Status changed from needs_review to positive_review

OK for me.

comment:65 Changed 20 months ago by vbraun

  • Branch changed from u/mkoeppe/python_package_sage_conf__provides_optional_configuration_information_for_sagelib to af18a01e25b70406cf5ea948f550493f065282ae
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:66 in reply to: ↑ 56 Changed 20 months ago by embray

  • Commit af18a01e25b70406cf5ea948f550493f065282ae deleted

Replying to mkoeppe:

Replying to embray:

In some ways this makes things even worse for packagers, because now to package sage_config they have to hand-patch this Python module.

No, to the contrary. This ticket defines a functional interface to configuration: to the shell, by sage-config, to Python, by importing the module sage_conf and reading its variables.

The version of sage_conf provided in build/pkgs is just a reference implementation of the functional interface that sage-the-distribution uses.

Packagers will just be able to generate a sage_conf module in any way they like.

I have chosen this design (external to the src directory, a Python module rather than a configuration file) for the following reasons:

  • committing to a functional interface is more flexible than (1) committing to configuration by a static file written at installation time; (2) (less importantly) to a particular file format for that.
  • for example, a binary distribution may decide to ship a version of sage_conf that is more dynamic: It could discover some settings at its runtime.
  • it is not part of the src directory; in this way it is working towards the goals of (a) distributors being able to use the whole unmodified src tree; (b) pip installability of the unmodified src tree.

But it's not even documented what this is supposed to provide or how it's supposed to work. This is all very confusing.

I don't really know what you mean by "functional" here. Do you mean executable? Why? Is there a specific reason for that? In any case I know of all that's needed here is at most a list of environment variables. In many cases this will just be static. If a packager has a need to generate some values for those variables programmatically they can do so however they like, but now they have to write a whole Python script that implements some undocumented interface.

comment:67 follow-up: Changed 20 months ago by dimpase

Once again, it's more Pythonic to wrap Python-needed things into Python modules, as sysconfig does, as opposed to keep stuff around in text files, which need to be read, parsed, etc etc. I guess that's what Matthias calls "functional".

Last edited 20 months ago by dimpase (previous) (diff)

comment:68 in reply to: ↑ 67 ; follow-up: Changed 20 months ago by embray

Replying to dimpase:

Once again, it's more Pythonic to wrap Python-needed things into Python modules

![citation needed]

as sysconfig does, as opposed to keep stuff around in text files, which need to be read, parsed, etc etc. I guess that's what Matthias calls "functional".

Perhaps you missed it, but as I already explained above this is exactly what sysconfig does. You are saying that sysconfig is "unpythonic".

comment:69 follow-up: Changed 20 months ago by mkoeppe

The larger plan is laid out in #21707 - Split sage-env into 5. It explains how pieces fit together. A recommended read.

comment:70 Changed 20 months ago by embray

For reference, again sysconfig, which is a hard-coded Python module which reads from a plain text-file.

Also recommended, the venv module which is actually a lot less mysterious than you guys seem to be making it out to be. The only magic involved is some support bits coded directly into the Python interpreter used for setting sys.prefix correctly (this is one of the few ways venv differs from the original virtualenv which needed some hacks to achieve this).

comment:71 in reply to: ↑ 69 Changed 20 months ago by embray

Replying to mkoeppe:

The larger plan is laid out in #21707 - Split sage-env into 5. It explains how pieces fit together. A recommended read.

Thanks, that is useful. Obviously I've participated in that ticket before, but I wasn't sure if it was the ticket tying the rest of this together. I still believe that steps 3/4/5 can be achieved by different means which merit more discussion. But I'm on board with the overall plan :)

comment:72 follow-up: Changed 20 months ago by mkoeppe

  • Cc vbraun added

No problem. I know that it is not always possible to keep up with all developments when there are more important things to do.

As you see in that plan in #21707, the present ticket on sage_conf was a step to cleaning up the sage-env mess.

It has little to do with my venv-python3 tickets (which I haven't had time to work on). (In fact, I now think that your version at #29032 may, after all, be the simpler way to go at it -- but I have to get back to the python3-spkg-configure business later.)

So there's no need to mix any "venv" discussion into the "sagelib configuration" discussion.

The "regression" caused by this ticket is one that is affecting developers who switch between new and old branches. It is fixed by just merging current develop into the old branch.

comment:73 in reply to: ↑ 68 Changed 20 months ago by dimpase

Replying to embray:

Replying to dimpase:

Once again, it's more Pythonic to wrap Python-needed things into Python modules

![citation needed]

cf. [sysconfig].

as sysconfig does, as opposed to keep stuff around in text files, which need to be read, parsed, etc etc. I guess that's what Matthias calls "functional".

Perhaps you missed it, but as I already explained above this is exactly what sysconfig does. You are saying that sysconfig is "unpythonic".

No, obviously Python needs to do some non-Pythonic things under the hood, but then the interface is Pythonic, and sysconfig is an example of such an approach. You are somehow against this approach, you say that such wrapping is too complicated, so it should not be done - if you were Python BDFL, you'd not have allowed sysconfig in.

Last edited 20 months ago by dimpase (previous) (diff)

comment:74 in reply to: ↑ 72 Changed 20 months ago by mkoeppe

Replying to mkoeppe:

The "regression" caused by this ticket is one that is affecting developers who switch between new and old branches. It is fixed by just merging current develop into the old branch.

For tickets where merging the current develop is not easy, I have prepared #29120 - One-line fix for "./configure is too sensitive to stray files/subdirectories".

Note: See TracTickets for help on using tickets.