Skip to content

Unbundle external libraries #191

Merged
merged 6 commits into from Mar 24, 2011

5 participants

@tomspur
tomspur commented Oct 29, 2010

This pull request contains an update to a new argparse and the unbundling of the libraries and nothing else anymore ( as requested )...

I read something on the mailing list about "git commit --amend" and to add the # of this pull request, so github will automatically close this.

Are there some notes how exactly that should look like? Or what exactly should I add there with --amend?

@ellisonbg
IPython member

I ran the test suite and looked through the code. I like this approach and the changes are clean, I think this is ready to go. But I know Fernando had been following this one closely, so I want him to at least look over this.

@fperez
IPython member
fperez commented Oct 30, 2010

I also re-ran the test suite both in normal and in installed mode, and it worked fine, so it's pretty much ready to go in. I was about to push though, when I saw that pexpect was left behind without being unbundled. Any particular reason for this? I figured you'd want to unbundle all of externals if possible, no? I'll hold off on the merge/push until you have a chance to let us know why pexpect was separate, or make one more commit that takes care of it.

Once the pexpect question is settled, it's good to go. Thanks!

@tomspur
tomspur commented Nov 2, 2010

I did the unbundling before pexpat was introduced, so I simply missed it.... ;-)

Thanks, for the notice.

@fperez
IPython member
fperez commented Nov 3, 2010

Tom, I'm afraid there are errors left. Once I try to merge your branch into trunk and run the test suite, I get:

Ran 9 test groups in 54.978s

Status:
ERROR - 2 out of 9 test groups failed.
----------------------------------------
Runner failed: IPython.core
You may wish to rerun this one individually, with:
/usr/bin/python /home/fperez/usr/lib/python2.6/site-packages/IPython/testing/iptest.py IPython.core

----------------------------------------
Runner failed: IPython.lib
You may wish to rerun this one individually, with:
/usr/bin/python /home/fperez/usr/lib/python2.6/site-packages/IPython/testing/iptest.py IPython.lib

Here's the full tracebacks:

iptest IPython.core:
...

======================================================================
ERROR: IPython.core.tests.test_formatters.test_pretty
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/pymodules/python2.6/nose/case.py", line 183, in runTest
    self.test(*self.arg)
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/core/tests/test_formatters.py", line 21, in test_pretty
    f.for_type(A, foo_printer)
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/core/formatters.py", line 126, in for_type
    oldfunc = self.type_pprinters.get(typ, None)
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/utils/traitlets.py", line 283, in __get__
    value = self.dynamic_initializer(obj)
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/core/formatters.py", line 90, in _type_pprinters_default
    return pretty._type_pprinters.copy()
AttributeError: 'module' object has no attribute '_type_pprinters'

----------------------------------------------------------------------
Ran 267 tests in 1.844s

FAILED (SKIP=1, errors=1)

and

dreamweaver[junk]> iptest IPython.lib
======================================================================
FAIL: Test the IPython runner.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/lib/tests/test_irunner.py", line 122, in testIPython
    self._test_runner(runner,source,output)
  File "/home/fperez/usr/lib/python2.6/site-packages/IPython/lib/tests/test_irunner.py", line 36, in _test_runner
    self.fail('mismatch in number of lines')
AssertionError: mismatch in number of lines
    """Fail immediately, with the given message."""
>>  raise self.failureException, 'mismatch in number of lines'
    

----------------------------------------------------------------------
Ran 6 tests in 1.200s

FAILED (failures=1)

Once the tests pass, I'll be happy to merge it, but you should always make sure that the test suite passes not only with your branch alone, but once merged, since that's what the reviewers will be testing. Passing on your branch alone isn't sufficient, because what goes into the project ultimately is the merge of your branch into trunk, so that's what must pass the test suite before we can accept it.

@tomspur
tomspur commented Nov 4, 2010

Both are because pretty seems to have changed some functions, so updating could solve this (I'll have a look)...

But thanks for noticing. Right now running the testsuite everytime, won't help here to detect little errors like that, because I have to many failing tests right now (the old fedora problem, whyever...).
I hope to further look around in ipython, to get more knowledge, to finally fix them all, but not right now.... :-(

@tomspur
tomspur commented Nov 5, 2010

The problem is that the pretty.py from ipython differs too much from the upstream pretty.

Till upstream has the needed changes, it would be best, to not unbundle pretty, because it's not possible right now.
The not unbundling happens now in the commit above, but I have here:
FAILED (SKIP=1, failures=2) all from Ipython.core, seem to be the usual fedora failures...

@ellisonbg
IPython member

What is the status of this pull request?

@tomspur
tomspur commented Jan 23, 2011

I need to wait for someone else to run the testsuite, because the fedora one is broken slightly, which is kind of expected (and I don't know why).

After this pull, everything except pretty is unbundled, and pretty can't be unbundled, because it differs to much from upstream...

At least anything else would be ok then.

@ellisonbg
IPython member

If pretty is really our own version now, we should move it to the lib subpackage. That way everything in externals will have upstream versions available.

@tomspur
tomspur commented Jan 24, 2011

It differs too much, that it can be patched somehow, but I don't know if you maybe want to get the changes into upstream...

The biggest changes are in 428050f

I did the rename now and you can choose, which way to go :)

@takluyver takluyver commented on the diff Mar 2, 2011
IPython/external/argparse/__init__.py
@@ -0,0 +1,12 @@
+try:
+ import argparse
+ # Workaround an argparse bug, FIXED in argparse 1.1.0
+ if 'RawTextHelpFormatterArgumentDefaultsHelpFormatter' in argparse.__all__:
+ import itertools
+ argparse.__all__ = list(itertools.chain( [i for i in argparse.__all__
+ if i != 'RawTextHelpFormatterArgumentDefaultsHelpFormatter'],
+ ['RawTextHelpFormatter', 'ArgumentDefaultsHelpFormatter']))
@takluyver
IPython member
takluyver added a note Mar 2, 2011

Is it necessary to use itertools.chain here, rather than just [i for i in ...] + [a, b]?

@abadger
abadger added a note Mar 2, 2011

I wrote the patch originally so tomspur asked me to comment.

AFAICT, there's no reason that the simpler method shouldn't work:

argparse.__all__ = ([i for i in argparse.__all__ 
        if i != 'RawTextHelpFormatterArgumentDefaultsHelpFormatter']
        +  ['RawTextHelpFormatter', 'ArgumentDefaultsHelpFormatter'])

itertools.chain seems like something I'd do while thinking things through and then fail to see that it was unnecessary when submitting the final patch.

@takluyver
IPython member
takluyver added a note Mar 2, 2011

OK, smashing. It just stood out when I was scanning through. Thomas, since it's in your branch, do you want to tweak that? We'll run the tests again, and hopefully it's good to go (unless anyone else sees a reason why not - speak now or forever hold your peace, and all that).

@tomspur
tomspur added a note Mar 3, 2011

I'd leave it like it is.
itertools is a std python lib, so it doesn't add any dependency, so it's ok to use it.
When argparse 1.1.0 is common in all main linux distros, we can simply delete the workaround again.
(And I guess it might be the time right now... Debian, Ubuntu and Fedora has the 1.1. in the repositories, if I see that right (can only be sure for Fedora))

@takluyver
IPython member
takluyver added a note Mar 3, 2011

Ubuntu seems to have 1.1.1 as of Lucid (the release before the current one), which is the first time it packaged it at all. The same version is in Debian stable (they had a new release recently). It looks like macports has 1.1 as well. Assuming the version that's in the stdlib for Python 2.7 and Python 3.2 doesn't have the bug, I think it's safe to remove the workaround.

@abadger
abadger added a note Mar 3, 2011

The EPEL repositories for RHEL 6 have argparse-1.0.1 so I wouldn't characterize it as "safe" to remove. OTOH, argparse-1.1.x looks API compatible so we might be able to upgrade the EPEL packages to a newer version where necessary to get a newer ipython. Definitely a judgement call.

@takluyver
IPython member
takluyver added a note Mar 3, 2011

But is IPython 0.11 (not even released yet) likely to be put onto that platform? I wouldn't make the assumption in the 0.10 series, but I don't think it's unreasonable for the next version of IPython to depend on a version of a package that's been out for at least a year.

@abadger
abadger added a note Mar 3, 2011

tomspur might have a more definite answer since he's involved with maintaining ipython in fedora (on which the EPEL packages are based) -- all I can say for sure is that there's not yet an ipython package for EPEL6 and because users like to get the most recent versions, we generally give them the most recent version at the time that someone steps up to branch and maintain the initial package for that release. Oh, and of course, if users are running RHEL as their workstation (as opposed to a server) they may want to install the latest versions of ipython themselves rather than using what the distro and EPEL provide so they might want to install ipython-0.11 on RHEL5 even.... like I say, though, that could be a case where the EPEL maintainers could be convinced to update argparse to a later version if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fperez
IPython member
fperez commented Mar 13, 2011

Hey tomspur, sorry this has taken so long to get right... But right now it doesn't even apply on top of master:

(tomspur-ready_unbundle)amirbar[ipython]> git pull https://github.com/tomspur/ipython.git ready_unbundle 
remote: Counting objects: 69, done.
remote: Compressing objects: 100% (48/48), done.
remote: Total 62 (delta 22), reused 52 (delta 14)
Unpacking objects: 100% (62/62), done.
From https://github.com/tomspur/ipython
 * branch            ready_unbundle -> FETCH_HEAD
Auto-merging IPython/core/formatters.py
CONFLICT (content): Merge conflict in IPython/core/formatters.py
Automatic merge failed; fix conflicts and then commit the result.

If you can rebase it so it applies cleanly on top of master, we'll try to finish up any remaining questions with this branch and merge it before it goes stale again.

tomspur added some commits Jun 19, 2010
@tomspur tomspur Unbundle of all external modules.
Move all libraries in external into subfolders and place a custom
__init__.py into each of them. This way the system installed ones are
tried first, before falling back to the bundled ones.

Now every distribution can properly depend on the upstream library.
(e.g. bundled libraries are not allowed in fedora)

Signed-off-by: Toshio Ernie Kuratomi <toshio@fedoraproject.org>
Signed-off-by: Thomas Spura <tomspur@fedoraproject.org>
261328d
@tomspur tomspur Update internal argparse to version 1.1
Signed-off-by: Thomas Spura <tomspur@fedoraproject.org>
994a729
@tomspur tomspur Also unbundle pexpect
Signed-off-by: Thomas Spura <tomspur@fedoraproject.org>
21647ae
@tomspur tomspur Don't use the system pretty...
... because upstream pretty does not (yet?) incorporate the changes to
the local _pretty.

Signed-off-by: Thomas Spura <tomspur@fedoraproject.org>
67f9fe7
@tomspur tomspur import from module and not from module.py
Thanks to Justin Riley for noticing.

Signed-off-by: Thomas Spura <tomspur@fedoraproject.org>
b390cc3
@tomspur tomspur Move pretty into lib, because it's heavily changed now. 664ecc2
@tomspur
tomspur commented Mar 13, 2011

Hi fperez,
it should work again.

You still need to decide, if you want to pull the last commit "Move pretty into lib" or if you want to push the changes to pretty back to upstream.

Greetings,
Tom

@fperez fperez merged commit 664ecc2 into ipython:master Mar 24, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.