Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2019.2: Revert "Update liblouis to version 3.10 (#9678)" #10232

Closed
wants to merge 2 commits into from

Conversation

leonardder
Copy link
Collaborator

@leonardder leonardder commented Sep 17, 2019

This reverts commit d5ed95c.

Link to issue number:

Fixes #9982

Summary of the issue:

Large chinese braille tables are unable to load in some cases when using Liblouis 3.10. Downgrading to Liblouis 3.9 seems to fix this according to #9982 (comment)

Description of how this pull request fixes the issue:

Downgrades to liblouis 3.9 by reverting the 3.10 pr

Testing performed:

Tested in a try build that the issues for Chinese no longer occurred.

Known issues with pull request:

Change log entry:

@leonardder leonardder requested a review from michaelDCurran Sep 17, 2019
@josephsl
Copy link
Collaborator

josephsl commented Sep 17, 2019

@leonardder
Copy link
Collaborator Author

leonardder commented Sep 17, 2019

@josephsl
Copy link
Collaborator

josephsl commented Sep 17, 2019

@michaelDCurran michaelDCurran changed the title 2018.2: Revert "Update liblouis to version 3.10 (#9678)" 2019.2: Revert "Update liblouis to version 3.10 (#9678)" Sep 17, 2019
@michaelDCurran
Copy link
Member

michaelDCurran commented Sep 17, 2019

I'm really not sure what we should do about this yet. As Liblouis 3.10 does fix some buffer overruns. Knowingly moving back to a release that introduces them is strange. Yet it obviously fixes the Chinese crash... Perhaps I'll spend a few hours debugging the chinese issue and see if we can provide a patch. If not we'll downgrade.

@lukaszgo1
Copy link
Contributor

lukaszgo1 commented Sep 17, 2019

It would be nice not to reintroduce #9486 if at all possible. Note that #9486 happens only when Liblouis dll's are optimized, so in the worth case it may be possible to use Liblouis 3.9, and if it breaks Korean disable optimization for it.

@Qchristensen
Copy link
Member

Qchristensen commented Sep 18, 2019

Would upgrading to LibLouis 3.11, rather than downgrading, resolve both the large Chinese table issue and the previous buffer overrun issue? http://liblouis.org/liblouis/2019/09/02/release-3.11.0.html

Or is that too unknown a change?

@leonardder
Copy link
Collaborator Author

leonardder commented Sep 18, 2019

There is an issue about updating to liblouis 3.11 (#10161), but that one is either blocked by an update to Visual Studio 2019, or bundling a binary build produced by the liblouis folks.

@leonardder
Copy link
Collaborator Author

leonardder commented Sep 18, 2019

It would be nice not to reintroduce #9486 if at all possible. Note that #9486 happens only when Liblouis dll's are optimized, so in the worth case it may be possible to use Liblouis 3.9, and if it breaks Korean disable optimization for it.

Liblouis 3.9 is likely to reintroduce this, indeed, so this pull request is certainly not ready as it is now.

@DrSooom
Copy link

DrSooom commented Sep 18, 2019

Comparing result between NVDA 2019.1.1 and 2019.2:

ko.cti – Line 64:

include chardefs.cti All character definition opcodes
» include en-chardefs.cti English character definition opcodes

ko-g2.ctb – New comment at line 45, rest is identical.

ko-chars.cti, ko-g1.ctb, ko-g1-rules.cti and ko-g2-rules.cti have the same SHA-256 checksum.

And please don't forget to read this comment here and this one here as well. There is no answer why the whole thing worked as expected with NVDA 2019.2 RC1 yet.

@dpy013
Copy link
Contributor

dpy013 commented Sep 23, 2019

hello
So is the problem with this pr completely solved?
thanks

This reverts commit d5ed95c.

Link to issue number:

Fixes #9982

Summary of the issue:

Large traditional Chinese braille tables are unable to load in some cases when using Liblouis 3.10. Downgrading to Liblouis 3.9 seems to fix this according to #9982 (comment)

Description of how this pull request fixes the issue:

Downgrades to liblouis 3.9 by reverting the 3.10 pr

Testing performed:

Tested in a try build that the issues for traditional Chinese no longer occurred.

Known issues with pull request:

  • I had a branch that was based on rc, and when I rebased on beta, I noticed that there are commits in rc not in beta.
  • In #9982 (comment), @josephsl expressed concerns about a downgrade
  • We probably have to be very careful when merging beta into master.

Change log entry:

hi Collaborator
I replaced the previous Chinese with the traditional Chinese. In the issue, the test is based on traditional Chinese braille.

@dpy013
Copy link
Contributor

dpy013 commented Sep 23, 2019

In Chinese, it is divided into Simplified Chinese (zh_CN) and Traditional Chinese (zh_TW)
issue 9982
Test braille is zh_TW

@Adriani90
Copy link
Collaborator

Adriani90 commented Sep 24, 2019

To be honnest, I am not really understanding what exactly the user impact will be 9in practice. We need more testing results from chinese users on this. @josephsl do you have any contacts we could use for this purpose? I would rather update to liblouis 3.11 and try to solve this issue after the user impact is assessed. Maybe it is not really a liblouis issue. In fact, there are people who cannot reproduce this issue under the same conditions. Could someone maybe post this to the NVDA users list and try to get some clearer test results?

@michaelDCurran
Copy link
Member

michaelDCurran commented Sep 24, 2019

Due to the inability to reproduce this, confusion around how many users are really affected, and no suitable solution being found which does not reintroduce other errors, this will not be in 2019.2.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BabbageWork
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants