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

src/bin/sage-env: Make sure $SAGE_VENV/bin is at the beginning of the PATH #30013

Closed
mkoeppe opened this issue Jun 29, 2020 · 33 comments
Closed

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Jun 29, 2020

Follow-up from #25486 (Discover SAGE_SCRIPTS_DIR to make $SAGE_LOCAL/bin/sage work from any directory, in an environment without SAGE_* variables) and #22731.

We make sure that $SAGE_VENV/bin (from #22731) appears at the beginning of the PATH. For virtual environments such as those set up by tox (#29950), this may be different from and should take precedence over $SAGE_LOCAL/bin. This will ensure that

  • the correct copy of auxiliary scripts such as sage-eval is invoked by sage;
  • the correct copy of python3 is run.

To determine SAGE_VENV:

  • If src/bin/sage is invoked directly out of the source tree, it will run a new non-installed configure-generated script sage-src-env-config
  • An installed sage script installed by setup.py in SAGE_LOCAL or in a venv will run a Python script sage-venv-config instead.

Depends on #29852

CC: @orlitzky @jhpalmieri

Component: scripts

Keywords: sd111

Author: Matthias Koeppe

Branch: 0140f84

Reviewer: Tobias Diez, Dima Pasechnik

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

@mkoeppe mkoeppe added this to the sage-9.2 milestone Jun 29, 2020
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 29, 2020

Dependencies: #25486

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 30, 2020

Changed dependencies from #25486 to #25486, #29951

@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Oct 24, 2020
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 11, 2020

Changed dependencies from #25486, #29951 to #29951, #22731

@mkoeppe mkoeppe changed the title src/bin/sage-env: Make sure SAGE_SCRIPTS_DIR is at the beginning of the PATH src/bin/sage-env: Make sure $SAGE_VENV/bin is at the beginning of the PATH Nov 11, 2020
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 11, 2020

Changed dependencies from #29951, #22731 to #29951, #22731, #30888

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 13, 2020

Changed dependencies from #29951, #22731, #30888 to #29951, #22731, #29852

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 13, 2020

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 13, 2020

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

842995fsrc/bin/sage-venv-config: Add comment

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 13, 2020

Commit: 842995f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 13, 2020

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

1a518c0src/bin/sage: Set SAGE_VENV from sage-venv-config

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 13, 2020

Changed commit from 842995f to 1a518c0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 13, 2020

Changed commit from 1a518c0 to d372ecf

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 13, 2020

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

d372ecfsrc/bin/sage-env: Put $SAGE_VENV/bin before $SAGE_LOCAL/bin in PATH

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 13, 2020

Changed commit from d372ecf to 0140f84

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 13, 2020

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

0140f84src/bin/sage-src-env-config.in: New, sourced in src/bin/sage

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 13, 2020

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 13, 2020

comment:15

Using the new script sage-venv-config is probably overkill.
If #30888 resolves the symlink of $0, we could just infer the SAGE_VENV from its directory.

@mkoeppe

This comment has been minimized.

@tobiasdiez
Copy link
Contributor

comment:17

Replying to @mkoeppe:

Using the new script sage-venv-config is probably overkill.
If #30888 resolves the symlink of $0, we could just infer the SAGE_VENV from its directory.

Why do you actually need go through the trouble of determining SAGE_VENV in some shell scripts? Isn't it enough to use SAGE_VENV = sys.prefix (which is already done in env.py)?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 13, 2020

comment:18

Replying to @tobiasdiez:

Replying to @mkoeppe:

Using the new script sage-venv-config is probably overkill.
If #30888 resolves the symlink of $0, we could just infer the SAGE_VENV from its directory.

Why do you actually need go through the trouble of determining SAGE_VENV in some shell scripts? Isn't it enough to use SAGE_VENV = sys.prefix (which is already done in env.py)?

Yes, for all Python scripts, sys.prefix does the right thing. But sage is a shell script (changing that would be outside of the scope of this ticket) and so we have to go through some extra trouble to determine this value in order to set PATH correctly.

@dimpase
Copy link
Member

dimpase commented Nov 20, 2020

Reviewer: Tobias Diez, Dima Pasechnik

@dimpase
Copy link
Member

dimpase commented Nov 20, 2020

comment:19

lgtm

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 20, 2020

comment:20

Thanks!

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 6, 2020

Changed dependencies from #29951, #22731, #29852 to #29852

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 11, 2020

Changed keywords from none to sd111

@vbraun
Copy link
Member

vbraun commented Dec 13, 2020

@antonio-rojas
Copy link
Contributor

Changed commit from 0140f84 to none

@antonio-rojas
Copy link
Contributor

comment:24

This breaks if sage_conf is not installed, because VERSION is defined there

Traceback (most recent call last):
  File "/usr/bin/sage-venv-config", line 29, in <module>
    _main()
  File "/usr/bin/sage-venv-config", line 17, in _main
    version='%(prog)s ' + VERSION)
NameError: name 'VERSION' is not defined

If this is really intended and sage_conf is now mandatory, then ImportError from sage_conf shouldn't be trapped. But I hope it can still be possible to support sage_conf-less systems.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 15, 2020

comment:25

sage_conf is still intended to be optional - this is a regression, patches welcome

@antonio-rojas
Copy link
Contributor

comment:26

OK, glad to hear - so what should we do if there's no sage_conf? don't add the --versionargument? get the version from somewhere else (where)?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 16, 2020

comment:27

I have created #31058 for this

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

5 participants