Fix beta build by better clearly distinguishing between the main nvda launcher and the l10nutil exe files #17249
Conversation
…code to look for the main nvda launcher as nvda_*.exe.
WalkthroughThe changes in this pull request involve modifications to several scripts and documentation related to the NV Access project. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
appveyor/scripts/deployScript.ps1 (1)
11-11: Approve the change with a minor suggestion for clarityThe modification to select only
nvda_*.exefiles effectively addresses the issue mentioned in the PR objectives. This change ensures that only the main NVDA launcher is processed during deployment, preventing conflicts with other executables like the localization utility.To further improve clarity and maintainability, consider using a more descriptive variable name.
$mainNvdaExe = Get-ChildItem -Name output\nvda_*.exeThis change makes the purpose of the variable more explicit throughout the rest of the script.
projectDocs/translating/crowdin.md (1)
197-197: LGTM: Grammatical correction applied.The change from "When ever" to "Whenever" is grammatically correct and improves the readability of the document.
For even better clarity, consider rephrasing the sentence to:
After saving the xliff file with Poedit, you can use the NVDA l10nUtil program to generate the html version of the documentation file. For example:This phrasing makes the sequence of actions clearer and avoids the potential ambiguity of "Whenever".
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- appveyor/scripts/deployScript.ps1 (1 hunks)
- projectDocs/translating/crowdin.md (2 hunks)
- sconstruct (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
appveyor/scripts/deployScript.ps1 (1)
Pattern
**/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.:suggestion <code changes>projectDocs/translating/crowdin.md (2)
Pattern
**/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.:suggestion <code changes>
Pattern
**/*.md: Focus on grammar, spelling, and punctuation. Also consider readability and clarity of contents. Ensure that changes follow the style of one sentence per line, suggest changes if this is not met.sconstruct (1)
Pattern
**/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.:suggestion <code changes>
🔇 Additional comments (5)
appveyor/scripts/deployScript.ps1 (1)
Line range hint
1-52: Overall impact of the change is positive and aligned with PR objectivesThe modification to select only
nvda_*.exefiles effectively addresses the issue mentioned in the PR objectives without introducing any negative side effects to the rest of the script. The change is well-contained and doesn't conflict with other deployment tasks such as Crowdin sync or Mozilla symbol upload.The script's overall structure and functionality remain intact, with the improvement focused on the NV Access server notification part. This change should resolve the beta build failure mentioned in the PR description.
To further enhance the script:
- Consider implementing the suggested error handling for file selection.
- You might want to add a comment explaining the reason for the specific file pattern, which could be helpful for future maintenance.
These suggestions are minor improvements to an already effective solution.
projectDocs/translating/crowdin.md (3)
172-172: LGTM: Executable name updated correctly.The change from
nvdaL10nUtiltol10nUtil.exeis consistent with the PR objectives and improves clarity by distinguishing this utility from the main NVDA launcher.
199-199: LGTM: Executable name updated consistently.The change from
nvdaL10nUtiltol10nUtil.exeis consistent with the previous update and maintains consistency throughout the document.
Line range hint
1-199: Summary: Documentation updated successfully.The changes in this file accurately reflect the renaming of
nvdaL10nUtil.exetol10nUtil.exeas per the PR objectives. This update improves clarity by distinguishing the localization utility from the main NVDA launcher. The grammatical correction from "When ever" to "Whenever" enhances readability.These changes will help translators use the correct utility name in their workflow and improve their understanding of the translation process. The document maintains its overall structure and clarity while providing up-to-date information.
sconstruct (1)
739-739: Update to nvdaL10nUtil command aligns with PR objectives.The change from
nvdaL10nUtil.pytol10nUtil.pyin the Nuitka command aligns with the PR's objective of renaming the localization utility. This modification ensures that the correct file is used when generating the standalone executable.However, to maintain consistency and clarity:
- Consider updating the target file name from
nvdaL10nUtil.exetol10nUtil.exeto match the new source file name.- Update any references to this utility throughout the codebase to reflect the new name.
To ensure all references are updated, run the following script:
This will help identify any places where the old name might still be used, ensuring a complete and consistent renaming process.
✅ Verification successful
Correction Needed in Shell Script File Types
The previous attempt to search for 'nvdaL10nUtil' failed due to an unrecognized file type. Please use the correct file types for Python scripts.
Update Shell Script to Correctly Target NSIS Files
The previous script failed because
nsisis not a recognized file type inrg. Please use a glob pattern to target.nsifiles.
All references to 'nvdaL10nUtil' have been successfully updated.
No remaining references found in Python and NSIS files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to 'nvdaL10nUtil' rg --type python --type nsis 'nvdaL10nUtil'Length of output: 74
Script:
#!/bin/bash # Search for any remaining references to 'nvdaL10nUtil' rg --type py --type nsis 'nvdaL10nUtil'Length of output: 68
Script:
#!/bin/bash # Search for any remaining references to 'nvdaL10nUtil' in Python and NSIS files rg --type py --glob '*.nsi' 'nvdaL10nUtil'Length of output: 42
See test results for failed build of commit fa2c6a0632 |
Probably fix-up of #17214. Follow-up of #17249. Summary of the issue: When an appVeyor test failure message is issued, the build link in this message is on l10nUtil.exe rather than on nvda_snapshot_xxxx.exe. Description of user facing changes The NVDA snapshot executable will be again downloadable from appVeyor messages. Description of development approach Refine the filtering to find the snapshot executable on "nvda_.exe" rather than on ".exe", as done in the deploy script in #17249 in appVeyor script.
Link to issue number:
None.
See failed beta build: https://ci.appveyor.com/project/NVAccess/nvda/builds/50731013
Summary of the issue:
NVDA's appVeyor diploy code finds the main NVDA launcher by simply matching on output*.exe. This is expected to match on a single file.
However, with the introduction of the nvdaL10nUtil.exe to the output directory, the pattern also matched on this file, causing the deploy to fail.
Description of user facing changes
None
Description of development approach
output\nvda_*.exe.Testing strategy:
This can only be tested via a successful beta build.
Known issues with pull request:
Code Review Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation