Opened 3 years ago

Closed 3 years ago

#29090 closed defect (fixed)

sage-system-python fixup

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.1
Component: build: configure Keywords:
Cc: dimpase, embray, jhpalmieri, vbraun, saraedum, slelievre, mjo Merged in:
Authors: Matthias Koeppe Reviewers: Michael Orlitzky
Report Upstream: N/A Work issues:
Branch: 7ca6033 (Commits, GitHub, GitLab) Commit: 7ca6033e076c9ba52fb1811729d5af47ca3d09d1
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

This ticket updates implementation and use of build/bin/sage-system-python.

  1. The script now prefers python3 over python, and in fact checks several more executable names such as python3.7.
  1. Because sage-download-file needs the urllib module (which is for example not provided by Debian's python3-minimal package), it only accepts a python that provides it.
  1. A mistake from #28657 (as reported in #28738) is fixed - src/bin/sage-location should use Sage's python, not the system python.
  1. Error reporting is improved, it gives a clearer error message instead of messages such as (see #28738 or https://github.com/mkoeppe/sage/runs/412019418)
    /sage/build/bin/sage-system-python: line 24: exec: : not found
    
  1. In configure, we now call sage-system-python and exit with an error if no suitable system python is found.

See also:

Change History (17)

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

Branch: u/mkoeppe/configure_forgets_to_check_whether_any_python_is_available_on_the_system

comment:4 Changed 3 years ago by mkoeppe

Authors: Matthias Koeppe
Commit: f3ba86386c66fbe575b0a6433c759dc758fc4e94
Description: modified (diff)
Status: newneeds_review
Summary: configure forgets to check whether ANY Python is available on the systemsage-system-python fixup

New commits:

be5d057build/bin/sage-system-python: Prefer python3, check a larger list of executable names, check for urllib; exit with a proper error message if no suitable Python is found
04e0932src/bin/sage-location: Use sage-python instead of sage-system-python
f3ba863configure.ac: Stop early if no suitable Python is found

comment:5 Changed 3 years ago by mkoeppe

Description: modified (diff)

comment:6 Changed 3 years ago by mkoeppe

Cc: saraedum slelievre added

comment:7 Changed 3 years ago by git

Commit: f3ba86386c66fbe575b0a6433c759dc758fc4e946efd73e0d254503c114d89a6b2b1af8b1735dbf1

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a55d1f9build/bin/sage-system-python: Prefer python3, check a larger list of executable names, check for urllib; exit with a proper error message if no suitable Python is found
b4b99c0src/bin/sage-location: Use sage-python instead of sage-system-python
6efd73econfigure.ac: Stop early if no suitable Python is found

comment:8 Changed 3 years ago by mkoeppe

Rebased on 9.1.beta8, needs review

comment:9 Changed 3 years ago by jhpalmieri

It looks okay to me, but others should look, too. I haven't done very thorough testing.

comment:10 Changed 3 years ago by mkoeppe

Cc: mjo added

comment:11 Changed 3 years ago by mjo

I think this eventually does the right thing, but if you're going to record a suitable system python in configure.ac, then why should sage-system-python try several different versions each time it's run? Couldn't we just move that check into configure.ac, and then have sage-system-python launch the version that ./configure found?

comment:12 Changed 3 years ago by mkoeppe

The problem is that it needs to work also for bootstrap, at a time when even the configure script is not there.

comment:13 in reply to:  12 ; Changed 3 years ago by mjo

Reviewers: Michael Orlitzky
Status: needs_reviewpositive_review

Replying to mkoeppe:

The problem is that it needs to work also for bootstrap, at a time when even the configure script is not there.

I am expecting this all to make sense and I don't think it's going to.

Placing "python" later in the list might do weird things on Gentoo, where eselect python is used to select the system python (what "python" does). If someone has selected python-3.6 but happens to have 3.8 installed, the script will choose 3.8 instead. I don't foresee a real problem here, just mentioning it for posterity.

I would suggest $() in configure.ac over backticks again, but other than it looks OK.

comment:14 Changed 3 years ago by git

Commit: 6efd73e0d254503c114d89a6b2b1af8b1735dbf17ca6033e076c9ba52fb1811729d5af47ca3d09d1
Status: positive_reviewneeds_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

7ca6033configure.ac: Change ` ` to $( )

comment:15 in reply to:  13 Changed 3 years ago by mkoeppe

Replying to mjo:

Replying to mkoeppe:

The problem is that it needs to work also for bootstrap, at a time when even the configure script is not there.

I am expecting this all to make sense and I don't think it's going to.

Right :)

Placing "python" later in the list might do weird things on Gentoo, where eselect python is used to select the system python (what "python" does). If someone has selected python-3.6 but happens to have 3.8 installed, the script will choose 3.8 instead. I don't foresee a real problem here, just mentioning it for posterity.

That's fine. The sage-system-python -- if not abused (see #29355) -- is used in a very limited context only.

I would suggest $() in configure.ac over backticks again, but other than it looks OK.

Thanks, done.

comment:16 Changed 3 years ago by mkoeppe

Status: needs_reviewpositive_review

comment:17 Changed 3 years ago by vbraun

Branch: u/mkoeppe/configure_forgets_to_check_whether_any_python_is_available_on_the_system7ca6033e076c9ba52fb1811729d5af47ca3d09d1
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.