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

python3 spkg-configure: Only search for "python3", implement "configure --with-python=/PATH/TO/PYTHON" #30546

Closed
dimpase opened this issue Sep 10, 2020 · 94 comments

Comments

@dimpase
Copy link
Member

dimpase commented Sep 10, 2020

We make the configure search for a suitable python3 more straightforward.

Instead of searching for various names such as python3.8, python3.7 etc., we only look for python3.

As is standard practice, users will set their PATH so that the python3 that is accessible from the PATH is the desired Python version.

As an additional mechanism, we add configure --with-python=/PATH/TO/PYTHON. In contrast to the (undocumented) configure PYTHON3=/PATH/TO/PYTHON, it runs the usual tests whether this python is actually suitable.

Depends on #30576
Depends on #29500

CC: @mkoeppe @embray @orlitzky

Component: build: configure

Author: Matthias Koeppe, Dima Pasechnik

Branch/Commit: f19d9a4

Reviewer: Dima Pasechnik, Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/30546

@dimpase dimpase added this to the sage-9.2 milestone Sep 10, 2020
@dimpase
Copy link
Member Author

dimpase commented Sep 10, 2020

comment:1

perhaps copypasted comments in .m4 file may be trimmed.

@dimpase
Copy link
Member Author

dimpase commented Sep 10, 2020

Upstream: Reported upstream. No feedback yet.

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 10, 2020

comment:3

I don't know if this is a defect. If you have a directory containing python3 in the front of your path, shouldn't that be the python we are using?

@dimpase
Copy link
Member Author

dimpase commented Sep 10, 2020

comment:5

