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

Make sagelib installation directory more flexible by creating a module src/sage/env_config.py from src/sage/env_config.py.in, defining variables for use in sage.env #29022

Closed
mkoeppe opened this issue Jan 15, 2020 · 53 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Jan 15, 2020

This is a follow-up from #28225 - "Allow sage to run in the absence of sage-env". The present ticket also allows $SAGE_LOCAL/bin/python to run and import sage.all in the absence of the shell script sage-env. (Getting a fully functional sage.all is future work.)

This ticket also makes sagelib more independent from the environment variables set by src/bin/sage-env (local/bin/sage-env), and to remove assumptions regarding install locations of sagelib relative to $SAGE_LOCAL.

  • To support users who want to install an experimental version of sagelib in other install locations, such as in a user site packages directory.

  • For 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
    
  • To support Add configure option --with-sage-venv=SAGE_VENV to create venv there instead of in SAGE_LOCAL #29013/spkg-configure.m4 for python3 #27824: spkg-configure.m4 for python3

  • To make it possible for distributions to use a stock src/sage/env.py (providing a custom src/sage/env_conf.py).

In a first step, we just set SAGE_LOCAL and MAXIMA (to the location of the maxima binary), but also SAGE_ROOT could be added (for developer conveniences such as sage.misc.edit_module; see also #25486); and later packages' spkg-configure might be setting MATHJAX_DIR etc.

In a follow-up ticket, src/sage/env_config.py would actually be generated by src/setup.py, not configure.

Ticket #29038 provides an alternative implementation that uses an standalone installed Python module sage_conf, rather than writing into src/sage

Related:

CC: @kiwifb @antonio-rojas @isuruf @embray @infinity0 @timokau @jdemeyer @dimpase @jhpalmieri

Component: build

Author: Matthias Koeppe

Branch/Commit: u/mkoeppe/create_module_src_sage_env_config_py_from_src_sage_env_config_py_in__defining_variables_for_use_in_sage_env @ 7a43544

Reviewer: Dima Pasechnik, Erik Bray

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

@mkoeppe mkoeppe added this to the sage-9.1 milestone Jan 15, 2020
@mkoeppe mkoeppe changed the title src/setup.py: Create module sage.env_config, which defines variables for use in sage.env Create module src/sage/env_config.py from src/sage/env_config.py.in, defining variables for use in sage.env Jan 15, 2020
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 15, 2020

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 15, 2020

Commit: bba08ec

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 15, 2020

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 15, 2020

New commits:

bba08ecCreate module src/sage/env_config.py from src/sage/env_config.py.in, defining SAGE_LOCAL

@kiwifb
Copy link
Member

kiwifb commented Jan 16, 2020

comment:5

Interesting concept. But your branch won't achieve much since SAGE_LOCAL is not None. In fact the problem of the value of SAGE_LOCAL is solved in most case by the current setting of os.path.abspath(sys.prefix). Did you mean to set SAGE_ROOT?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 16, 2020

comment:6

Replying to @kiwifb:

Interesting concept. But your branch won't achieve much since SAGE_LOCAL is not None. In fact the problem of the value of SAGE_LOCAL is solved in most case by the current setting of os.path.abspath(sys.prefix).

  1. With Add configure option --with-sage-venv=SAGE_VENV to create venv there instead of in SAGE_LOCAL #29013, os.path.abspath(sys.prefix) is the location of the venv, and not equal to SAGE_LOCAL.

  2. If one starts Sage's Python directly, without going through sage-env, then the environment variable SAGE_LOCAL is not set; and in this case it is retrieved from env_config.py.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 16, 2020

comment:7

But yes, let's add SAGE_ROOT to it.

@kiwifb
Copy link
Member

kiwifb commented Jan 16, 2020

comment:8

I misunderstood the way the code works in var. I thought your value from env_config.py wouldn't be used.
For your points

  1. OK - I get it, that's a problem

  2. I am perfectly happy with it being set as os.path.abspath(sys.prefix) if sage-env is not loaded as it is now. But your alternative is acceptable and of course solves your issue at (1).

I removed sage-env completely from sage-on-gentoo a number of releases ago and it does work nicely with very little patching (I upstreamed most of it). There is only one place now where I have to replace a call to SAGE_ROOT at runtime (in sage.misc.copying). I'd say sage works at 99.99% out of the box without needing sage-env, in particular you should be able to call it directly from python.

However

  1. your venv work may introduce a few disparities

  2. being able to override some default from a standardised configuration file instead of patching for your distro values is very appealing.

I don't particularly need SAGE_ROOT to be set (in fact None suits me just fine since it doesn't make much sense on a distro), it was just my misconception on what you were doing.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 16, 2020

