Skip to content

Update configobj to 5.1.0dev#7945

Merged
feerrenrut merged 2 commits into
nvaccess:masterfrom
LeonarddeR:configobj
Aug 13, 2018
Merged

Update configobj to 5.1.0dev#7945
feerrenrut merged 2 commits into
nvaccess:masterfrom
LeonarddeR:configobj

Conversation

@LeonarddeR

@LeonarddeR LeonarddeR commented Jan 29, 2018 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Link to issue number:

Closes #4470

Summary of the issue:

configobj is currently on version 4.6.0, which is pretty old. The current stable version is 5.0.6, it is said to add full support for Python 3, but later commits suggest that support had to be improved for version 3.4 and 3.5. Also, version 5.0.6 contained the bug as described in #7945 (comment), it didn't allow executing from validate import *.

Description of how this pull request fixes the issue:

Adds a configobj submodule to the repository on github, and uses its current master version in favor of the version from miscdeps, which can be removed if this gets eventually merged.

Testing performed:

Ran NVDA from source, the configuration persisted. Also, ran tests as proposed by @feerrenrut, see #7945 (comment) for results.

Known issues with pull request:

I've seen a case where NVDA did restore its configuration to factory defaults, but that might have been because of me fiddling with the config in a majorly destructive way. This was in the 5.0.6 situation, and I don't have steps to reproduce.

@josephsl

josephsl commented Jan 29, 2018 via email •
edited by feerrenrut
Loading

Copy link
Copy Markdown
Contributor

@LeonarddeR

This comment has been minimized.

Sign in to view
@josephsl

This comment has been minimized.

Sign in to view
@feerrenrut

feerrenrut commented Apr 11, 2018

Copy link
Copy Markdown
Contributor

Please also test the configuration file upgrade paths.

@josephsl

This comment has been minimized.

Sign in to view
@LeonarddeR

LeonarddeR commented Apr 30, 2018

Copy link
Copy Markdown
Collaborator Author

@feerrenrut commented on 11 Apr 2018, 03:46 CEST:

Please also test the configuration file upgrade paths.

Do you have an idea about how to properly test this?

@feerrenrut

feerrenrut commented May 1, 2018

Copy link
Copy Markdown
Contributor

Please also test the configuration file upgrade paths.

Essentially, get hold of an old config file (pre introduction of schema version), copy to user config directory, and start NVDA. Inspect the config file and log for errors.
It's probably wise to do this with a blank, "default", config. As well as with one that has been populated with a variety of options.
The first version with the schema upgrade process was 2017.1 (introduced with #6558), so starting from a config file out of 2016.4 should suffice.

@dkager

This comment has been minimized.

Sign in to view
@josephsl

josephsl commented Jun 18, 2018

Copy link
Copy Markdown
Contributor

Hi,

I propose queueing this for review after wxPython 4 is merged into master. In the meantime, can you merge current master so Appveyor can present status updates? Thanks.

@LeonarddeR LeonarddeR force-pushed the configobj branch 2 times, most recently from c1fea4d to 5721759 Compare July 11, 2018 10:35
@LeonarddeR

LeonarddeR commented Aug 6, 2018

Copy link
Copy Markdown
Collaborator Author

@feerrenrut commented on 11 Apr 2018, 03:46 CEST:

Please also test the configuration file upgrade paths.

How would you like me to do this?

@LeonarddeR LeonarddeR changed the title Update configobj to 5.0.6 Update configobj to 5.1.0dev Aug 6, 2018
@feerrenrut

feerrenrut commented Aug 7, 2018

Copy link
Copy Markdown
Contributor

@LeonarddeR Essentially:

  1. Get hold of an old config file (pre introduction of schema version)
  2. Copy the old config file to user config directory
  3. Start NVDA with --debug-logging, save config, and quit NVDA
  4. Inspect the config file (compare to old config file) and inspect the log for errors.

It's probably wise to do this with a blank (default) config as well as with one that has been populated with a variety of options, focusing on the options that have upgrade steps (see profileUpgradeSteps.py)

The first version with the schema upgrade process was 2017.1 (introduced with #6558), so starting from a config file out of 2016.4 should suffice.

I understand manually doing this is a little onerous, see #7220

@LeonarddeR

LeonarddeR commented Aug 11, 2018

Copy link
Copy Markdown
Collaborator Author

@feerrenrut: I've been able to perform the tests, here are the results.

This is what I did.

  1. Download NVDA 2016.4
  2. Changed basically every setting that can be changed from within the gui, including those that are involved in scheme upgrades.
  3. Ran this branch from source with the particular config file.
  4. After upgrading, all seemed well with the new config file.

@LeonarddeR LeonarddeR requested a review from feerrenrut August 11, 2018 12:27
@feerrenrut feerrenrut merged commit cbaca42 into nvaccess:master Aug 13, 2018
@nvaccessAuto nvaccessAuto added this to the 2018.3 milestone Aug 13, 2018
@LeonarddeR

LeonarddeR commented Aug 13, 2018

Copy link
Copy Markdown
Collaborator Author

Ugh, I was quite sure that I included a changes.t2t update as part of this, but turns out i didn't.

@LeonarddeR LeonarddeR mentioned this pull request Aug 13, 2018
Closed
@feerrenrut

feerrenrut commented Aug 13, 2018

Copy link
Copy Markdown
Contributor

I wondered about a changes file update. I didn't think it was strictly necessary, but now looking at the original issue again there does seem to be a few changes that would be good to point out. Could you create another PR to handle that.

This was referenced Aug 13, 2018
Closed
Merged
feerrenrut pushed a commit that referenced this pull request Aug 14, 2018
@LeonarddeR @feerrenrut
Second attempt to update to configobj 5.1.0dev (PR #8626)
5952d0f 
Really use the new configobj this time, and add proper logging for configobj version when starting NVDA for this to be more easily verified. For add-ons or other code that might be still using validate directly, a deprecation warning has been added.

Closes #4470
Implements #7945
Supersedes #8622
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@feerrenrut feerrenrut feerrenrut approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2018.3

Development

Successfully merging this pull request may close these issues.

Upgrade to ConfigObj 5.x.y

5 participants

@LeonarddeR @josephsl @feerrenrut @dkager @nvaccessAuto