Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#24729 closed enhancement (fixed)

Add --with-python=3 configure flag to replace SAGE_PYTHON3=yes

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.2
Component: build: configure Keywords:
Cc: embray, chapoton, mkoeppe Merged in:
Authors: Jeroen Demeyer Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: 8af9c37 (Commits) Commit:
Dependencies: Stopgaps:

Description


Change History (34)

comment:1 Changed 2 years ago by jdemeyer

  • Branch set to u/jdemeyer/add___with_python_3_configure_flag_to_replace_sage_python3_yes

comment:2 Changed 2 years ago by jdemeyer

  • Cc mkoeppe added
  • Commit set to 4b4f40ee915f683991cb4ece6314c9a242b5792c

New commits:

4b4f40eAdd --with-python=3 configure flag to replace SAGE_PYTHON3=yes

comment:3 Changed 2 years ago by jdemeyer

  • Status changed from new to needs_review

comment:4 Changed 2 years ago by embray

+1 Been meaning to do the same for a while (actually already did once in an old branch that I abandoned). Though as written this will obviously conflict with #21524.

comment:5 follow-up: Changed 2 years ago by chapoton

what is the advantage ?

comment:6 in reply to: ↑ 5 Changed 2 years ago by jdemeyer

Replying to chapoton:

what is the advantage ?

First of all, it's a more standard way to configure things. But one real advantage is that you only need to set this once when running ./configure initially. With this ticket, once you ran ./configure --with-python=2 you can just run make without needing to remember to set SAGE_PYTHON3=yes.

comment:7 follow-up: Changed 2 years ago by chapoton

ok. Great.

Stupid question maybe: in

+export SAGE_PYTHON_VERSION=@SAGE_PYTHON_VERSION@

should there be " around @SAGE_PYTHON_VERSION@ ?

comment:8 in reply to: ↑ 7 Changed 2 years ago by jdemeyer

Replying to chapoton:

ok. Great.

Stupid question maybe: in

+export SAGE_PYTHON_VERSION=@SAGE_PYTHON_VERSION@

should there be " around @SAGE_PYTHON_VERSION@ ?

That's not needed in shell scripts.

comment:9 Changed 2 years ago by mkoeppe

When I see a configure option --with-python=..., I would expect that I can do ./configure --with-python=/usr/bin/python3.5.77 . How about having options --with-python2, --with-python3 instead?

comment:10 Changed 2 years ago by jdemeyer

That's a fair comment. Let me just say that I chose the option analogous to --with-mp=gmp|mpir

comment:11 Changed 2 years ago by jdemeyer

Actually, this shouldn't be a --with option at all since those typically indicate components of the system which should be used. For example, mpfr has a --with-gmp option to indicate which GMP it should use. So it should be an --enable option.

I don't like --enable-python2 and --enable-python3 so much because it's not obvious that they are mutually exclusive. So I would prefer --enable-python=2 and --enable-python=3.

Are you willing to give positive review if I change the option to --enable-python instead of --with-python?

comment:12 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:13 follow-up: Changed 2 years ago by mkoeppe

No, I would say that --with... is the right thing to use, because Python is a dependency, rather than a feature of Sage. --with... does not require an argument. And we can reserve the use of an argument for future use (such as working with a distribution's Python rather than our own).

And a help string can make clear that two options are mutually exclusive (as well as signalling an error if both are provided.)

comment:14 in reply to: ↑ 13 ; follow-up: Changed 2 years ago by jdemeyer

Replying to mkoeppe:

No, I would say that --with... is the right thing to use, because Python is a dependency, rather than a feature of Sage.

Huh? Maybe you are confusing Sage-the-library with Sage-the-distribution. I would argue that ./configure is really the configuration script for Sage-the-distribution and that the Python-in-Sage is a feature (not a dependency) of Sage-the-distribution. Python is a dependency of Sage-the-library but I don't think that is relevant here.

Apart from this, there is also the unrelated Python-used-by-the-build-system which is a dependency. In fact, that is what we should use the --with-python option for.

comment:15 in reply to: ↑ 14 Changed 2 years ago by mkoeppe

Replying to jdemeyer:

Replying to mkoeppe:

No, I would say that --with... is the right thing to use, because Python is a dependency, rather than a feature of Sage.

Huh? Maybe you are confusing Sage-the-library with Sage-the-distribution. I would argue that ./configure is really the configuration script for Sage-the-distribution and that the Python-in-Sage is a feature (not a dependency) of Sage-the-distribution. Python is a dependency of Sage-the-library but I don't think that is relevant here.

Hm, well, I thought you might say that. But the configuration of sage-the-distribution does also affect sagelib (configuration is somehow passed through); it does not just make one or the other version of python available for sagelib to find.

Also, in light of various efforts to use system packages when available I could imagine that one day sage-the-distribution could skip installing its copy of Python and then --with-python3=/usr/bin/python3.5.77 would make sense.

Apart from this, there is also the unrelated Python-used-by-the-build-system which is a dependency. In fact, that is what we should use the --with-python option for.

For that one I would suggest something like --with-build-python to disambiguate.

comment:16 Changed 2 years ago by jdemeyer

OK, I see your point of using --with-python instead of --enable-python. Still, I see no reason for separate --with-python2 and --with-python3 options. Why do you prefer that over --with-python? We already have --with-blas=atlas and not --with-atlas, so an option --with-python=2 seems more consistent.

comment:17 follow-up: Changed 2 years ago by mkoeppe

First of all, I'd say --with-python=2 just looks odd to the trained configure-user's eye, one would expect to see --with-python=python2 or perhaps --with-python-version=2. And as I explained, --with-python2 and --with-python3 allows for future, compatible extensions.

Not sure if the following will make sense, but perhaps a difference to BLAS is that Python exists as an executable. So --with-python3=/usr/bin/python3.5.77 makes sense; whereas one would perhaps see --with-blas=atlas --with-atlas-lib=/usr/lib/dfdkfldkflkdlfk --with-atlas-include=/usr/include/fjdkfjkdjfkjd.

But obviously all of this is not exact science; so I don't insist.

comment:18 Changed 2 years ago by embray

I like --with-python=2. I think it would be more consistent to be --with-python=python2 but that's obviously redundant and more likely to clash with a future use of --with-python=<path-to-python-executabe>. So I think in this case we could make an exception.

comment:19 Changed 2 years ago by chapoton

Not an expert at all on this kind of things, but I agree with --with-python=2.

comment:20 in reply to: ↑ 17 Changed 2 years ago by jdemeyer

Replying to mkoeppe:

First of all, I'd say --with-python=2 just looks odd to the trained configure-user's eye, one would expect to see --with-python=python2 or perhaps --with-python-version=2.

I mainly used --with-python=2 because it's the shortest of those alternatives.

comment:21 Changed 2 years ago by jdemeyer

Given that nobody really objects, is somebody willing to give positive review?

comment:22 Changed 2 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:23 Changed 2 years ago by embray

  • Status changed from needs_review to positive_review

Unless mkoeppe still objects, +1 from me. I will just make this a dependency of #21524 (even though it came first, I don't mind).

comment:24 Changed 2 years ago by embray

  • Reviewers set to Erik Bray

comment:25 Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work
Running the test suite for scipy-0.19.1...
  File "spkg-check.py", line 9
    print r
          ^
SyntaxError: Missing parentheses in call to 'print'

comment:26 Changed 2 years ago by git

  • Commit changed from 4b4f40ee915f683991cb4ece6314c9a242b5792c to 8af9c37152ef40a71238966e54e245ff86cd2bf3

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

8af9c37Safer enabling of SAGE_PYTHON3=yes

comment:27 Changed 2 years ago by jdemeyer

Right, let's play the make-this-work-without-patching-configure game again :-)

comment:28 Changed 2 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:29 follow-up: Changed 2 years ago by chapoton

shouldn't we fix the scipy print also, in passing ?

comment:30 in reply to: ↑ 29 Changed 2 years ago by jdemeyer

Replying to chapoton:

shouldn't we fix the scipy print also, in passing ?

See #24766 for that.

comment:31 Changed 2 years ago by embray

  • Status changed from needs_review to positive_review

comment:32 Changed 2 years ago by vbraun

  • Branch changed from u/jdemeyer/add___with_python_3_configure_flag_to_replace_sage_python3_yes to 8af9c37152ef40a71238966e54e245ff86cd2bf3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:33 Changed 2 years ago by chapoton

  • Commit 8af9c37152ef40a71238966e54e245ff86cd2bf3 deleted

Well.. On 8.2.b7 I get

./configure --with-python=3
configure: WARNING: unrecognized options: --with-python

comment:34 Changed 2 years ago by jdemeyer

Try running make configure first.

Note: See TracTickets for help on using tickets.