Opened 3 years ago

Closed 2 years ago

#29355 closed enhancement (fixed)

Sagelib's scripts in src/bin should not use build/bin/sage-system-python; remove sage-pypkg-location

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.3
Component: scripts Keywords:
Cc: dimpase, jhpalmieri, fbissey, mjo, vbraun Merged in:
Authors: Matthias Koeppe Reviewers: Dima Pasechnik, Sébastien Labbé
Report Upstream: N/A Work issues:
Branch: u/mkoeppe/sagelib_s_scripts_in_src_bin_should_not_use_build_bin_sage_system_python__remove_sage_pypkg_location (Commits, GitHub, GitLab) Commit: a4e757e65628505736c69918c10e274b09469840
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

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.

Change History (33)

comment:1 Changed 3 years ago by mkoeppe

Description: modified (diff)

comment:2 Changed 3 years ago by mkoeppe

Description: modified (diff)

comment:3 Changed 3 years ago by mkoeppe

Description: modified (diff)

comment:4 Changed 3 years ago by mkoeppe

Milestone: sage-9.1sage-9.2

comment:5 Changed 2 years ago by mkoeppe

Milestone: sage-9.2sage-9.3

comment:6 Changed 2 years ago by mkoeppe

Cc: fbissey added
Description: modified (diff)
Milestone: sage-9.3sage-9.2
Summary: Remove SAGE_ROOT/build/bin from PATH set by sage-envSagelib's scripts in src/bin should not use build/bin/sage-system-python; remove sage-pypkg-location

#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).

comment:7 Changed 2 years ago by mkoeppe

Description: modified (diff)

comment:8 Changed 2 years ago by mkoeppe

Branch: u/mkoeppe/sagelib_s_scripts_in_src_bin_should_not_use_build_bin_sage_system_python__remove_sage_pypkg_location

comment:9 Changed 2 years ago by mkoeppe

Authors: Matthias Koeppe
Commit: f6dcc1436578fbf7c3ce6386d794c92a6492c1c0
Status: newneeds_review

New commits:

063a0afsrc/bin/sage-pypkg-location: Remove
f6dcc14src/bin/sage*: Do not use sage-system-python

comment:10 Changed 2 years ago by mkoeppe

Cc: mjo added

comment:11 Changed 2 years ago by jhpalmieri

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?

comment:12 Changed 2 years ago by mkoeppe

Thanks for catching this. This needs some thought

comment:13 Changed 2 years ago by jhpalmieri

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?

comment:14 Changed 2 years ago by mkoeppe

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.

comment:15 Changed 2 years ago by mkoeppe

Status: needs_reviewneeds_work

comment:16 Changed 2 years ago by jhpalmieri

Sounds good to me.

comment:17 Changed 2 years ago by git

Commit: f6dcc1436578fbf7c3ce6386d794c92a6492c1c0850f97c381f65be85319f4c3d94e0a4b1efc0399

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

f76082aMerge 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
ab72437build/bin/sage-build-num-threads: Copy from src/bin/sage-num-threads.py, simplify src/bin/sage-num-threads.py
1e797a1build/make/install: Set SAGE_NUM_THREADS, SAGE_NUM_THREADS_PARALLEL using sage-build-num-threads
850f97csrc/bin/sage-env: If SAGE_NUM_THREADS, SAGE_NUM_THREADS_PARALLEL are set already, do not call sage-num-threads.py to set them

comment:18 Changed 2 years ago by mkoeppe

Status: needs_workneeds_review

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.

comment:19 Changed 2 years ago by mkoeppe

Description: modified (diff)

comment:20 in reply to:  18 Changed 2 years ago by mkoeppe

Replying to mkoeppe:

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.

I have opened #30639 for that.

comment:21 Changed 2 years ago by mjo

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

comment:22 Changed 2 years ago by mjo

comment:24 Changed 2 years ago by git

Commit: 850f97c381f65be85319f4c3d94e0a4b1efc0399ef1444a24a9458ffc00114c0c22145621342dd70

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

ef1444aMerge 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

comment:25 Changed 2 years ago by mkoeppe

Needs review

comment:26 Changed 2 years ago by mkoeppe

Milestone: sage-9.2sage-9.3

comment:27 Changed 2 years ago by git

Commit: ef1444a24a9458ffc00114c0c22145621342dd70f4a75a93353a1122f768dd78d93ad06d71652ab2

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

f4a75a9Merge 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

comment:28 Changed 2 years ago by git

Commit: f4a75a93353a1122f768dd78d93ad06d71652ab2a4e757e65628505736c69918c10e274b09469840

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

a4e757ebuild/pkgs/sagelib/src/setup.py: Remove bin/sage-pypkg-location

comment:29 Changed 2 years ago by mkoeppe

Can we please get this ticket in?

comment:30 Changed 2 years ago by dimpase

Reviewers: Dima Pasechnik
Status: needs_reviewpositive_review

lgtm

comment:31 Changed 2 years ago by mkoeppe

Thanks!

comment:32 Changed 2 years ago by slabbe

Reviewers: Dima PasechnikDima Pasechnik, Sébastien Labbé

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!

comment:33 Changed 2 years ago by mkoeppe

Cc: vbraun added
Resolution: fixed
Status: positive_reviewclosed

Looks like this ticket was merged as part of #30627.

Note: See TracTickets for help on using tickets.