Opened 2 years ago
Closed 2 years ago
#30546 closed defect (fixed)
python3 spkg-configure: Only search for "python3", implement "configure --with-python=/PATH/TO/PYTHON"
Reported by: | Dima Pasechnik | Owned by: | |
---|---|---|---|
Priority: | critical | Milestone: | sage-9.2 |
Component: | build: configure | Keywords: | |
Cc: | Matthias Köppe, Erik Bray, Michael Orlitzky | Merged in: | |
Authors: | Matthias Koeppe, Dima Pasechnik | Reviewers: | Dima Pasechnik, Matthias Koeppe |
Report Upstream: | N/A | Work issues: | |
Branch: | f19d9a4 (Commits, GitHub, GitLab) | Commit: | f19d9a4a356da5e4b068dc52c321e703c2648da3 |
Dependencies: | #30576, #29500 | Stopgaps: |
Description (last modified by )
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.
Change History (77)
comment:1 Changed 2 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 2 years ago by
Report Upstream: | N/A → Reported upstream. No feedback yet. |
---|
comment:3 Changed 2 years ago by
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?
comment:4 Changed 2 years ago by
Status: | needs_review → needs_info |
---|
comment:5 follow-up: 7 Changed 2 years ago by
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)
comment:6 Changed 2 years ago by
Status: | needs_info → needs_review |
---|
comment:7 follow-up: 8 Changed 2 years ago by
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.
comment:8 follow-up: 17 Changed 2 years ago by
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.
comment:9 Changed 2 years ago by
Putting the bin directory of a Python installation in the front of the PATH is the standard way of activating a venv, for example.
comment:10 follow-up: 11 Changed 2 years ago by
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.
comment:11 follow-up: 14 Changed 2 years ago by
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/python@3.7/bin:$PATH"
Prepending to the PATH here might open a can of worms.
comment:12 Changed 2 years ago by
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?
comment:13 follow-ups: 15 18 Changed 2 years ago by
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.
comment:14 Changed 2 years ago by
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.
comment:15 follow-up: 26 Changed 2 years ago by
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?
comment:16 Changed 2 years ago by
Priority: | critical → major |
---|
Sorry, I don't see evidence of a "critical defect" anywhere here
comment:17 Changed 2 years ago by
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.
comment:18 follow-up: 19 Changed 2 years ago by
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?
comment:19 follow-ups: 20 21 Changed 2 years ago by
Replying to mjo:
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 doesPATH=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.
comment:20 Changed 2 years ago by
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?
comment:21 Changed 2 years ago by
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.
comment:22 follow-up: 23 Changed 2 years ago by
We could perhaps just reduce this list to just python3
,
and add the --with-python
support.
comment:23 Changed 2 years ago by
Replying to mkoeppe:
We could perhaps just reduce this list to just
python3
,
But there are cases where python3 is not what one wants - do you mean to bypass the test then all together? This is not a good idea, we still need to test the input provided by --with-python=
and add the
--with-python
support.
comment:24 follow-up: 27 Changed 2 years ago by
No, just shorten the list of candidate binary names to just python3
. Keeping the test and thus the loop over the elements of PATH.
And add --with-python
.
Both in the same ticket.
comment:25 Changed 2 years ago by
I don't understand - isn't prepending to PATH a venv's binary directory about a new directory, only containing (links to) things which aren't in the PATH at all, but nothing else?
comment:26 Changed 2 years ago by
Replying to mkoeppe:
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?
if you want to install Sage into a Python that is in /usr/bin, then under the current scheme of things it might not get picked if /usr/bin is not the 1st in PATH.
comment:27 Changed 2 years ago by
Replying to mkoeppe:
No, just shorten the list of candidate binary names to just
python3
.
Then only the unversioned python3 would be possible, no?
comment:28 follow-up: 29 Changed 2 years ago by
If a user wants a versioned one, they would be using --with-python=....
comment:29 Changed 2 years ago by
Replying to mkoeppe:
If a user wants a versioned one, they would be using
--with-python=....
naturally one will need to run tests, currently encapsulated in an argument of AC_PATH_PROGS_FEATURE_CHECK()
on the input.
comment:31 Changed 2 years ago by
Status: | needs_review → needs_work |
---|---|
Summary: | make sure the newest system python3 is selected → python3 spkg-configure: Only search for "python3", implement "configure --with-python=/PATH/TO/PYTHON" |
comment:32 Changed 2 years ago by
Can we get rid of Python 2 checks here? It's pointless to try to build Sage with Python 3, so why bother with
AS_IF([test $SAGE_PYTHON_VERSION = 2], [ dnl If we use Python 2 for Sage, we install Python 3...
comment:33 Changed 2 years ago by
Matthias, please refactor your wall of code in build/pkgs/python3/spkg-configure.m4
into a separate macro that just checks the features (to be put in m4/
), and the rest,
its too onerous as it is, with setting various ac_*_PYTHON3*
variables, very hard for me to proceed here otherwise.
The refactoring should have taken place on #27824, I know...
comment:35 Changed 2 years ago by
Authors: | Dima Pasechnik |
---|---|
Branch: | u/dimpase/packages/python3conf |
Commit: | 3add5ca179ba42ba7c3136eba5b2be674572c09a |
comment:36 Changed 2 years ago by
Description: | modified (diff) |
---|---|
Report Upstream: | Reported upstream. No feedback yet. → N/A |
comment:37 Changed 2 years ago by
Authors: | → Matthias Koeppe |
---|
comment:38 Changed 2 years ago by
Branch: | → u/mkoeppe/python3_spkg_configure__only_search_for__python3___implement__configure___with_python__path_to_python_ |
---|
comment:39 Changed 2 years ago by
Authors: | Matthias Koeppe → Matthias Koeppe, ... |
---|---|
Commit: | → 3b17cb944885581f7a97b05e8c95e1acf42c0c3f |
Here you go
New commits:
3b17cb9 | m4/sage_check_python_for_venv.m4: New, factored out from build/pkgs/python3/spkg-configure.m4
|
comment:40 Changed 2 years ago by
Commit: | 3b17cb944885581f7a97b05e8c95e1acf42c0c3f → 0677e24c2cb26195ef32535d3b1e7b246e6d5d3b |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
0677e24 | build/pkgs/python3/spkg-configure.m4: Remove dead code for python2 build
|
comment:41 Changed 2 years ago by
Authors: | Matthias Koeppe, ... → Matthias Koeppe, Dima Pasechnik |
---|---|
Branch: | u/mkoeppe/python3_spkg_configure__only_search_for__python3___implement__configure___with_python__path_to_python_ → u/dimpase/build/py3x_conf |
Commit: | 0677e24c2cb26195ef32535d3b1e7b246e6d5d3b → d8a0bfb3535af622235ff9305b927eb8b02a327e |
Status: | needs_work → needs_review |
comment:42 follow-up: 56 Changed 2 years ago by
--- a/src/bin/sage-env-config.in +++ b/src/bin/sage-env-config.in @@ -46,7 +46,6 @@ CONFIGURED_OBJCXX="@OBJCXX@" ####################################### # Other configuration (exported) ####################################### -export SAGE_PYTHON_VERSION=@SAGE_PYTHON_VERSION@ export PYTHON_FOR_VENV="@PYTHON_FOR_VENV@"
I don't think this is a good idea. User packages' install scripts might depend on it.
comment:43 Changed 2 years ago by
-case "$with_python" in - 3) SAGE_PYTHON_VERSION=3;; - 3*) AC_MSG_WARN([the only allowed value for --with-python is 3; specific Python 3.x versions cannot be requested using --with-python. For compatibility reasons, this is only a warning. Use './configure PYTHON3=/path/to/python3' to use a specific Python 3 binary for the Sage venv.]) - SAGE_PYTHON_VERSION=3;; - "") SAGE_PYTHON_VERSION=3;; - *) - AC_MSG_ERROR([the only allowed value for --with-python is 3. Support for Python 2 has been removed in Sage 9.2.]);;
And I think the previously allowed argument --with-python=3
should still be allowed and be a no-op; and --with-python=2
should give a specific error message as before.
comment:44 Changed 2 years ago by
Commit: | d8a0bfb3535af622235ff9305b927eb8b02a327e → fe70ff3743a9e68671d2a1929c2b292d3d002d31 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
fe70ff3 | correct the use of --with-python in docs and tox
|
comment:45 Changed 2 years ago by
there is still one obsolete place, in docker/Dockerfile:
ARG WITH_PYTHON=2 ENV WITH_PYTHON $WITH_PYTHON ARG MAKEFLAGS="-j2" ENV MAKEFLAGS $MAKEFLAGS ARG SAGE_NUM_THREADS="2" ENV SAGE_NUM_THREADS $SAGE_NUM_THREADS RUN make configure RUN ./configure --with-python=$WITH_PYTHON
can it be removed?
comment:46 Changed 2 years ago by
Commit: | fe70ff3743a9e68671d2a1929c2b292d3d002d31 → 10baeecc00a179e6191458f173a05c8fdaa34edc |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
10baeec | rectify README a bit
|
comment:49 Changed 2 years ago by
Commit: | 10baeecc00a179e6191458f173a05c8fdaa34edc → 7f20ad1e3e50a667069496c64fa7c7e7bc37ff82 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
7f20ad1 | more changes for docker and tox
|
comment:51 Changed 2 years ago by
--- a/src/doc/en/developer/portability_testing.rst +++ b/src/doc/en/developer/portability_testing.rst @@ -371,7 +371,7 @@ command ``docker build``. For example:: [mkoeppe@sage sage]$ docker build . -f Dockerfile \ --build-arg BASE_IMAGE=ubuntu:latest \ --build-arg NUMPROC=4 \ - --build-arg EXTRA_CONFIGURE_ARGS="--with-python=2" + --build-arg EXTRA_CONFIGURE_ARGS="--with-python=/usr/opt/python3.42"
better to use an example that looks like a binary, not a directory
comment:52 Changed 2 years ago by
Commit: | 7f20ad1e3e50a667069496c64fa7c7e7bc37ff82 → 83acd7b0916e66fda4d0998132e69e441fb6c328 |
---|
comment:54 Changed 2 years ago by
Commit: | 83acd7b0916e66fda4d0998132e69e441fb6c328 → 7ac00cdd4725a01d53f46dba635e824bb5eb921f |
---|
comment:56 Changed 2 years ago by
Replying to mkoeppe:
--- a/src/bin/sage-env-config.in +++ b/src/bin/sage-env-config.in @@ -46,7 +46,6 @@ CONFIGURED_OBJCXX="@OBJCXX@" ####################################### # Other configuration (exported) ####################################### -export SAGE_PYTHON_VERSION=@SAGE_PYTHON_VERSION@ export PYTHON_FOR_VENV="@PYTHON_FOR_VENV@"I don't think this is a good idea. User packages' install scripts might depend on it.
it was never documented, thus, a fair game.
comment:57 follow-up: 58 Changed 2 years ago by
Commit: | 7ac00cdd4725a01d53f46dba635e824bb5eb921f → fc6b49d4071a63c83c7ecb2db12b4ff0c43e5568 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
fc6b49d | handle --with-python=2 or 3
|
comment:58 Changed 2 years ago by
Replying to git:
Branch pushed to git repo; I updated commit sha1. New commits:
fc6b49d handle --with-python=2 or 3
this addesses comment:43
comment:59 Changed 2 years ago by
Branch: | u/dimpase/build/py3x_conf → u/mkoeppe/build/py3x_conf |
---|
comment:60 Changed 2 years ago by
Commit: | fc6b49d4071a63c83c7ecb2db12b4ff0c43e5568 → b07596ddaa6e6b36fce4f9b981b9a20aec25adf5 |
---|
This seems to work well. I have made some small changes.
New commits:
201db18 | build/pkgs/python3/spkg-configure.m4: Exit with error if python3 provided by --with-python=... is not suitable
|
2262df0 | Merge branch 'u/dimpase/build/py3x_conf' of git://trac.sagemath.org/sage into t/30546/build/py3x_conf
|
b07596d | fixup
|
comment:61 Changed 2 years ago by
Reviewers: | → Dima Pasechnik, Matthias Koeppe |
---|
comment:63 Changed 2 years ago by
Actually... this still needs to ensure an absolute path for PYTHON_FOR_VENV
comment:64 Changed 2 years ago by
Commit: | b07596ddaa6e6b36fce4f9b981b9a20aec25adf5 → b21ed975edf54eff10c22a2298fc6097f5c39317 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
b21ed97 | build/pkgs/python3/spkg-configure.m4: Ensure that PYTHON_FOR_VENV is a full path
|
comment:66 Changed 2 years ago by
Commit: | b21ed975edf54eff10c22a2298fc6097f5c39317 → 8906897f6f752673eaf2c099ec4d398e5a5fc5dd |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
49aa061 | Revert "build/pkgs/python3/spkg-configure.m4: Use 'python >= 3.7'; keep checking for 'python3.8, python3.7'"
|
a8b77cd | Revert "only test python3, to be 3.7 or 3.8"
|
9403629 | build/bin/sage-system-python: Work around LC_ALL=C
|
ff0dbc6 | build/sage_bootstrap/uncompress/tar_file.py: Fix encoding to utf-8
|
f05a110 | Merge 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
|
945c8c5 | build/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
|
8906897 | Merge branch 't/30576/public/30576' into t/30546/build/py3x_conf
|
comment:68 Changed 2 years ago by
Dependencies: | #30576 → #30576, #29500 |
---|
comment:69 Changed 2 years ago by
Commit: | 8906897f6f752673eaf2c099ec4d398e5a5fc5dd → a2dbb8d4f4a7cb203fd9ee0b8b2d8d6ca093da9c |
---|
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
55993b6 | build/bin/sage-dist-helpers: Fixup
|
0a64674 | build/pkgs/gambit/spkg-install.in: Install via bdist_wheel
|
ca58693 | build/pkgs/pillow/spkg-install.in: Install via bdist_wheel
|
5a747c4 | build/bin/sage-pip-{install,uninstall}: Remove pip2 support
|
9ee2110 | build/bin/sage-dist-helpers: Also use $sudo for storing the wheel file
|
d7aac84 | src/doc/en/developer/packaging.rst: Update sdh_... documentation
|
9b7c7a0 | build/bin/sage-pip-{install,uninstall}: Fix typo in comment
|
4135e8b | build/bin/sage-pip-install: Remove an outdated comment
|
f2e7075 | Merge tag '9.2.beta13' into t/29500/install_all_python_packages_via_pip_wheel__create_pep_503_simple_repository_for_wheels
|
a2dbb8d | Merge branch 't/29500/install_all_python_packages_via_pip_wheel__create_pep_503_simple_repository_for_wheels' into t/30546/build/py3x_conf
|
comment:71 Changed 2 years ago by
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?
comment:72 Changed 2 years ago by
Status: | needs_review → needs_work |
---|
comment:73 Changed 2 years ago by
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...
comment:74 Changed 2 years ago by
Branch: | u/mkoeppe/build/py3x_conf → u/dimpase/build/py3x_conf |
---|---|
Commit: | a2dbb8d4f4a7cb203fd9ee0b8b2d8d6ca093da9c → f19d9a4a356da5e4b068dc52c321e703c2648da3 |
Status: | needs_work → needs_review |
OK, this appears to work. I just convert the name into an absolute one, if needed, by calling AC_PATH_PROG()
New commits:
f19d9a4 | allow absolute names as arguments to --with-python
|
comment:75 Changed 2 years ago by
Status: | needs_review → positive_review |
---|
comment:76 Changed 2 years ago by
Priority: | major → critical |
---|
I think we really want this in 9.2, not the least as it documents stuff.
comment:77 Changed 2 years ago by
Branch: | u/dimpase/build/py3x_conf → f19d9a4a356da5e4b068dc52c321e703c2648da3 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
perhaps copypasted comments in .m4 file may be trimmed.