[WIP] Call 2to3 automatically from setup.py #2262

Closed
wants to merge 5 commits into
from

Projects

None yet

4 participants

@certik
Member
certik commented Jul 8, 2013

Current status: as of 4a5b5da, all tests at Travis seem to pass. But there are still a few bugs, see below.

Prefix Path Bug

If you do:

python setup.py install --prefix=xx

it will install into build/py3k-sympy/xx instead to xx in the current directory. By following NumPy's 1.7.0 approach in their setup.py, I tried to apply the following patch:

commit c6c40f0cdbc1a4774730d4acc093ed4716cb2761
Author: Ondřej Čertík <ondrej.certik@gmail.com>
Date:   Sun Jul 7 21:50:31 2013 -0600

    Try to fix the directory change

diff --git a/setup.py b/setup.py
index 6142f7c..235c805 100755
--- a/setup.py
+++ b/setup.py
@@ -41,9 +41,12 @@

 if sys.version_info[0] >= 3:
     local_path = os.path.dirname(os.path.abspath(sys.argv[0]))
+    src_path = os.path.join(local_path, 'build', 'py3k')
     os.system("bin/use2to3")
-    os.chdir("py3k-sympy")
-    sys.path.insert(0, os.path.join(local_path, 'py3k-sympy'))
+    os.system("mkdir -p build")
+    os.system("mv py3k-sympy %s" % src_path)
+    os.chdir(src_path)
+    sys.path.insert(0, src_path)

 import sympy

and even this on top of it:

diff --git a/setup.py b/setup.py
index 235c805..580c7d4 100755
--- a/setup.py
+++ b/setup.py
@@ -45,6 +45,9 @@
     os.system("bin/use2to3")
     os.system("mkdir -p build")
     os.system("mv py3k-sympy %s" % src_path)
+    global __file__
+    __file__ = os.path.join(os.curdir, os.path.basename(__file__))
+
     os.chdir(src_path)
     sys.path.insert(0, src_path)

