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

Make it easier for devs outside of NV Access to utilize the Appveyor build process #16221

Merged
merged 17 commits into from Mar 7, 2024

Conversation

XLTechie
Copy link
Collaborator

@XLTechie XLTechie commented Feb 24, 2024

Link to issue number:

Closes #16216
Fixed #8806 (Issue discovered after merge)

Summary of the issue:

Currently, for a developer to make a private AppVeyor build of NVDA, requires maintaining different build scripts, in addition to an intricately modified appveyor.yml file, and feeding them to AppVeyor on every build.

This PR removes much of the difficulty from private AppVeyor builds, by making the build process more configurable in appveyor.yml, without having to modify the build scripts at all to make a basic build work.

In addition, it adds a few sensible state checks to a couple build scripts.

It tries to avoid changing any of the existing logic, except where an if condition could be anded with another to achieve the desired result.

Description of user facing changes

None

Edit after merge: developer usage documentation can now be found in project_docs/dev/buildingNVDAOnAppVeyor.md.

Description of development approach

  • appveyor.yml:
    • Move the environment section above the init section.
    • Added a scons_publisher variable, to avoid hard-coding "NV Access" in the scons variable script. Those doing private builds should change it to reflect their (org) name.
    • Added a feature subsection to the environment section. Included features to control:
      • building symbols,
      • Uploading symbols to Mozilla,
      • building the appx,
      • crowdinSync, and
      • package signing.
    • (For private builds, all of those features should be commented out, except, optionally, the one for building symbols.)
    • Added logic to check the setting of the buildSymbols feature before building symbols.
  • appveyor/scripts/buildSymbolStore.ps1: Added a test for feature_buildSymbols.
  • appveyor/scripts/deployScript.ps1:
    • Make symbol upload dependent on both the buildSymbols and uploadSymbolsToMozilla features being set in appveyor.yml.
    • Cause crowdin synchronization to be dependent on its feature from appveyor.yml.
    • Only deploy to the NV Access server, if it's an nvaccess owned repo.
  • appveyor/scripts/setSconsArgs.ps1:
    • Use the scons_publisher variable from appveyor.yml to designate the publisher, instead of hardcoding in the script.
    • Don't include cert options if not signing based on feature in appveyor.yml.
    • Respond to the buildAppx feature in appveyor.yml.
  • appveyor/scripts/pushPackagingInfo.ps1: Change the appVeyor message with the exe link to mention a "branch" instead of a "PR", if no PR number is set in the build. It is assumed that most private builds will be for branches, not PRs against their own clone.
  • appveyor/scripts/decryptFilesForSigning.ps1: Do not attempt to decrypt files if signing feature is not set in appveyor.yml.
  • appveyor/scripts/logCiTiming.ps1: added a test to exit successfully if the timing record file is not found.

Regarding the last: originally I included a feature to disable CI timing checks. I decided that feature was unnecessary so reverted it, but I kept the change to detect the missing timing file and silently exit rather than failing hard, since it makes sense that something else may cause that file not to exist, and we wouldn't want the build failing over it.

Testing strategy:

Built several versions of NVDA with various combinations of the features disabled, and reached a successful build. Enabling any of them (except buildSymbols) on a non-NV Access build, causes a failure of one kind or another, only sometimes resulting in an executable being made. This is as expected and intended.

This PR built successfully in an NV Access configured environment.

Known issues with pull request:

None

Code Review Checklist:

  • Documentation:
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@XLTechie
Copy link
Collaborator Author

Regarding the developer/technical documentation point in the documentation checkbox above: I already have the developer documentation from my previous how-to on doing Appveyor builds of NVDA. I would need only to simplify it to account for these changes, and could then either contribute it to the WIKI, the non-wiki docs, or make it part of the developer guide (which I do not think is the correct approach).

Regardless of where it should go, a documentation PR will come shortly after this one, if this work is successful and approved.

@XLTechie XLTechie marked this pull request as ready for review February 24, 2024 09:38
@XLTechie XLTechie requested a review from a team as a code owner February 24, 2024 09:38
@XLTechie XLTechie marked this pull request as draft February 27, 2024 20:30
@XLTechie XLTechie force-pushed the fix16216 branch 2 times, most recently from 85e66d1 to 2db35e6 Compare February 29, 2024 07:32
@XLTechie XLTechie marked this pull request as ready for review February 29, 2024 07:34
@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Mar 5, 2024
Luke Davis added 16 commits March 6, 2024 15:26
…Included features to control building symbols, uploading symbols, building the appx, crowdinSync, and signing.
…efore each timing write."

Decided that this was a harmless feature, and builders with Appveyor could just put up with it if they donn't like it.
This reverts commit 3429f2d04.
… configured to build and upload in appveyor.yml.
…e dependent on its feature from appveyor.yml.
…publisher variable to designate the publisher, instead of hardcoding in the script.
…ith the exe link to mention a "branch" instead of a "PR", if no PR number.
…t files if signing feature is not set in appveyor.yml.
@AppVeyorBot
Copy link

See test results for failed build of commit c849cfc5d0

@XLTechie
Copy link
Collaborator Author

XLTechie commented Mar 6, 2024 via email

appveyor/scripts/deployScript.ps1 Outdated Show resolved Hide resolved
user_docs/en/changes.t2t Outdated Show resolved Hide resolved
@seanbudd
Copy link
Member

seanbudd commented Mar 7, 2024

For documentation, I would encourage a new file in projectDocs\dev\CICD.md linked from projectDocs\dev\contributing.md and projectDocs\dev\readme.md

@seanbudd seanbudd merged commit aa3abfd into nvaccess:master Mar 7, 2024
1 check was pending
@nvaccessAuto nvaccessAuto added this to the 2024.2 milestone Mar 7, 2024
@XLTechie
Copy link
Collaborator Author

XLTechie commented Mar 7, 2024 via email

@XLTechie
Copy link
Collaborator Author

XLTechie commented Mar 11, 2024 via email

@seanbudd
Copy link
Member

Sure thing, the name was just a suggestion

XLTechie pushed a commit to XLTechie/xlnvda that referenced this pull request Mar 12, 2024
XLTechie pushed a commit to XLTechie/xlnvda that referenced this pull request Mar 13, 2024
Adriani90 pushed a commit to Adriani90/nvda that referenced this pull request Mar 13, 2024
…build process (nvaccess#16221)

Closes nvaccess#16216

Summary of the issue:
Currently, for a developer to make a private Appveyor build of NVDA, requires maintaining different build scripts, in addition to an intricately modified appveyor.yml file, and feeding them to Appveyor on every build.

This PR removes much of the difficulty from private Appveyor builds, by making the build process more configurable in appveyor.yml, without having to modify the build scripts at all to make a basic build work.

In addition, it adds a few sensible state checks to a couple build scripts.

It tries to avoid changing any of the existing logic, except where an if condition could be anded with another to achieve the desired result.

Description of user facing changes
None

Description of development approach
appveyor.yml:
Move the environment section above the init section.
Added a scons_publisher variable, to avoid hard-coding "NV Access" in the scons variable script. Those doing private builds should change it to reflect their (org) name.
Added a feature subsection to the environment section. Included features to control:
building symbols,
Uploading symbols to Mozilla,
building the appx,
crowdinSync, and
package signing.
(For private builds, all of those features should be commented out, except, optionally, the one for building symbols.)
Added logic to check the setting of the buildSymbols feature before building symbols.
appveyor/scripts/buildSymbolStore.ps1: Added a test for feature_buildSymbols.
appveyor/scripts/deployScript.ps1:
Make symbol upload dependent on both the buildSymbols and uploadSymbolsToMozilla features being set in appveyor.yml.
Cause crowdin synchronization to be dependent on its feature from appveyor.yml.
Only deploy to the NV Access server, if it's an nvaccess owned repo.
appveyor/scripts/setSconsArgs.ps1:
Use the scons_publisher variable from appveyor.yml to designate the publisher, instead of hardcoding in the script.
Don't include cert options if not signing based on feature in appveyor.yml.
Respond to the buildAppx feature in appveyor.yml.
appveyor/scripts/pushPackagingInfo.ps1: Change the appveyor message with the exe link to mention a "branch" instead of a "PR", if no PR number is set in the build. It is assumed that most private builds will be for branches, not PRs against their own clone.
appveyor/scripts/decryptFilesForSigning.ps1: Do not attempt to decrypt files if signing feature is not set in appveyor.yml.
appveyor/scripts/logCiTiming.ps1: added a test to exit if the timing record file is not found.
Regarding the last: originally I included a feature to disable CI timing checks. I decided that feature was unnecessary so reverted it, but I kept the change to detect the missing timing file and silently exit rather than failing hard, since it makes sense that something else may cause that file not to exist, and we wouldn't want the build failing over it.
XLTechie pushed a commit to XLTechie/xlnvda that referenced this pull request Mar 15, 2024
michaelDCurran pushed a commit that referenced this pull request Apr 1, 2024
Follow-up of #16221

Summary of the issue:
The streamlined AppVeyor build process implemented in #16221, requires documentation in order to be useful.

Description of user facing changes
None.

Description of development approach
• Wrote a document in projectDocs/dev/buildingNVDAOnAppVeyor.md.
• Added references in readme.md and contributing.md, at @seanbudd's suggestion.
• Took the liberty of adding a missing lead heading in buildingNVDA.md.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
4 participants