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

spkg-configure.m4 for python3 with SAGE_LOCAL as a venv for system pythons #29032

Closed
embray opened this issue Jan 17, 2020 · 16 comments
Closed

Comments

@embray
Copy link
Contributor

embray commented Jan 17, 2020

This is an experimental alternative proposal to #27824.

It isn't yet as "complete" as #27824 as it lacks some of the fixes (e.g. the patches to Pillow) which I didn't need for my tests, but which may still be needed on other platforms (TBD).


The spkg-configure.m4 itself is taken almost directly from #27824, as most of it was great, except for a couple minor changes:

  • No temporary "config_venv" to test the system Python's packages; instead just run the system Python (if detected) with the -S flag.

  • config_setup.py and config_build are renamed conftest.py and conftest.dir respectively since autoconf will automatically clean up files and directories beginning with "conftest"; see https://www.gnu.org/software/autoconf/manual/autoconf-2.60/autoconf.html#Guidelines

  • AC_SUBST([SAGE_SYSTEM_PYTHON]) instead of AC_SUBST([PYTHON_FOR_VENV])


The changes to build/make/deps are also inspired by #27824 but are simpler in my opinion: Instead of creating a virtualenv nested inside SAGE_LOCAL, SAGE_LOCAL itself is just treated as a virtualenv, and we install the necessary files for the Python interpreter to treat it as such, which keeps things simpler, as SAGE_LOCAL + source sage-env already fills the same purpose as a virtualenv.


The changes to src/sage/env.py are reworked just a little bit (in particular, the version in #27824 was buggy on Cygwin, as it would privilege system DLLs over DLLs in SAGE_LOCAL).

This was tested on Ubuntu 18.04, and make ptestlong passes from a clean build with the system Python detected. No guarantees about OSX or other platforms.

Depends on #29357

Component: build: configure

Author: Erik Bray

Branch/Commit: u/mkoeppe/build/configure/python3 @ 99b032e

Reviewer: Dima Pasechnik

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

@embray embray added this to the sage-9.1 milestone Jan 17, 2020
@embray

This comment has been minimized.

@embray
Copy link
Contributor Author

embray commented Jan 17, 2020

comment:2

See also #29033 which expands on this to add support for a system Python 3.6.

@mkoeppe
Copy link
Member

mkoeppe commented Jan 17, 2020

comment:4

Thanks for working of this.
I have started to merge a few things from this branch.

One concern that I have is that in this approach, there is a $SAGE_LOCAL/bin/activate which signals to users that it would "activate" $SAGE_LOCAL. But it doesn't. It only activates the venv.

@mkoeppe
Copy link
Member

mkoeppe commented Jan 17, 2020

comment:5

A comment regarding the variable SAGE_SYSTEM_PYTHON: I think this is not specific enough because build/bin/sage-system-python means something different from that.

@embray
Copy link
Contributor Author

embray commented Jan 22, 2020

comment:6

Replying to @mkoeppe:

Thanks for working of this.
I have started to merge a few things from this branch.

One concern that I have is that in this approach, there is a $SAGE_LOCAL/bin/activate which signals to users that it would "activate" $SAGE_LOCAL. But it doesn't. It only activates the venv.

Are you sure? I'll double check, but with this branch as written it should not install any bin/activate scripts.

@embray
Copy link
Contributor Author

embray commented Jan 22, 2020

comment:7

Replying to @mkoeppe:

A comment regarding the variable SAGE_SYSTEM_PYTHON: I think this is not specific enough because build/bin/sage-system-python means something different from that.

Right, that's a bit confusing. I wasn't totally sure how I felt about that variable name either, but I couldn't think of something better. I think it does make some sense in that just as build/bin/sage-system-python runs the system Python, SAGE_SYSTEM_PYTHON points to the path to the system Python if it is being used to run Sage. But I'm open to other ideas here.

@embray
Copy link
Contributor Author

embray commented Jan 22, 2020

comment:8

Replying to @embray:

Replying to @mkoeppe:

Thanks for working of this.
I have started to merge a few things from this branch.

One concern that I have is that in this approach, there is a $SAGE_LOCAL/bin/activate which signals to users that it would "activate" $SAGE_LOCAL. But it doesn't. It only activates the venv.

Are you sure? I'll double check, but with this branch as written it should not install any bin/activate scripts.

I confirmed that as expected it does not install an activate script. Is it possible you had that hanging around from something else?

@mkoeppe
Copy link
Member

mkoeppe commented Mar 18, 2020

Changed dependencies from #29002 to #29357

@mkoeppe
Copy link
Member

mkoeppe commented Mar 18, 2020

@mkoeppe
Copy link
Member

mkoeppe commented Mar 18, 2020

Changed commit from b6b9c3a to 8ce6517

@mkoeppe
Copy link
Member

mkoeppe commented Mar 18, 2020

comment:11

Rebased on top of #29357 (and 9.1.beta7)


New commits:

80b3933Re-configure if src/bin/sage-env-config.in changes
f1b3424Also reconfigure if build/bin/sage-build-env-config.in or build/pkgs/sage_conf/src/*.in change
8ce6517Add spkg-configure.m4 for Python 3 to detect and use a system Python 3.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 19, 2020

Changed commit from 8ce6517 to 99b032e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 19, 2020

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

f5f493fRe-configure if src/bin/sage-env-config.in changes
fe6aa38Also reconfigure if build/bin/sage-build-env-config.in or build/pkgs/sage_conf/src/*.in change
99b032eAdd spkg-configure.m4 for Python 3 to detect and use a system Python 3.

@mkoeppe
Copy link
Member

mkoeppe commented Mar 19, 2020

comment:13

Rebased again on top of #29357 (and 9.1.beta8)

@mkoeppe
Copy link
Member

mkoeppe commented Mar 19, 2020

comment:14

@embray
I have adopted the approach of using venv.EnvBuilder on SAGE_LOCAL in #27824. Thanks for your work on it.

Could you elaborate on what the environment variable VIRTUAL_ENV is needed for?

The present ticket can be closed as a duplicate.

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

dimpase commented Mar 30, 2020

Reviewer: Dima Pasechnik

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