comment:9

Replying to @kiwifb:

I don't particularly need SAGE_ROOT to be set (in fact None suits me just fine since it doesn't make much sense on a distro), it was just my misconception on what you were doing.

Yes, I agree that SAGE_ROOT is not very important, but it's still used for developer convenience functions like those in sage.misc.edit_module.

@kiwifb
Copy link
Member

kiwifb commented Jan 16, 2020

comment:10

Replying to @mkoeppe:

Replying to @kiwifb:

I don't particularly need SAGE_ROOT to be set (in fact None suits me just fine since it doesn't make much sense on a distro), it was just my misconception on what you were doing.

Yes, I agree that SAGE_ROOT is not very important, but it's still used for developer convenience functions like those in sage.misc.edit_module.

SAGE_ROOT is mentioned in the documentation string for edit_devel but is otherwise absent. The bit in documentation should have been amended at the same time as c352b1d#diff-141c6e649507c15775d157f417db1e26 I guess.

Almost all of the SAGE_ROOT stuff left pertains to packaging I think.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 16, 2020

comment:11

Note SAGE_SRC is defined through SAGE_ROOT.

@kiwifb
Copy link
Member

kiwifb commented Jan 16, 2020

comment:12

Replying to @mkoeppe:

Note SAGE_SRC is defined through SAGE_ROOT.

var('SAGE_SRC',            join(SAGE_ROOT, 'src'), SAGE_LIB)

I made sure of that :) - so if SAGE_ROOT is None, SAGE_SRC is set to SAGE_LIB. I guess you may prefer the SAGE_ROOT based value. But on a distro it will give me a read only view of the code (which is the best you should hope for if you install stuff with your distro package manager).

@embray
Copy link
Contributor

embray commented Jan 16, 2020

comment:13

I don't see why it needs to explicitly be a Python module. I think my proposal in #22652 is simpler, because the same file could be used both for configuring the shell environment, and can be read by the Python code.

To me this just seems like extra cruft.

@embray
Copy link
Contributor

embray commented Jan 16, 2020

comment:14

Replying to @mkoeppe:

Replying to @kiwifb:

Interesting concept. But your branch won't achieve much since SAGE_LOCAL is not None. In fact the problem of the value of SAGE_LOCAL is solved in most case by the current setting of os.path.abspath(sys.prefix).

  1. With Add configure option --with-sage-venv=SAGE_VENV to create venv there instead of in SAGE_LOCAL #29013, os.path.abspath(sys.prefix) is the location of the venv, and not equal to SAGE_LOCAL.

sys.prefix may not be, but even in a venv sysconfig.get_config_var('prefix') will always give you the original install prefix for Python, which when using Sage's Python will be SAGE_LOCAL.

@embray
Copy link
Contributor

embray commented Jan 16, 2020

comment:15

Replying to @embray:

I don't see why it needs to explicitly be a Python module. I think my proposal in #22652 is simpler, because the same file could be used both for configuring the shell environment, and can be read by the Python code.

To me this just seems like extra cruft.

For example, a file like this could be generated at build time by running something like:

$ (source src/bin/sage-env-config && env | grep '^SAGE_' | sort)
SAGE_ARB_LIBRARY=arb
SAGE_CONFIGURE_FLINT=--with-flint=/home/embray/src/sagemath/sage/local
SAGE_CONFIGURE_GMP=--with-gmp=/home/embray/src/sagemath/sage/local
SAGE_CONFIGURE_MPC=--with-mpc=/home/embray/src/sagemath/sage/local
SAGE_CONFIGURE_MPFR=--with-mpfr=/home/embray/src/sagemath/sage/local
SAGE_CONFIGURE_NTL=--with-ntl=/home/embray/src/sagemath/sage/local
SAGE_CONFIGURE_PARI=--with-pari=/home/embray/src/sagemath/sage/local
SAGE_CRTI_DIR=
SAGE_FLINT_PREFIX=/home/embray/src/sagemath/sage/local
SAGE_FREETYPE_PREFIX=
SAGE_GLPK_PREFIX=/home/embray/src/sagemath/sage/local
SAGE_GMP_INCLUDE=/home/embray/src/sagemath/sage/local/include
SAGE_GMP_PREFIX=/home/embray/src/sagemath/sage/local
SAGE_LOCAL=/home/embray/src/sagemath/sage/local
SAGE_MPC_PREFIX=/home/embray/src/sagemath/sage/local
SAGE_MPFR_PREFIX=/home/embray/src/sagemath/sage/local
SAGE_NTL_PREFIX=/home/embray/src/sagemath/sage/local
SAGE_PARI_CFG=/home/embray/src/sagemath/sage/local/lib/pari/pari.cfg
SAGE_PARI_PREFIX=/home/embray/src/sagemath/sage/local
SAGE_PKG_CONFIG_PATH=
SAGE_PYTHON_VERSION=2

It could be output to a .py module that's (optionally) imported by sage.env, or just a ".env" file (a common extension being used in recent years for a file like this that specifies some environment variables for a project). Distros, if they wish, could write their own version of this file without patching.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 16, 2020

comment:16

Replying to @embray:

Replying to @mkoeppe:

Replying to @kiwifb:

Interesting concept. But your branch won't achieve much since SAGE_LOCAL is not None. In fact the problem of the value of SAGE_LOCAL is solved in most case by the current setting of os.path.abspath(sys.prefix).

  1. With Add configure option --with-sage-venv=SAGE_VENV to create venv there instead of in SAGE_LOCAL #29013, os.path.abspath(sys.prefix) is the location of the venv, and not equal to SAGE_LOCAL.

sys.prefix may not be, but even in a venv sysconfig.get_config_var('prefix') will always give you the original install prefix for Python, which when using Sage's Python will be SAGE_LOCAL.

Thanks for this useful info. The purpose of this ticket, however, is to get rid of assumptions like that, to enable use of system python, see #27824.

@embray
Copy link
Contributor

embray commented Jan 16, 2020

comment:17

Yeah, I see what you're saying. Long term I think it would be great if we could get rid of "SAGE_LOCAL" altogether, though I'm not exactly sure what the alternative looks like either...

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 17, 2020

comment:19

Needs review.

@embray
Copy link
Contributor

embray commented Jan 17, 2020

comment:20

Sorry, I'm just not convinced this is useful. At least not yet. It's not needed for setting SAGE_LOCAL. I do agree something like this might be useful for setting other paths related to Sage's dependencies, but I don't think that's necessarily something that should be output by a configure script either, but rather should be loaded from a config file that downstream packagers can modify without patching if need be.

To expand on that a bit, the sage-the-distribution configure script is not generally run by packagers. Instead, this should be handled at the level of the sagelib package itself.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 17, 2020

comment:21

Replying to @embray:

I don't think that's necessarily something that should be output by a configure script either, but rather should be loaded from a config file that downstream packagers can modify without patching if need be.

Sage-the-distribution generates the file using configure script. Downstream packagers would generate the file by any means that they prefer.

@embray
Copy link
Contributor

embray commented Jan 17, 2020

comment:22

Replying to @mkoeppe:

Replying to @embray:

I don't think that's necessarily something that should be output by a configure script either, but rather should be loaded from a config file that downstream packagers can modify without patching if need be.

Sage-the-distribution generates the file using configure script. Downstream packagers would generate the file by any means that they prefer.

If that's the case, I don't think env_config.py.in should even be directly in the sage package. But I also think this file should not go directly into the sage package at all. It would be better if it were external to the sage package's source tree; but it could still be generated and installed along with the package, but that would be a step handled by setup.py. I also still fail to see the point of making it an actual Python module.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 18, 2020

comment:39

Replying to @embray:

Sage-the-distribution generates the file using configure script. Downstream packagers would generate the file by any means that they prefer.

If that's the case, I don't think env_config.py.in should even be directly in the sage package. But I also think this file should not go directly into the sage package at all. It would be better if it were external to the sage package's source tree; but it could still be generated and installed along with the package

In #29038 I now do exactly that. Thanks for the input.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 19, 2020

comment:40

This ticket is superseded by #29038 - Python package sage_conf: Provides optional configuration information for sagelib.

@mkoeppe mkoeppe removed this from the sage-9.1 milestone Jan 19, 2020
@dimpase
Copy link
Member

dimpase commented Jan 19, 2020

Reviewer: Dima Pasechnik, Erik Bray

@embray
Copy link
Contributor

embray commented Jan 22, 2020

comment:42

Replying to @mkoeppe:

Replying to @embray:

Sage-the-distribution generates the file using configure script. Downstream packagers would generate the file by any means that they prefer.

If that's the case, I don't think env_config.py.in should even be directly in the sage package. But I also think this file should not go directly into the sage package at all. It would be better if it were external to the sage package's source tree; but it could still be generated and installed along with the package

In #29038 I now do exactly that. Thanks for the input.