But unfortunately neither fixes the problem. The core of the problem is that we change the current directory in setup.py (following the NumPy's approach), but for some reason NumPy works, but this code doesn't yet.

TODO

  • Fix the prefix path bug (see above)
  • Update .travis.yml to test this
  • If use2to3 is run manually, then the setup.py should not run 2to3 again (currently it always runs it in Python 3)
  • Test this in all scenarios and make sure it always does the right thing
  • Update use2to3 to also work from a release tarball (currently it needs the git repository)
  • Currently the bin/doctest doc doesn't find any tests in Travis, which is a bug in doctest (#3935)
certik added some commits Jul 8, 2013
@certik certik setup.py: run 2to3 automatically for Python 3 4f75c93
@certik certik setup.py: import sympy after (possible) 2to3 run
In Python 3, we need to call 2to3 first before import sympy, which is needed in
order to obtain the SymPy version.

For this to work, we need to put the translated sources into sys.path.
d9bca77
@certik certik Travis-CI: don't run use2to3 by hand
Instead let setup.py do it automatically.
80917ad
@certik certik Travis-CI: make sure we test the installed version
doctests were executed using the local version, not the installed version
previously. This patch fixes that.
ed0d5d1
@asmeurer
Member
asmeurer commented Jul 8, 2013

What's the point?

@certik
Member
certik commented Jul 8, 2013

It will simplify the installation for many people (and it might also get pip working). Others can continue using the old method.

@jrioux
Member
jrioux commented Jul 8, 2013

I think currently bin/use2to3 assumes that you are working off a git checkout, so it would need to be modified so that the approach here works also as part of a source tarball.

@certik
Member
certik commented Jul 8, 2013

Thanks @jrioux, I've added it into the TODO list.

@asmeurer
Member
asmeurer commented Jul 8, 2013

We need to do it one way or the other, but not both. I personally think the current way works and doesn't need fixing, but I'll yield if the community feels otherwise.

@certik certik Travis: only delete the sympy dir for doctests
But keep it for building documentation, because it is needed for that.
4a5b5da
@certik
Member
certik commented Jul 8, 2013

Aaron, what potential problem do you see in doing both? Doing both seems to me it would fix it for everybody.

@asmeurer
Member
asmeurer commented Jul 8, 2013

Just to be clear, by "doing both" you mean shipping Python 2 code that can be translated to Python 3 via 2to3 and shipping Python 3 code?

@certik
Member
certik commented Jul 8, 2013

By doing both I meant to

  • Allow setup.py to run the 2to3 automatically, so that a way to install sympy in both Python 2 and Python 3 is just python setup.py install. This includes completing all the TODO items in this PR, among other things allowing this to work from the release tarball for Python 2.
  • Allow to run use2to3 separately, so that it produces clean sources for just Python 3. This is very useful, and that's what we currently do.

As far as the release process goes, the Python 2 tarball would work in both Python 2 and 3. But I also see value in providing sources to Python 3 as well, so that users don't have to run the long 2to3 process.

Summary: if we manage to fix everything in this PR, we don't need to modify our release process at all. Ideally all this PR does is that it allows to just use setup.py to install sympy for people that want this, but doesn't change anything for others who don't want this.

@asmeurer
Member
asmeurer commented Jul 8, 2013

As far as the release process goes, the Python 2 tarball would work in both Python 2 and 3. But I also see value in providing sources to Python 3 as well, so that users don't have to run the long 2to3 process.

That's what I don't want to do. It's a waste of time and effort, and puts an unnecessary burden of maintenance on us. We should just pick one way and stick with it. I personally am fine with the current way, but if the community wants to move to the other way, that is fine.

Remember the theme of the release process automation was to remove the maintenance burden on us (which, since it is the release, actually means me) of things that we are doing which are unnecessary. What you want to do here is exactly the opposite.

@asmeurer
Member
asmeurer commented Jul 8, 2013

One thing I wouldn't mind doing is making python3 setup.py install work from git, because that would be about three lines of code and would work in our current framework. I don't really see a need for it, though, so I actually would be slightly -1 to it as well, just because it unnecessarily creates another way to do it.

@certik
Member
certik commented Jul 8, 2013

Yeah, I agree. I just thought about this too after I posted my comment by noticing the fact, that I had to modify our Travis testing for this PR, which means the old way is not tested in this PR. So we would have to test both and I am also 👎 on that, we already test way too many things in Travis.

That being said, I am actually leaning towards this PR. Pros of this PR:

  • Just one way to install sympy, using setup.py, just like any other package. More natural, you don't have to constantly switch directories as a user, etc.
  • It should be possible to make it so that "python3 setup.py sdist" produces just Python 3 sources. We can modify our Travis to always first produce the tarball (sdist), then install it, and then test it. Currently we don't test sdist on Travis, but we really should (no matter if this PR gets in or not), so that we are not surprised at release time.

Pros of our current approach:

  • It already works and is reasonably well tested
  • No hacks in setup.py (though so far the modification introduced by this PR is simple)
@asmeurer
Member
asmeurer commented Jul 8, 2013

you don't have to constantly switch directories as a user

I don't understand what you mean by this.

Currently we don't test sdist on Travis, but we really should (no matter if this PR gets in or not), so that we are not surprised at release time.

How about instead of doing setup.py install we do setup.py sdist and then unpack the tarball and install from there. It will be a little slower (which could be bad, because we already hit the timeout quite often), but it should test this exactly.

@certik
Member
certik commented Jul 8, 2013

you don't have to constantly switch directories as a user

I don't understand what you mean by this.

The user needs to go to the py3k-sympy directory to use it from Python 3.

Currently we don't test sdist on Travis, but we really should (no matter if this PR gets in or not), so that we are not surprised at release time.

How about instead of doing setup.py install we do setup.py sdist and then unpack the tarball and install from there. It will be a little slower (which could be bad, because we already hit the timeout quite often), but it should test this exactly.

This is precisely my suggestion.

@rlamy
Member
rlamy commented Jul 8, 2013

Calling use2to3 at import time of setup.py is way too much of a side-effect. I guess that no existing tool expects it.

@asmeurer
Member
asmeurer commented Jul 8, 2013

The user needs to go to the py3k-sympy directory to use it from Python 3.

You always have to change directories. The only way to not have that is to have a unified codebase, which is way more work than this PR (IMHO, more work than it's worth especially since really no one has even requested this).

Calling use2to3 at import time of setup.py is way too much of a side-effect. I guess that no existing tool expects it.

Maybe you're referring to something Ondrej said that I missed, but what do you mean by import time? I don't think anybody does "import setup" on a package. Doing it at import time of SymPy would indeed be dark magic, and probably a bad idea (though conceivably doable with import hooks). But it didn't seem to me like anyone was suggesting that (but again, maybe I missed it).

@certik
Member
certik commented Jul 8, 2013

Ronan, both NumPy and SciPy used to do this custom translation in setup.py. Also IPython currently calls 2to3 from setup.py. If your objection is to the call os.system(), we can simply put the use2to3 to a module and import it from setup.py, which is for example what the latest release of NumPy does (it imports a module from the tools directory).

But as Aaron said, maybe I misunderstood what you meant.

@certik
Member
certik commented Jul 8, 2013

Aaron:

The user needs to go to the py3k-sympy directory to use it from Python 3.

You always have to change directories. The only way to not have that is to have a unified codebase, which is way more work than this PR (IMHO, more work than it's worth especially since really no one has even requested this).

With this PR, the user only does:

python setup.py install --prefix=somewhere

and he doesn't need to change directories.

@asmeurer
Member
asmeurer commented Jul 8, 2013

But note that that is only the case if you install from git. If you install from the tarball, it is also just that, only for the Python 3 tarball.

@rlamy
Member
rlamy commented Jul 8, 2013

Maybe you're referring to something Ondrej said that I missed, but what do you mean by import time? I don't think anybody does "import setup" on a package.

No, but they can call setup.py more than once (e.g. pip does that, IIRC). There are other commands besides 'install'. What effect it has on them, I don't know, but I guess it cannot be good.

@jrioux
Member
jrioux commented Jul 13, 2013

SymPy Bot Summary: 🔴 Failed after merging certik/py3k (4a5b5da) into master (c9611ed).
@certik: Please fix the test failures.
PyPy 2.0.0-beta-1; 2.7.3-final-42: pass
Python 2.7.2-final-0: pass
🔴 Python 3.2.1-final-0: fail
Sphinx 1.1.3: pass
Doc Coverage: unchanged
36.146% of functions have doctests (compared to 36.146% in master)
41.466% of functions are imported into Sphinx (compared to 41.466% in master)

@asmeurer
Member

This can be closed.

@asmeurer asmeurer closed this Jul 31, 2013
@certik certik deleted the certik:py3k branch Jul 31, 2013
@certik
Member
certik commented Jul 31, 2013

Yes! The solution in this PR bothered me, but ultimately the goal was to fix "setup.py". This is now fixed, in a much better way.

@asmeurer
Member
asmeurer commented Sep 4, 2013

SymPy Bot Summary: Could not fetch the branch certik/py3k.
@certik: Please make sure that certik/py3k has been pushed to GitHub and run the sympy-bot tests again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment