add dirty trick for readline import on OSX #937

Merged
merged 2 commits into from Oct 28, 2011

Conversation

Projects
None yet
2 participants
@minrk
Member

minrk commented Oct 28, 2011

also made the libedit warning extremely loud, so people don't miss it.

We still get reports of people never having noticed the warning, and getting confused when readline is broken on OSX.

The libedit test is also simplified to just check libedit in readline.__doc__, which the official Python docs recommend, rather than the elaborate subprocess/otool method that was not especially reliable.

The reason for the dirty trick

pip installs to site-packages by default, but site-packages dirs always come after lib-dynload (and extras, etc.), which is where the system readline is installed.

That means that a non-setuptools install (pip or setup.py install) cannot override any package that ships with OSX, including:

  • numpy
  • readline
  • twisted
  • pyobjc

without installing to a non-standard path (not even user site-packages via --user).

Note that this path issue not unique to OSX - lib-dynload, and patform packages always come before site-packages, including --user. It's just that OSX seems to be the only platform where there's anything in those locations that you would actually want to supplant.

The method for the dirty trick

  1. remove lib-dynload from sys.path before trying to import readline the first time
  2. after import, restore lib-dynload to its place in sys.path
  3. if import failed without lib-dynload, try it one more time, to get the default module

This doesn't solve everything

It has also come to my attention that EPD and Fink Pythons both ship with broken readline (logical for EPD, given readline's license). However, neither of these report as being anything other than GNU readline, so we can't detect that they are broken.

It's possible that we could print a warning if we detect that we are in EPD and we get to step 3 without readline, but if EPD's readline ever does work, then that message would be misleading, and we would have no real way to know that the issue has been fixed.

The answer remains, in all cases (except for possibly Python.org Python and macports, only because they are untested), that the first command OSX Python users should ever do is easy_install readline. Not just the System Python. After this merge, pip does work to override readline in IPython, but not anywhere else in your Python world, so the warning message does note that pip install will likely not work, even though it does within IPython.

add dirty trick for readline import on OSX
also made the libedit warning extremely loud, so people don't miss it.
We still get reports of people never having noticed the warning, and
getting confused when readline is broken on OSX.

The reason for the dirty trick:

    pip installs to site-packages by default, but site-packages dirs always
    come *after* lib-dynload (and extras, etc.), which is where the system
    readline is installed.

    That means that a non-setuptools install (pip or setup.py install) *cannot*
    override any package that ships with OSX, including:
        numpy, readline, twisted, pyobjc
    without installing to a non-standard path (not even user site-packages via `--user`).

The method for the dirty trick:

    1. remove lib-dynload from sys.path before trying to import readline the first time
    2. after import, restore lib-dynload to its place in sys.path
    3. if import failed without lib-dynload, try it one more time
IPython/utils/rlineimpl.py
+ try:
+ dynload_idx = sys.path.index(lib_dynload)
+ except ValueError:
+ dynload_idx = -1

This comment has been minimized.

@fperez

fperez Oct 28, 2011

Member

I'd use None instead of -1 for the sentinel, since it's a bit more obvious that a None index can never be actually correct. That will make the check below read if dynload_idx is not None, which also reads better (to me).

@fperez

fperez Oct 28, 2011

Member

I'd use None instead of -1 for the sentinel, since it's a bit more obvious that a None index can never be actually correct. That will make the check below read if dynload_idx is not None, which also reads better (to me).

This comment has been minimized.

@minrk

minrk Oct 28, 2011

Member

Sure, I'll do that. It's definitely more Pythonic, I have just gotten used to c-style '-1 index means not found'.

@minrk

minrk Oct 28, 2011

Member

Sure, I'll do that. It's definitely more Pythonic, I have just gotten used to c-style '-1 index means not found'.

IPython/utils/rlineimpl.py
+ # known culprits of this include: EPD, Fink
+ # There is not much we can do to detect this, until we find a specific failure
+ # case, rather than relying on the readline module to self-identify as broken.
+if uses_libedit and sys.platform == 'darwin':

This comment has been minimized.

@fperez

fperez Oct 28, 2011

Member

I'd add a line of whitespace above 94 to separate these two big if blocks for readability

@fperez

fperez Oct 28, 2011

Member

I'd add a line of whitespace above 94 to separate these two big if blocks for readability

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Oct 28, 2011

Member

Other than the minor edits above, I have nothing against the approach. We should probably ping Enthought to ask them to make their readline identifiable in some manner though, I'm sure they could just add a flag to their module to allow us to recognize it.

I haven't run the code myself, but given your OSX expertise and how much you've dealt with this issue, I'm OK if you want to merge once the above tiny things are fixed. Though if you want a second pair of OSX-specific eyes, that's OK too.

Member

fperez commented Oct 28, 2011

Other than the minor edits above, I have nothing against the approach. We should probably ping Enthought to ask them to make their readline identifiable in some manner though, I'm sure they could just add a flag to their module to allow us to recognize it.

I haven't run the code myself, but given your OSX expertise and how much you've dealt with this issue, I'm OK if you want to merge once the above tiny things are fixed. Though if you want a second pair of OSX-specific eyes, that's OK too.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Oct 28, 2011

Member

I do worry about mucking with the path, but I was helping someone out today, explaining why pip doesn't work, and they asked if I could just instruct Python to ignore the system one, allowing the site-packages to be searched for readline, and I realized it actually is possible. I tested it, and it certainly does work - pip-installed readline will be found by IPython, at least with System Python and EPD on my machine.

I honestly don't know why EPD and/or fink readline are broken - we should ask Ilan, or someone. Just this week, I have had people with EPD at py4science and Fink (in #915) report weird readline behavior (not the same), and both found that their respective weirdnesses were resolved by easy_install readline. If it is that they are linked against libedit (fink is not, but EPD might be) and just not reporting it, that would make sense, and it is a bug that their readline.__doc__ is wrong. But I don't know what the default behavior is, when you build Python and readline is not present.

Member

minrk commented Oct 28, 2011

I do worry about mucking with the path, but I was helping someone out today, explaining why pip doesn't work, and they asked if I could just instruct Python to ignore the system one, allowing the site-packages to be searched for readline, and I realized it actually is possible. I tested it, and it certainly does work - pip-installed readline will be found by IPython, at least with System Python and EPD on my machine.

I honestly don't know why EPD and/or fink readline are broken - we should ask Ilan, or someone. Just this week, I have had people with EPD at py4science and Fink (in #915) report weird readline behavior (not the same), and both found that their respective weirdnesses were resolved by easy_install readline. If it is that they are linked against libedit (fink is not, but EPD might be) and just not reporting it, that would make sense, and it is a bug that their readline.__doc__ is wrong. But I don't know what the default behavior is, when you build Python and readline is not present.

STY: rlineimpl review from @fperez
* add some line breaks
* use None to indicate dynload not found, instead of -1
@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Oct 28, 2011

Member

style comments addressed

Member

minrk commented Oct 28, 2011

style comments addressed

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Oct 28, 2011

Member

I'd say go for this one, but a ping to Ilan on enthought-dev would be probably a good idea. Sorry to dump that one on you, but I simply don't know enough about osx to say anything intelligent on such a discussion...

Member

fperez commented Oct 28, 2011

I'd say go for this one, but a ping to Ilan on enthought-dev would be probably a good idea. Sorry to dump that one on you, but I simply don't know enough about osx to say anything intelligent on such a discussion...

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Oct 28, 2011

Member

Awesome, merging now!

Member

fperez commented Oct 28, 2011

Awesome, merging now!

fperez added a commit that referenced this pull request Oct 28, 2011

Merge pull request #937 from minrk/readline
Add dirty trick for readline import on OSX to more aggressively detect the presence of libedit masquerading as true GNU readline.

Also made the libedit warning extremely loud, so people don't miss it.

See the original PR page for the gory details; short version:

1. remove lib-dynload from sys.path before trying to import readline the first time
2. after import, restore lib-dynload to its place in sys.path
3. if import failed without lib-dynload, try it one more time, to get the default module

@fperez fperez merged commit 90b0139 into ipython:master Oct 28, 2011

@minrk minrk referenced this pull request in ludwigschwardt/python-gnureadline Dec 9, 2013

Merged

put readline.so in python_readline.readline #26

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #937 from minrk/readline
Add dirty trick for readline import on OSX to more aggressively detect the presence of libedit masquerading as true GNU readline.

Also made the libedit warning extremely loud, so people don't miss it.

See the original PR page for the gory details; short version:

1. remove lib-dynload from sys.path before trying to import readline the first time
2. after import, restore lib-dynload to its place in sys.path
3. if import failed without lib-dynload, try it one more time, to get the default module
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment