Opened 2 years ago

Closed 19 months ago

Last modified 19 months ago

#30013 closed enhancement (fixed)

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

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.3
Component: scripts Keywords: sd111
Cc: mjo, jhpalmieri Merged in:
Authors: Matthias Koeppe Reviewers: Tobias Diez, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 0140f84 (Commits, GitHub, GitLab) Commit:
Dependencies: #29852 Stopgaps:

Status badges

Description (last modified by mkoeppe)

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.

Change History (27)

comment:1 Changed 2 years ago by mkoeppe

  • Dependencies set to #25486

comment:2 Changed 2 years ago by mkoeppe

  • Dependencies changed from #25486 to #25486, #29951

comment:3 Changed 21 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:4 Changed 20 months ago by mkoeppe

  • Dependencies changed from #25486, #29951 to #29951, #22731
  • Description modified (diff)
  • Summary changed from src/bin/sage-env: Make sure SAGE_SCRIPTS_DIR is at the beginning of the PATH to src/bin/sage-env: Make sure $SAGE_VENV/bin is at the beginning of the PATH

comment:5 Changed 20 months ago by mkoeppe

  • Description modified (diff)

comment:6 Changed 20 months ago by mkoeppe

  • Dependencies changed from #29951, #22731 to #29951, #22731, #30888

comment:7 Changed 20 months ago by mkoeppe

  • Description modified (diff)

comment:8 Changed 20 months ago by mkoeppe

  • Cc mjo added

comment:9 Changed 20 months ago by mkoeppe

  • Dependencies changed from #29951, #22731, #30888 to #29951, #22731, #29852

comment:10 Changed 20 months ago by mkoeppe

  • Branch set to u/mkoeppe/src_bin_sage_env__make_sure__sage_venv_bin_is_at_the_beginning_of_the_path

comment:11 Changed 20 months ago by git

  • Commit set to 842995f51bf527c86cb346f31a35741400893620

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

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

comment:12 Changed 20 months ago by git

  • Commit changed from 842995f51bf527c86cb346f31a35741400893620 to 1a518c0cd17cfd81ad05492ee4b932d0fd4a93a3

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

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

comment:13 Changed 20 months ago by git

  • Commit changed from 1a518c0cd17cfd81ad05492ee4b932d0fd4a93a3 to d372ecf8623695519c8471e3232f50355e8cbc15

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

comment:14 Changed 20 months ago by git

  • Commit changed from d372ecf8623695519c8471e3232f50355e8cbc15 to 0140f8429e166081798cb678dcc273c3ad769ec6

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

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

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

  • Authors set to Matthias Koeppe
  • Status changed from new to needs_review

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.

comment:16 Changed 20 months ago by mkoeppe

  • Cc jhpalmieri added
  • Description modified (diff)

comment:17 in reply to: ↑ 15 ; follow-up: Changed 20 months ago by gh-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)?

comment:18 in reply to: ↑ 17 Changed 20 months ago by mkoeppe

Replying to gh-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.

comment:19 Changed 20 months ago by dimpase

  • Reviewers set to Tobias Diez, Dima Pasechnik
  • Status changed from needs_review to positive_review

lgtm

comment:20 Changed 20 months ago by mkoeppe

Thanks!

comment:21 Changed 19 months ago by mkoeppe

  • Dependencies changed from #29951, #22731, #29852 to #29852

comment:22 Changed 19 months ago by mkoeppe

  • Keywords sd111 added

comment:23 Changed 19 months ago by vbraun

  • Branch changed from u/mkoeppe/src_bin_sage_env__make_sure__sage_venv_bin_is_at_the_beginning_of_the_path to 0140f8429e166081798cb678dcc273c3ad769ec6
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:24 Changed 19 months ago by arojas

  • Commit 0140f8429e166081798cb678dcc273c3ad769ec6 deleted

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.

comment:25 Changed 19 months ago by mkoeppe

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

comment:26 Changed 19 months ago by arojas

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)?

comment:27 Changed 19 months ago by mkoeppe

I have created #31058 for this

Note: See TracTickets for help on using tickets.