Why is that the case? It's perfectly fine to have different fully-featured python3.x installed.
It works on Homebrew to use its python3.8 in Sage, while python3 still is python3.7
It works on Gentoo, which may have python3.6-9 installed all at the same time.
(Python plays an important role on Linux, one can't easily bump the version)

E.g. that's what I have on a Gentoo box, without doing anything special:

# python3 <tab>
python3            python3.7-config   python3.7m-config  python3.8-config   python3.9-config   
python3.7          python3.7m         python3.8          python3.9          python3-config 

On Ubuntu 18.04 one can install python3.8 (the system python is 3.6, and I'm not sure if it works well in Sage)

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 10, 2020

comment:7

Replying to @dimpase:

Why is that the case?

Because the user seems to have expressed a preference for the python3 provided by the directory in the front of the PATH.

@dimpase
Copy link
Member Author

dimpase commented Sep 10, 2020

comment:8

Replying to @mkoeppe:

Replying to @dimpase:

Why is that the case?

Because the user seems to have expressed a preference for the python3 provided by the directory in the front of the PATH.

for this, we should revive --with-python= option, not this kinds of silly hacks.

Besides, as I said, it's not always safe to prepend stuff to your PATH.

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 10, 2020

comment:9

Putting the bin directory of a Python installation in the front of the PATH is the standard way of activating a venv, for example.

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 10, 2020

Replying to @dimpase:

As modifying PATH to put the best python 1st might be error-prone (think about Conda, Homebrew, etc)

I would think this needs some elaboration.

@dimpase
Copy link
Member Author

dimpase commented Sep 10, 2020

comment:11

Replying to @mkoeppe:

Replying to @dimpase:

As modifying PATH to put the best python 1st might be error-prone (think about Conda, Homebrew, etc)

I would think this needs some elaboration.

Say, you build under Homebrew, but want to use system Python, in /usr/bin.
Prepending /usr/bin to your PATH will likely break stuff in Hombrew, as some of it depends on /usr/local/bin being first in your PATH.

Same with Conda - it prepends stuff to PATH, and prepending /usr/bin or /usr/local/bin to PATH under active Conda will likely break it.

In general, many Python-dependent environments have the concept of "main" Python. We discussed on sage-devel that Homebrew modifies ~/.zshrc (if the account's shell is zsh)

% cat ~/.zshrc
export PATH="/usr/local/opt/[email protected]/bin:$PATH"

Prepending to the PATH here might open a can of worms.

@orlitzky
Copy link
Contributor

comment:12

I think I agree with Matthias here. Prepending locations to the search path is the "standard" way of encouraging autotools to find your preferred version of something, and our widespread use of e.g. AC_CHECK_PROGS and AC_SEARCH_LIBS relies on that.

In this case maybe it is unexpected that a copy of python-3.6 in /usr/local/bin will be preferred over a copy of python-3.8 in /usr/bin, but... does that cause any real problems? We only detect supported versions, so any version we detect should be OK, right?

@dimpase
Copy link
Member Author

dimpase commented Sep 10, 2020

comment:13

I don't understand - I have given you an example: prepending /usr/bin to PATH on Homebrew or Conda is basically not possible.

Yet I want to build Sage with Python3.8 in /usr/bin, as opposed to an earlier version which is 1st in the PATH.

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 10, 2020

comment:14

Replying to @dimpase:

In general, many Python-dependent environments have the concept of "main" Python.

Yes, it's represented by the unversioned "python3" in the front of the PATH.

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 10, 2020

comment:15

Replying to @dimpase:

I don't understand - I have given you an example: prepending /usr/bin to PATH on Homebrew or Conda is basically not possible.

Why would anyone want to prepend /usr/bin?

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 10, 2020

comment:16

Sorry, I don't see evidence of a "critical defect" anywhere here

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 10, 2020

comment:17

Replying to @dimpase:

we should revive --with-python= option

Yes, I think so. Note that it is already possible to do ./configure PYTHON3=/some/path/to/python3, but this completely disables the check for the suitability of this python.

@orlitzky
Copy link
Contributor

comment:18

Replying to @dimpase:

I don't understand - I have given you an example: prepending /usr/bin to PATH on Homebrew or Conda is basically not possible.

Yet I want to build Sage with Python3.8 in /usr/bin, as opposed to an earlier version which is 1st in the PATH.

I understand. Can you prepend your preferred location to PATH only during ./configure? It's a given that you don't want to prepend it everywhere. We have a problem either way. If we make your change, then it becomes harder to override a newer python in /usr/bin with an older one in e.g. /usr/local/bin.

Bringing back --with-python could address both concerns... but does PATH=whatever ./configure not work?

@dimpase
Copy link
Member Author

dimpase commented Sep 10, 2020

comment:19

Replying to @orlitzky:

Replying to @dimpase:

I don't understand - I have given you an example: prepending /usr/bin to PATH on Homebrew or Conda is basically not possible.

Yet I want to build Sage with Python3.8 in /usr/bin, as opposed to an earlier version which is 1st in the PATH.

I understand. Can you prepend your preferred location to PATH only during ./configure?

This is a very fragile solution. There are many things in /usr/bin that might wreak havoc, imagine some foobar-config which has to be called by ./configure

It's a given that you don't want to prepend it everywhere. We have a problem either way.

my solution has one advantage - it makes the outcome independent of the order of things in PATH, which is something that users might - and will - screw up.

If we make your change, then it becomes harder to override a newer python in /usr/bin with an older one in e.g. /usr/local/bin.

Bringing back --with-python could address both concerns... but does PATH=whatever ./configure not work?

it is a bit pointless to discuss whether it works on a box at hand, as it is obviously a very fragile solution, and I explained why.

@orlitzky
Copy link
Contributor

comment:20

Replying to @dimpase:

it is a bit pointless to discuss whether it works on a box at hand, as it is obviously a very fragile solution, and I explained why.

We probably don't want normal users doing it to override one specific program (python) and not others, sure. I was just curious. And I had forgotten that PYTHON3=... works by virtue of autotools.

I guess we all agree --with-python is the most robust answer?

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 10, 2020

comment:21

Replying to @dimpase:

my solution has one advantage - it makes the outcome independent of the order of things in PATH, which is something that users might - and will - screw up.

But I don't think this is what users want. If there's a python3 in the front of the PATH, we should not prefer a python3.8 from the rear of the PATH. This is incompatible with standard practice of activating a venv.

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 24, 2020

comment:63

Actually... this still needs to ensure an absolute path for PYTHON_FOR_VENV

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 24, 2020

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

b21ed97build/pkgs/python3/spkg-configure.m4: Ensure that PYTHON_FOR_VENV is a full path

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 24, 2020

Changed commit from b07596d to b21ed97

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 24, 2020

comment:65

I think this version is good.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 24, 2020

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

49aa061Revert "build/pkgs/python3/spkg-configure.m4: Use 'python >= 3.7'; keep checking for 'python3.8, python3.7'"
a8b77cdRevert "only test python3, to be 3.7 or 3.8"
9403629build/bin/sage-system-python: Work around LC_ALL=C
ff0dbc6build/sage_bootstrap/uncompress/tar_file.py: Fix encoding to utf-8
f05a110Merge branch 't/30008/after__30053__sphinx_3_1_2_does_not_build_on_ubuntu__trusty_xenial_bionic___debian_jessie__centos_7__again_' into t/30576/public/30576
945c8c5build/bin/sage-site (--docbuild): Set an English locale suuch as C.utf-8, but not the C locale, to ensure correct operation on Python 3.6
8906897Merge branch 't/30576/public/30576' into t/30546/build/py3x_conf

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 24, 2020

Changed commit from b21ed97 to 8906897

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 24, 2020

comment:67

Merged #30576 to avoid merge conflict

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 24, 2020

Dependencies: #30576

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 24, 2020

Changed dependencies from #30576 to #30576, #29500

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 24, 2020

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

55993b6build/bin/sage-dist-helpers: Fixup
0a64674build/pkgs/gambit/spkg-install.in: Install via bdist_wheel
ca58693build/pkgs/pillow/spkg-install.in: Install via bdist_wheel
5a747c4build/bin/sage-pip-{install,uninstall}: Remove pip2 support
9ee2110build/bin/sage-dist-helpers: Also use $sudo for storing the wheel file
d7aac84src/doc/en/developer/packaging.rst: Update sdh_... documentation
9b7c7a0build/bin/sage-pip-{install,uninstall}: Fix typo in comment
4135e8bbuild/bin/sage-pip-install: Remove an outdated comment
f2e7075Merge tag '9.2.beta13' into t/29500/install_all_python_packages_via_pip_wheel__create_pep_503_simple_repository_for_wheels
a2dbb8dMerge branch 't/29500/install_all_python_packages_via_pip_wheel__create_pep_503_simple_repository_for_wheels' into t/30546/build/py3x_conf

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 24, 2020

Changed commit from 8906897 to a2dbb8d

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 24, 2020

comment:70

Meregd #29500 to avoid merge conflict

@dimpase
Copy link
Member Author

dimpase commented Sep 24, 2020

comment:71

it is broken:

$ ./configure --with-python=/usr/bin/python3 >/tmp/p
configure: error: the python3 selected using --with-python=/usr/bin/python3 is not suitable

(while just ./configure, using the same python3, but implicitly, works)

It seems you are repeating the error that took me hours to sort out today, you have two
AC_PATH_PROGS_FEATURE_CHECK with the same tag. That's why I made sure only to have one call to AC_PATH_PROGS_FEATURE_CHECK - and why do you need more?

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 24, 2020

comment:73

Perhaps it's just that AC_PATH_PROGS_FEATURE_CHECK does not like it if the searched executable is already an absolute path?

I may only have tested with executable names without directory...

@dimpase
Copy link
Member Author

dimpase commented Sep 24, 2020

Changed branch from u/mkoeppe/build/py3x_conf to u/dimpase/build/py3x_conf

@dimpase
Copy link
Member Author

dimpase commented Sep 24, 2020

comment:74

OK, this appears to work. I just convert the name into an absolute one, if needed, by calling AC_PATH_PROG()


New commits:

f19d9a4allow absolute names as arguments to --with-python

@dimpase
Copy link
Member Author

dimpase commented Sep 24, 2020

Changed commit from a2dbb8d to f19d9a4

@dimpase
Copy link
Member Author

dimpase commented Sep 25, 2020

comment:76

I think we really want this in 9.2, not the least as it documents stuff.

@vbraun
Copy link
Member

vbraun commented Sep 30, 2020

Changed branch from u/dimpase/build/py3x_conf to f19d9a4

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

4 participants