Sagelib's scripts in src/bin should not use build/bin/sage-system-python; remove sage-pypkg-location
mkoeppe opened this issue · 37 comments
In previous tickets we have moved sage-the-distribution scripts out of src/bin.
Some scripts that are (correctly) located in src/bin use sage-system-python, which is in build/bin and not installed and not available in distributions.
src/bin/sage-coverage:1:#!/usr/bin/env sage-system-python
src/bin/sage-pypkg-location:1:#!/usr/bin/env sage-system-python
src/bin/sage-coverageall:1:#!/usr/bin/env sage-system-python
src/bin/sage-num-threads.py:1:#!/usr/bin/env sage-system-python
We change sage-coverage* to just use python3. If Sage's venv is in the front of the PATH, this is Sage's python. But these scripts work with "any python3", it does not have to be the one in which sage is installed.
We remove src/bin/sage-pypkg-location, which is undocumented and unused.
We move src/bin/sage-num-threads.py to build/bin/sage-build-num-threads (where it keeps using sage-system-python), leaving a much simplified version src/bin/sage-num-threads.py (which is using python3) behind.
CC: @dimpase @jhpalmieri @kiwifb @orlitzky @vbraun
Component: scripts
Author: Matthias Koeppe
Branch/Commit: u/mkoeppe/sagelib_s_scripts_in_src_bin_should_not_use_build_bin_sage_system_python__remove_sage_pypkg_location @ a4e757e
Reviewer: Dima Pasechnik, Sébastien Labbé
Issue created by migration from https://trac.sagemath.org/ticket/29355
Description changed:
---
+++
@@ -3,5 +3,18 @@
This will require changes to some scripts in `src/bin`:
```
-src/bin/sage-pkg
+This directory contains executables needed during building sage-the-distribution only. It should only be added to `PATH` by `sage-build-env` (see #29052 for `sage-build-env-config`).
+
+This will require changes to some scripts in `src/bin`:
+{{{
+src/bin/sage-pkg:1:#!/usr/bin/env sage-system-python
+src/bin/sage-coverage:1:#!/usr/bin/env sage-system-python
+src/bin/sage-pypkg-location:1:#!/usr/bin/env sage-system-python
+src/bin/sage-unzip:1:#!/usr/bin/env sage-system-python
+src/bin/sage-coverageall:1:#!/usr/bin/env sage-system-python
+src/bin/sage-location:1:#!/usr/bin/env sage-system-python
+src/bin/sage-num-threads.py:1:#!/usr/bin/env sage-system-python
+}}}
+each of which should either be moved to `build/bin` (#21559) or changed to use the Sage python instead (#29090).
+
```Description changed:
---
+++
@@ -3,10 +3,6 @@
This will require changes to some scripts in `src/bin`:
```
-This directory contains executables needed during building sage-the-distribution only. It should only be added to `PATH` by `sage-build-env` (see #29052 for `sage-build-env-config`).
-
-This will require changes to some scripts in `src/bin`:
-{{{
src/bin/sage-pkg:1:#!/usr/bin/env sage-system-python
src/bin/sage-coverage:1:#!/usr/bin/env sage-system-python
src/bin/sage-pypkg-location:1:#!/usr/bin/env sage-system-python
@@ -14,7 +10,6 @@
src/bin/sage-coverageall:1:#!/usr/bin/env sage-system-python
src/bin/sage-location:1:#!/usr/bin/env sage-system-python
src/bin/sage-num-threads.py:1:#!/usr/bin/env sage-system-python
-}}}
+```
each of which should either be moved to `build/bin` (#21559) or changed to use the Sage python instead (#29090).
-```Description changed:
---
+++
@@ -11,5 +11,5 @@
src/bin/sage-location:1:#!/usr/bin/env sage-system-python
src/bin/sage-num-threads.py:1:#!/usr/bin/env sage-system-python
```
-each of which should either be moved to `build/bin` (#21559) or changed to use the Sage python instead (#29090).
+each of which should either be moved to `build/bin` or deleted (#21559) or changed to use the Sage python instead (#29090).
#29951 will only put $SAGE_ROOT/bin in $PATH if $SAGE_ROOT is set and readable.
Repurposing this ticket to change scripts in src/bin to not use sage-system-python (which is in build/bin and not installed and not available in distributions).
Description changed:
---
+++
@@ -1,15 +1,17 @@
-This directory contains executables needed during building sage-the-distribution only. It should only be added to `PATH` by `sage-build-env` (see #29052 for `sage-build-env-config`).
+In previous tickets we have moved sage-the-distribution scripts out of `src/bin`.
-This will require changes to some scripts in `src/bin`:
+Some scripts that are (correctly) located in `src/bin` use `sage-system-python`, which is in `build/bin` and not installed and not available in distributions.
+
+We change them to just `python3`. If Sage's venv is in the front of the PATH, this is Sage's python. But these scripts work with "any python3", it does not have to be the one in which sage is installed.
```
-src/bin/sage-pkg:1:#!/usr/bin/env sage-system-python
src/bin/sage-coverage:1:#!/usr/bin/env sage-system-python
src/bin/sage-pypkg-location:1:#!/usr/bin/env sage-system-python
-src/bin/sage-unzip:1:#!/usr/bin/env sage-system-python
src/bin/sage-coverageall:1:#!/usr/bin/env sage-system-python
src/bin/sage-location:1:#!/usr/bin/env sage-system-python
src/bin/sage-num-threads.py:1:#!/usr/bin/env sage-system-python
```
-each of which should either be moved to `build/bin` or deleted (#21559) or changed to use the Sage python instead (#29090).
+We also remove `src/bin/sage-pypkg-location`, which is undocumented and unused.
+
+Description changed:
---
+++
@@ -8,7 +8,6 @@
src/bin/sage-coverage:1:#!/usr/bin/env sage-system-python
src/bin/sage-pypkg-location:1:#!/usr/bin/env sage-system-python
src/bin/sage-coverageall:1:#!/usr/bin/env sage-system-python
-src/bin/sage-location:1:#!/usr/bin/env sage-system-python
src/bin/sage-num-threads.py:1:#!/usr/bin/env sage-system-python
```
Author: Matthias Koeppe
sage-num-threads.py gets used in the build process, I think: it gets used in src/bin/sage-env. So I think this change will break building on systems with no Python 3 anywhere. Do we need to support such systems?
Thanks for catching this. This needs some thought
I think the issue is that the line between build scripts and runtime scripts is blurred. sage-env and sage-num-threads.py, maybe others?
Yes, I agree, I think the solution is to split the build-specific parts (parsing MAKEFLAGS) out from sage-num-threads.py. I'll work on it.
Sounds good to me.
Branch pushed to git repo; I updated commit sha1. New commits:
f76082a | Merge tag '9.2.beta13' into t/29355/sagelib_s_scripts_in_src_bin_should_not_use_build_bin_sage_system_python__remove_sage_pypkg_location |
ab72437 | build/bin/sage-build-num-threads: Copy from src/bin/sage-num-threads.py, simplify src/bin/sage-num-threads.py |
1e797a1 | build/make/install: Set SAGE_NUM_THREADS, SAGE_NUM_THREADS_PARALLEL using sage-build-num-threads |
850f97c | src/bin/sage-env: If SAGE_NUM_THREADS, SAGE_NUM_THREADS_PARALLEL are set already, do not call sage-num-threads.py to set them |
In a follow-up ticket, we could probably get rid of src/bin/sage-num-threads.py completely by implementing the desired defaulting behavior in src/sage/parallel/ncpus.py and src/sage/doctest/control.py.
Description changed:
---
+++
@@ -1,8 +1,6 @@
In previous tickets we have moved sage-the-distribution scripts out of `src/bin`.
Some scripts that are (correctly) located in `src/bin` use `sage-system-python`, which is in `build/bin` and not installed and not available in distributions.
-
-We change them to just `python3`. If Sage's venv is in the front of the PATH, this is Sage's python. But these scripts work with "any python3", it does not have to be the one in which sage is installed.
```
src/bin/sage-coverage:1:#!/usr/bin/env sage-system-python
@@ -11,6 +9,8 @@
src/bin/sage-num-threads.py:1:#!/usr/bin/env sage-system-python
```
-We also remove `src/bin/sage-pypkg-location`, which is undocumented and unused.
+We change `sage-coverage*` to just use `python3`. If Sage's venv is in the front of the PATH, this is Sage's python. But these scripts work with "any python3", it does not have to be the one in which sage is installed.
+We remove `src/bin/sage-pypkg-location`, which is undocumented and unused.
+We move `src/bin/sage-num-threads.py` to `build/bin/sage-build-num-threads` (where it keeps using `sage-system-python`), leaving a much simplified version `src/bin/sage-num-threads.py` (which is using `python3`) behind.I should probably know the answer to this, but is there a promise that "python3" is the official name for the python interpreter in the future? After 2.x is completely dead, they're not going to change it back to "python"?
(This is also relevant when we're searching for (only) a system "python3")
The name "python3" should be OK for a long time:
and https://twitter.com/gvanrossum/status/1306082472443084801
Branch pushed to git repo; I updated commit sha1. New commits:
ef1444a | Merge tag '9.2.beta14' into t/29355/sagelib_s_scripts_in_src_bin_should_not_use_build_bin_sage_system_python__remove_sage_pypkg_location |
Needs review
Branch pushed to git repo; I updated commit sha1. New commits:
f4a75a9 | Merge tag '9.3.beta0' into t/29355/sagelib_s_scripts_in_src_bin_should_not_use_build_bin_sage_system_python__remove_sage_pypkg_location |
Branch pushed to git repo; I updated commit sha1. New commits:
a4e757e | build/pkgs/sagelib/src/setup.py: Remove bin/sage-pypkg-location |
Can we please get this ticket in?
Reviewer: Dima Pasechnik
lgtm
Thanks!
I was currently checking it also. So, I confirm that sage -coverage and sage -coverageall still work. Also, sage-num-threads.py still seems to work:
(sage-sh) $ echo $SAGE_NUM_THREADS
1
(sage-sh) $ echo $SAGE_NUM_THREADS_PARALLEL
8
or the above were defined from build/make/install?
Anyway, I have:
sage: import os
sage: os.environ['SAGE_NUM_THREADS']
'1'
sage: os.environ['SAGE_NUM_THREADS_PARALLEL']
'8'
positive review!
Changed reviewer from Dima Pasechnik to Dima Pasechnik, Sébastien Labbé