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

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

Closed
mkoeppe opened this issue Mar 17, 2020 · 37 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Mar 17, 2020

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

@mkoeppe mkoeppe added this to the sage-9.1 milestone Mar 17, 2020
@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 Apr 9, 2020
@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Aug 13, 2020
@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 18, 2020

comment:6

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

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Remove SAGE_ROOT/build/bin from PATH set by sage-env Sagelib's scripts in src/bin should not use build/bin/sage-system-python; remove sage-pypkg-location Sep 18, 2020
@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.2 Sep 18, 2020
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 18, 2020

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 18, 2020

Author: Matthias Koeppe

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 18, 2020

Commit: f6dcc14

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 18, 2020

New commits:

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

@jhpalmieri
Copy link
Member

comment:11

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?

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 21, 2020

comment:12

Thanks for catching this. This needs some thought

@jhpalmieri
Copy link
Member

comment:13

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?

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 21, 2020

comment:14

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.

@jhpalmieri
Copy link
Member

comment:16

Sounds good to me.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 22, 2020

Changed commit from f6dcc14 to 850f97c

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 23, 2020

comment:20

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.

@orlitzky
Copy link
Contributor

comment:21

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

@orlitzky
Copy link
Contributor

comment:22

The name "python3" should be OK for a long time:

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 25, 2020

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 2, 2020

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 2, 2020

Changed commit from 850f97c to ef1444a

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 2, 2020

comment:25

Needs review

@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Oct 24, 2020
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 1, 2020

Changed commit from ef1444a to f4a75a9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 1, 2020

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 1, 2020

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 1, 2020

Changed commit from f4a75a9 to a4e757e

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 2, 2020

comment:29

Can we please get this ticket in?

@dimpase
Copy link
Member

dimpase commented Nov 3, 2020

Reviewer: Dima Pasechnik

@dimpase
Copy link
Member

dimpase commented Nov 3, 2020

comment:30

lgtm

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 3, 2020

comment:31

Thanks!

@seblabbe
Copy link
Contributor

seblabbe commented Nov 3, 2020

comment:32

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!

@seblabbe
Copy link
Contributor

seblabbe commented Nov 3, 2020

Changed reviewer from Dima Pasechnik to Dima Pasechnik, Sébastien Labbé

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 14, 2020

comment:33

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

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