I'm sorry, what you did in that ticket is not what I meant at all, and that's my fault for not being clearer and more explicit. What I had in mind here was this:

  • I was unclear when I wrote "external to the sage package's source tree". What I meant was (relative to the git repository), still in src/, but not in src/sage.

  • These settings (whether written by sage-the-distribution's configure script, or by hand by a packager) would be written by src/setup.py into a file that is installed in the sage package.

An analogy I would use is numpy.__config__. If you look at it, and Numpy's setup.py, you can see that it's generated and installed inside the numpy package. This is close to what you are trying to do, but just a different approach to how the file is installed.

I'm still -1 on it being a .py module, but if you feel strongly about that I'd take it as long as we can do the rest more like Numpy does.

@dimpase
Copy link
Member

dimpase commented Jan 22, 2020

comment:43

numpy's configuration/setup is IMHO a good example of how NOT to do such things.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 22, 2020

comment:44

Replying to @embray:

I'd take it as long as we can do the rest more like Numpy does.

Numpy's solution, expecting a configuration file in its to-be-installed sources, is outdated (and, by the way, untested by upstream) because it is incompatible with pip-installing from a source other than a local directory.

Let's continue the discussion in #29038.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 25, 2020

comment:45

This ticket can now be closed because superseded by #29038.

@embray
Copy link
Contributor

embray commented Jan 31, 2020

comment:48

Replying to @mkoeppe:

Replying to @embray:

I'd take it as long as we can do the rest more like Numpy does.

Numpy's solution, expecting a configuration file in its to-be-installed sources, is outdated (and, by the way, untested by upstream) because it is incompatible with pip-installing from a source other than a local directory.

Let's continue the discussion in #29038.

I dont think so; you could also pass the configuration by other means but it's just an example. I don't see how #29038 makes this any better. Now you have to have a whole external Python module to do the same thing.

@dimpase
Copy link
Member

dimpase commented Jan 31, 2020

comment:49

wrapping up configuration into a Python package is more Pythonic than "other means". "A whole external Python" module is way cleaner than a bunch of text files (maybe sourceable from shell, maybe not) for a Python library.

@kiwifb
Copy link
Member

kiwifb commented Jan 31, 2020

comment:50

I need to do a write up about this. I guess I exchanged a bigger patch for a smaller one and an extra file. The issue with your python module is that it is not a proper package. I don't have a tarball for the source apart from sage's own source and it is not versioned. So as a distro packager I am looking at this as extra work if I want to package it as a separate python module because I have to do the packaging and versioning myself.

So right now I have a patch to look for sage.sage_conf in env.py, and I am putting sage_conf.py next to env.py. There may be something wrong with putting sage_cong.py.in in the source themselves. But replacing it by an half assed package is hardly ideal.

@dimpase
Copy link
Member

dimpase commented Jan 31, 2020

comment:51

Replying to @kiwifb:

I need to do a write up about this. I guess I exchanged a bigger patch for a smaller one and an extra file. The issue with your python module is that it is not a proper package. I don't have a tarball for the source apart from sage's own source and it is not versioned. So as a distro packager I am looking at this as extra work if I want to package it as a separate python module because I have to do the packaging and versioning myself.

We can make ./bootstrap to generate a tarball of it in upstream/ just as it's done
for configure package.

Will it help?

@kiwifb
Copy link
Member

kiwifb commented Jan 31, 2020

comment:52

It'd possibly be an improvement if it is properly versioned. But it would still need to be pre-processed before it being usable. You are running away from the complexity by heaping a bit more :)

I can adapt with most of the stuff that you throw at me. I have been doing that for about 12 years now. But I am fairly much in agreement with Erik that a different design should have been chosen altogether. In fact it's been years that I have been expecting sage to get a setup.cfg - at the very least for optional packages (the new design for coinor/gurobi/cplex rocks by the way).

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 31, 2020

comment:53

Replying to @kiwifb:

I need to do a write up about this. I guess I exchanged a bigger patch for a smaller one and an extra file. The issue with your python module is that it is not a proper package. I don't have a tarball for the source apart from sage's own source and it is not versioned. So as a distro packager I am looking at this as extra work if I want to package it as a separate python module because I have to do the packaging and versioning myself.

So right now I have a patch to look for sage.sage_conf in env.py, and I am putting sage_conf.py next to env.py. There may be something wrong with putting sage_cong.py.in in the source themselves. But replacing it by an half assed package is hardly ideal.

Are you commenting on the right ticket? #29038 was merged, not this one.

@kiwifb
Copy link
Member

kiwifb commented Jan 31, 2020

comment:54

You are right I should have commented on the other ticket. I just took the conversation on this ticket when I found it first in my inbox this morning. That being said I have been thinking of doing a comment for a week so I just took the opportunity before postponing it and never writing it. Of course the order I read things in my inbox in the morning is also a big influence in this case. But if I had waited to read all my inbox before thinking "I should put my comment there" I may not have written it at all :(

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 31, 2020

comment:55

I recommend to read #21707. It gives the high-level overview.

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

No branches or pull requests

4 participants