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

Update espeak-ng to 1.51-dev commit cad1c8e8 #12280

Merged
merged 5 commits into from
Apr 9, 2021
Merged

Conversation

feerrenrut
Copy link
Contributor

@feerrenrut feerrenrut commented Apr 7, 2021

Link to issue number:

None

Summary of the issue:

Several issues in espeak have been fixed, espeak devs have asked for feedback.

Description of how this pull request fixes the issue:

Updates espeak to commit cad1c8e8
Also adds a readme for updating espeak submodule, sourced from:
https://github.com/nvaccess/nvda/wiki/Updating-eSpeak-NG

Testing strategy:

Build and run locally

Known issues with pull request:

The added readme duplicates the instructions found on the wiki: https://github.com/nvaccess/nvda/wiki/Updating-eSpeak-NG
I think it is more helpful to have these in the repository, if get agreement on this, I'll update the wiki to refer to this readme.

Change log entry:

See changes file.

Code Review Checklist:

This checklist is a reminder of things commonly forgotten in a new PR.
Authors, please do a self-review and confirm you have considered the following items.
Mark items you have considered by checking them.
You can do this when editing the Pull request description with an x: [ ] becomes [x].
You can also check the checkboxes after the PR is created.

  • Pull Request description is up to date.
  • Unit tests.
  • System (end to end) tests.
  • Manual tests.
  • User Documentation.
  • Change log entry.
  • Context sensitive help for GUI changes.

@AppVeyorBot

This comment has been minimized.

Copy link
Member

@michaelDCurran michaelDCurran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with the update instructions to be included, but I disagree with it being in include. I think it should be in devDocs or another directory, otherwise it looks a bit too important for the general user of the repository and may be misleading.

@feerrenrut
Copy link
Contributor Author

I did consider putting it in devDocs, but I wanted to make it easy to find when trying to update the espeak submodule. I figured proximity would help.

@feerrenrut
Copy link
Contributor Author

I also hoped the naming of the file updating-espeak.md would help to describe its limited scope.

@@ -138,6 +138,7 @@ espeakLib=env.SharedLibrary(
"phonemelist.c",
"readclause.c",
"setlengths.c",
"soundIcon.c",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how did you know to add this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updating-espeak.md should cover.. or at least hint at this. The source espeak project is built from Makefile.am, I diffed this file (last commit we used vs new commit).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could clarify that in the md file?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right so it is only if the build fails?

I was expecting this in the "doing the update"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right so it is only if the build fails?

Good point, I guess the answer is "it depends". The build may not always fail, we explicitly exclude certain modules of espeak that we don't use. However this is only possible because of the way that these modules are implemented. I'd say it is a good idea to always do that diff and make a decision about whether changes need to be made.

@feerrenrut feerrenrut merged commit 49a8578 into master Apr 9, 2021
@feerrenrut feerrenrut deleted the updateEspeak branch April 9, 2021 07:15
@nvaccessAuto nvaccessAuto added this to the 2021.1 milestone Apr 9, 2021
@feerrenrut feerrenrut self-assigned this Apr 9, 2021
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.

None yet

5 participants