Skip to content

Ensure pickle files can be read by earlier versions of NVDA (#7105) - #10255

Merged
michaelDCurran merged 1 commit into
nvaccess:masterfrom
accessolutions:i7105-pickleProtocolVersion
Sep 23, 2019
Merged

Ensure pickle files can be read by earlier versions of NVDA (#7105)#10255
michaelDCurran merged 1 commit into
nvaccess:masterfrom
accessolutions:i7105-pickleProtocolVersion

Conversation

@JulienCochuyt

Copy link
Copy Markdown
Contributor

Impacts both the add-ons state and the updater state files.
Both are now written using pickle protocol 0 so that Python 2 version of NVDA
can read them in case of downgrading.

Re #10224 (comment)

Link to issue number:

Pointed out in #10224 (comment)

Summary of the issue:

NVDA 2019.2 cannot read pickle files written by latest Python 3 master.
When testing with the default user configuration, this as the potential nasty effect of activating add-ons that were manually disabled.

Description of how this pull request fixes the issue:

Use pickle protocol 0 instead of the default protocol 3.
This has the advantage of being readable by earlier versions of NVDA that open files in text mode before unpickling.
Both the add-ons state and the updater state files are impacted.

Testing performed:

Ran a source copy, pointing to the installed 2019.2 configuration.
Checked that manually disabled compatible add-ons were still indeed marked as such.
Manually disabled a running add-on.
Saved the configuration, and launched installed 2019.2 to check the add-on state was preserved.

Known issues with pull request:

Impacting the updater state file is maybe superfluous in this context.
For the better or the worse, it makes the updater behave the same when sharing a configuration between 2019.2 and master as between any other versions.

Change log entry:

I don't think this deserves to be announced.

cc @LeonarddeR

…#7105)

Impacts both the add-ons state and the updater state files.
Both are now written using pickle protocol 0 so that Python 2 version of NVDA
can read them in case of downgrading.

Re nvaccess#10224 (comment)
@LeonarddeR

Copy link
Copy Markdown
Collaborator

I'm a bit worried by the fact thaht this now pickles in protocol 0, which is by far the oldest protocol available and superseded for a reason. More importantly though, are we sure that python 2 versions pickled data using protocol 0? As far as I know, protocol 2 is the default for Python 2.7.

@JulienCochuyt

JulienCochuyt commented Sep 21, 2019

Copy link
Copy Markdown
Contributor Author

I'm a bit worried by the fact thaht this now pickles in protocol 0, which is by far the oldest protocol available and superseded for a reason. More importantly though, are we sure that python 2 versions pickled data using protocol 0? As far as I know, protocol 2 is the default for Python 2.7.

Legitimate concerns.
Protocol 0 is supported by all Python releases since the introduction of the pickle module, and is not meant to be deprecated any soon.
It has been superseded mainly to better handle new style classes and big objects hierarchy, which is of no concern in our present use case.
As being the sole text-based protocol, it is the best candidate we have to support both ways compatibility as our Python 2 code reads files in text modes while our Python 3 code reads them in binary mode.
Furthermore, my backwards and forward compatibility tests are all successful. I can only encourage you to please try to reproduce them.

As a reference:

@LeonarddeR

LeonarddeR commented Sep 21, 2019 via email

Copy link
Copy Markdown
Collaborator

@JulienCochuyt

JulienCochuyt commented Sep 21, 2019

Copy link
Copy Markdown
Contributor Author

Strictly spoken, I think the only difference between NVDA python 2 and NVDA python 3 pickled files after this pr will be the line endings, as we will now open them in binary mode instead.

This is actually not true.
NVDA Python 2 luckily happen to write the pickle files in binary mode, despite reading them in text mode.
Thus, it produces protocol 0 files with LF Unix-style line-endings, just like Python 3 does.
The line-ending would be CRLF Windows-style only if it wrote in text mode.
It's even quite strange that this historical inconsistency in the NVDA Python 2 code does not cause any issue with protocol 0.

if we want to be 100% equal, we might have to consider opening them in text mode again.

This is not supported by Python 3. Its pickle module only handles binary mode, unlike cPickle in Python 2.

In conclusion, as long as you run on Windows and stick to protocol 0, you can safely exchange pickle files between Python 2 and 3, as long as the Python 2 implementation writes in binary mode.

@LeonarddeR

LeonarddeR commented Sep 23, 2019 via email

Copy link
Copy Markdown
Collaborator

@michaelDCurran

Copy link
Copy Markdown
Member

In principle this all sounds okay to me. However, it would be good to note here what might happen to all the people who are currently on alpha builds if this pr is merged. Will they experience any breakage as they already have pickl version 3 files?

@LeonarddeR

Copy link
Copy Markdown
Collaborator

Will they experience any breakage as they already have pickl version 3 files?

No. The pickle module automatically detects the protocol of pickled files when unpicking. The protocol is only specified when pickling. This means that when updating to an Alpha after merging this pr will load the version 3 files just fine and, when repickling, the files will be protocol 0.

@michaelDCurran
michaelDCurran merged commit e9cfa1b into nvaccess:master Sep 23, 2019
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Sep 23, 2019
@JulienCochuyt
JulienCochuyt deleted the i7105-pickleProtocolVersion branch September 23, 2019 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants