bookmarks created in 0.11 are corrupt in 0.12 #971

Closed
jdmarch opened this Issue Nov 3, 2011 · 12 comments

Comments

Projects
None yet
5 participants
@jdmarch
Contributor

jdmarch commented Nov 3, 2011

Create a bookmark in 0.11. Start 0.12 and list bookmarks.
In 0.12 qtconsole, the bookmark displays as if its name ended with newline (name on one line, arrow and path on the next)
In 0.12 terminal, it displays just the arrow and path (presumably that second line overwrites the first line which had the name).
In either case, the bookmarks are not usable.

Moreover, if a 0.12 session creates bookmarks and then a 0.11 session opens bookmarks at all (even just to list them), they will then be corrupt in 0.12.

This is significant (beyond the obvious but not critical usefulness of preserving the environment across an upgrade) because an application may embed 0.12 but the user may still have 0.11 installed for direct ipython access, so the 0.12 embed cannot depend on its own bookmarks being usable on a subsequent run. And of course it would also be useful for the two to be able to share bookmarks safely, as they do share history.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Nov 3, 2011

Member

I think this is a blocker: we shouldn't have a regression like that messing up user data. Thanks for the report, @jdmarch. If you make any progress on a fix it would be awesome, I'm swamped at work and will have very little IPython time, if any, for a week or so.

Member

fperez commented Nov 3, 2011

I think this is a blocker: we shouldn't have a regression like that messing up user data. Thanks for the report, @jdmarch. If you make any progress on a fix it would be awesome, I'm swamped at work and will have very little IPython time, if any, for a week or so.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Nov 5, 2011

Member

Based on some quick tests, I think this is Windows-specific, so we should look at any Windows-specific cd/path related changes we may have made since 0.11. @jdmarch, I think you contributed one or two of those, yes?

Member

minrk commented Nov 5, 2011

Based on some quick tests, I think this is Windows-specific, so we should look at any Windows-specific cd/path related changes we may have made since 0.11. @jdmarch, I think you contributed one or two of those, yes?

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Nov 5, 2011

Member

Further specifics: It would appear that any key put into the shell.db in 0.11 comes out in 0.12 with a trailing \r (also affect dhist). I do not see any issues going from 0.12 to 0.11, though. @jdmarch, can you create bookmarks in each, and look directly at: get_ipython().db.get('bookmarks'), and give a sample path that gets messed up from 0.12 to 0.11?

Member

minrk commented Nov 5, 2011

Further specifics: It would appear that any key put into the shell.db in 0.11 comes out in 0.12 with a trailing \r (also affect dhist). I do not see any issues going from 0.12 to 0.11, though. @jdmarch, can you create bookmarks in each, and look directly at: get_ipython().db.get('bookmarks'), and give a sample path that gets messed up from 0.12 to 0.11?

@jdmarch

This comment has been minimized.

Show comment
Hide comment
@jdmarch

jdmarch Nov 5, 2011

Contributor

@minrk No, there is no issue going from 0.12 to 0.11 per se. (0.11 can apparently read anything.) The issue is that if 0.11 accesses a bookmark created in 0.12 (even if only by listing it), and then 0.12 subsequently tries to access it, then it (actually both the key and the value) is corrupted with a trailing \r the same as if it had been created in 0.11.

I or co-workers will try to look at this in next few days.

Contributor

jdmarch commented Nov 5, 2011

@minrk No, there is no issue going from 0.12 to 0.11 per se. (0.11 can apparently read anything.) The issue is that if 0.11 accesses a bookmark created in 0.12 (even if only by listing it), and then 0.12 subsequently tries to access it, then it (actually both the key and the value) is corrupted with a trailing \r the same as if it had been created in 0.11.

I or co-workers will try to look at this in next few days.

@prabhuramachandran

This comment has been minimized.

Show comment
Hide comment
@prabhuramachandran

prabhuramachandran Nov 5, 2011

Contributor

Just an update, I've been trying to reproduce this and just want to add that this is Windows specific. I cannot reproduce this on Mac OS X and can do so on WinXP 64bit.

Contributor

prabhuramachandran commented Nov 5, 2011

Just an update, I've been trying to reproduce this and just want to add that this is Windows specific. I cannot reproduce this on Mac OS X and can do so on WinXP 64bit.

@prabhuramachandran

This comment has been minimized.

Show comment
Hide comment
@prabhuramachandran

prabhuramachandran Nov 5, 2011

Contributor

More updates, I've tracked it down to this:

a2d83e4

the change to pickleshare.py is the culprit. It isn't clear what the best way to work around this going to be. pickle's are supposed to be binary and all standard examples explicitly open a pickle using 'rb' and write as 'wb' (see http://docs.python.org/library/pickle.html).

Contributor

prabhuramachandran commented Nov 5, 2011

More updates, I've tracked it down to this:

a2d83e4

the change to pickleshare.py is the culprit. It isn't clear what the best way to work around this going to be. pickle's are supposed to be binary and all standard examples explicitly open a pickle using 'rb' and write as 'wb' (see http://docs.python.org/library/pickle.html).

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Nov 5, 2011

Member

great! That's precisely where I was looking as well.

So it's a bug in 0.11 and earlier that these things were not binary. Still, the fact that it's always a trailing \r means we should be able to have a relatively simple workaround to just strip that out.

Member

minrk commented Nov 5, 2011

great! That's precisely where I was looking as well.

So it's a bug in 0.11 and earlier that these things were not binary. Still, the fact that it's always a trailing \r means we should be able to have a relatively simple workaround to just strip that out.

@minrk minrk closed this in 8ddc568 Nov 5, 2011

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Nov 5, 2011

Member

It's a tiny one-line fix to use universal-newlines, so I just pushed the fix.

Member

minrk commented Nov 5, 2011

It's a tiny one-line fix to use universal-newlines, so I just pushed the fix.

@prabhuramachandran

This comment has been minimized.

Show comment
Hide comment
@prabhuramachandran

prabhuramachandran Nov 5, 2011

Contributor

@minrk -- thanks for the fix!

Contributor

prabhuramachandran commented Nov 5, 2011

@minrk -- thanks for the fix!

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Nov 6, 2011

Member

Just to ask - has anyone checked the fix with Python 3? I changed the files
to opening in binary mode because you can't pickle to a text-mode file
handle in Python 3. I think Python 3 should just ignore the 'U' flag, but
it's worth checking. I can do this later if you haven't already.

Member

takluyver commented Nov 6, 2011

Just to ask - has anyone checked the fix with Python 3? I changed the files
to opening in binary mode because you can't pickle to a text-mode file
handle in Python 3. I think Python 3 should just ignore the 'U' flag, but
it's worth checking. I can do this later if you haven't already.

@prabhuramachandran

This comment has been minimized.

Show comment
Hide comment
@prabhuramachandran

prabhuramachandran Nov 6, 2011

Contributor

I haven't checked on Python3. It would help if you could, thanks!

Contributor

prabhuramachandran commented Nov 6, 2011

I haven't checked on Python3. It would help if you could, thanks!

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Nov 6, 2011

Member

Yes, I tested with Python 3, and it seemed fine.

Member

minrk commented Nov 6, 2011

Yes, I tested with Python 3, and it seemed fine.

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

use universal-newline in pickleshare
This prevents entries (bookmarks) made in 0.11 on Windows from
having an extra '\r' on all strings.

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