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

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

Closed
jdemeyer opened this issue Feb 14, 2018 · 32 comments
Closed

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

jdemeyer opened this issue Feb 14, 2018 · 32 comments

Comments

@jdemeyer
Copy link

CC: @embray @fchapoton @mkoeppe

Component: build: configure

Author: Jeroen Demeyer

Branch: 8af9c37

Reviewer: Erik Bray

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

@jdemeyer jdemeyer added this to the sage-8.2 milestone Feb 14, 2018
@jdemeyer
Copy link
Author

@jdemeyer
Copy link
Author

Commit: 4b4f40e

@jdemeyer
Copy link
Author

New commits:

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

@embray
Copy link
Contributor

embray commented Feb 14, 2018

comment:4

+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.

@fchapoton
Copy link
Contributor

comment:5

what is the advantage ?

@jdemeyer
Copy link
Author

comment:6

Replying to @fchapoton:

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.

@fchapoton
Copy link
Contributor

comment:7

ok. Great.

Stupid question maybe: in

+export SAGE_PYTHON_VERSION=@SAGE_PYTHON_VERSION@

should there be " around @SAGE_PYTHON_VERSION@ ?

@jdemeyer
Copy link
Author

comment:8

Replying to @fchapoton:

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.

@mkoeppe
Copy link
Member

mkoeppe commented Feb 14, 2018

comment:9

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?

@jdemeyer
Copy link
Author

comment:10

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

@jdemeyer
Copy link
Author

comment:11

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?

@mkoeppe
Copy link
Member

mkoeppe commented Feb 15, 2018

comment:13

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

@jdemeyer
Copy link
Author

comment:14

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.

@mkoeppe
Copy link
Member

mkoeppe commented Feb 15, 2018

comment:15

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.

@jdemeyer
Copy link
Author

comment:16

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.

@mkoeppe
Copy link
Member

mkoeppe commented Feb 15, 2018

comment:17

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.

@embray
Copy link
Contributor

embray commented Feb 16, 2018

comment:18

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.

@fchapoton
Copy link
Contributor

comment:19

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

@jdemeyer
Copy link
Author

comment:20

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.

@jdemeyer
Copy link
Author

comment:21

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

@embray
Copy link
Contributor

embray commented Feb 16, 2018

comment:23

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

@embray
Copy link
Contributor

embray commented Feb 16, 2018

Reviewer: Erik Bray

@vbraun
Copy link
Member

vbraun commented Feb 17, 2018

comment:25
Running the test suite for scipy-0.19.1...
  File "spkg-check.py", line 9
    print r
          ^
SyntaxError: Missing parentheses in call to 'print'

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 18, 2018

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

8af9c37Safer enabling of SAGE_PYTHON3=yes

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 18, 2018

Changed commit from 4b4f40e to 8af9c37

@jdemeyer
Copy link
Author

comment:27

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

@fchapoton
Copy link
Contributor

comment:29

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

@jdemeyer
Copy link
Author

comment:30

Replying to @fchapoton:

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

See #24766 for that.

@vbraun
Copy link
Member

vbraun commented Feb 20, 2018

@fchapoton
Copy link
Contributor

Changed commit from 8af9c37 to none

@fchapoton
Copy link
Contributor

comment:33

Well.. On 8.2.b7 I get

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

@jdemeyer
Copy link
Author

jdemeyer commented Mar 3, 2018

comment:34

Try running make configure first.

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