Skip to content

Avoid building dlls twice due to signing postAction#20084

Closed
michaelDCurran wants to merge 8 commits into
betafrom
i20033
Closed

Avoid building dlls twice due to signing postAction#20084
michaelDCurran wants to merge 8 commits into
betafrom
i20033

Conversation

@michaelDCurran

@michaelDCurran michaelDCurran commented May 8, 2026

Copy link
Copy Markdown
Member

Link to issue number:

Fixes #20033

Summary of the issue:

Signing of dlls and executables during NVDA build was implemented in SCons as post actions, as in once the dll or exe target was fully created, then a separate post-action for signing was run. The problems with this approach are:

  • when building with multiple processor cores, further actions that depend on a dll (such as packaging, might start running before the dll or exe is actually signed, as SCons thinks the source already exists.
  • When building as separate steps (E.g. source, then launcher) it is possible in environments such as Github actions, that the second running of SCons may think that some DLLs or EXEs are outdated because the signing postAction changed them, so therefore they are re-build and re-signed. This is especially problematic as collecting of these dls and exes for debug symbols may have archived the copies from the first build, rather than the second. (See #22033).

Description of user facing changes:

Description of developer facing changes:

Description of development approach:

  • Rather than using AddPostAction to sign files, make a new copyAndSign builder that copies a source file to the target, and then signs the target all in the same action, thus Scons treats the target as complete once signed.
  • Replace any usage of env.AddPostAction and then env.Install with env.copyAndSign.
  • When building the launcher and uninstaller, optionally add the signing step as one of the actions, rather than running a post action later.
  • Github CI again tells Scons to build with all cores. This should be safe enough now.

Testing strategy:

  • Build locally with and without signing info. Ensure builds succeeed. For a build with signing info, ensure each of the dlls and exes are really signed.
  • Make a try build and verify that dlls and exes are correctly signed.
  • Check try build log Create Launcher job to ensure that dlls are no longer copied and signed.

Known issues with pull request:

None known.

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • 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.

… builder that copies a source file to the target, and then signs the target all in the same action, thus Scons treats the target as complete once signed. Replace any usage of env.AddPostAction and then env.Install with env.copyAndSign. This should all mean that re-running the build should not treat a target as being changed just because it was signed, then causing the target to be re-build and re-signed inappropriately.
…equiring signing, by inserting the signing action only if signing is enabled.
Copilot AI review requested due to automatic review settings May 8, 2026 04:05
@michaelDCurran michaelDCurran requested a review from a team as a code owner May 8, 2026 04:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR aims to prevent redundant rebuilds (and related symbol mismatches) caused by code-signing implemented as SCons post-actions, by moving signing into the primary build/copy actions so targets are considered “complete” only after signing.

Changes:

  • Introduces an env.copyAndSign helper to copy outputs into their install locations and sign them within the same SCons action sequence.
  • Replaces several AddPostAction(...sign...) + Install(...) patterns in nvdaHelper/third-party builds with copyAndSign.
  • Updates CI SCons invocation to build with --all-cores (supported via custom option in sconstruct).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
sconstruct Adds copyAndSign, moves launcher/uninstaller signing into main actions, and includes a minor comment tweak.
nvdaHelper/liblouis/sconscript Switches liblouis install/sign flow to use env.copyAndSign.
nvdaHelper/archBuild_sconscript Switches multiple helper library install/sign flows to use env.copyAndSign.
ci/scripts/setSconsArgs.ps1 Changes CI to pass --all-cores instead of -j1.

Comment thread sconstruct
Comment thread nvdaHelper/liblouis/sconscript Outdated
Comment thread nvdaHelper/archBuild_sconscript Outdated
Comment thread nvdaHelper/archBuild_sconscript Outdated
Comment thread sconstruct Outdated
@seanbudd seanbudd added this to the 2026.2 milestone May 8, 2026
@SaschaCowley SaschaCowley marked this pull request as draft May 10, 2026 23:04
…f this method have been changed to pass a single file, and then Install any remaining files separately.
@michaelDCurran michaelDCurran requested a review from Copilot May 11, 2026 04:19
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comment thread sconstruct Outdated
Comment thread sconstruct
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@michaelDCurran michaelDCurran marked this pull request as ready for review May 11, 2026 04:34
@seanbudd seanbudd changed the base branch from master to beta May 11, 2026 08:22
@seanbudd seanbudd requested a review from a team as a code owner May 11, 2026 08:22
@seanbudd seanbudd requested a review from Qchristensen May 11, 2026 08:22
@seanbudd seanbudd changed the base branch from beta to master May 11, 2026 08:56
@seanbudd seanbudd changed the base branch from master to beta May 11, 2026 08:57
@github-actions github-actions Bot requested a deployment to snapshot May 11, 2026 09:06 Abandoned
@michaelDCurran michaelDCurran marked this pull request as draft May 11, 2026 21:23
@michaelDCurran

michaelDCurran commented May 11, 2026

Copy link
Copy Markdown
Member Author

Unfortunately even with these changes scons launcher still wants to again copy and sign all dlls and exes as it thinks they are missing or the timestamp is bad in the cache.
See https://github.com/nvaccess/nvda/actions/runs/25652575422/job/75320041651#step:8:40

@SaschaCowley SaschaCowley added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label May 12, 2026
@hwf1324

hwf1324 commented May 12, 2026

Copy link
Copy Markdown
Contributor

Is it possible to determine the reason for rebuilding these files by using --debug=explain?

Perhaps it is also possible to decide whether to build files by using a custom env.Decider function?

…ider to be content rather than timestamp for the signed files as otherwise Github actions caching timestamps seem to confuse SCons and cause a rebuild.
@michaelDCurran

Copy link
Copy Markdown
Member Author

Closing in favor of #20129.
Local testing proves that SCons does correctly track env.AddPostAction. and the real issue was that Github CI was not setting the API signing token when running scons source therefore files were never even signed during scons source and therefore scons launcher was needing to do it as a dependency.

seanbudd pushed a commit that referenced this pull request May 19, 2026
… doesn't do it, causing uploaded symbols to mismatch (#20129)

Fixes #20033
Replaces pr #20084
Summary of the issue:

The debug symbols archived from builds of NvDA do not match the actual files in the final NVDA launcher, if files are required to be signed.
This is due to the fact that the API signing key was never set in the CI environment when building NVDA with scons source, although it was present when running scons launcher. NVDA would be initially built without signed files, and these files would be collected for the symbols, but then as the launcher was being created, the files would be retroactively signed.
Also if trying to run CI instructing SCons to use multiple cores to speed up builds, some of the files may have been signed after the launcher was created, thus leaving them as the old unsigned copies.
Signing with signpath takes a very long time, and can add up to 10 minutes for a build when using a single core.
Thus signing properly in scons source would address the incorrect symbols and allow for building using multiple cores.
Description of user facing changes:
Description of developer facing changes:
Description of development approach:

    Set the API signing token in the environment for the set scons args step in the Build NVDA job. this mirrors what is already done for the Create Launcher job.
    In the Create Launcher job, Split the powershell commands to install signpath out of the Create launcher step and into its own Install Signpath step.
    Add an Install Signpath step to the Build NVDA job, mirroring the step in the Create launcher job.
    In the Create launcher job, split out the building of the NVDA controller client from the Create Launcher step into its own Create NVDA controller client step.
    Remove the sconsCores variable from the scons launcher command in the Create Launcher step, meaning that scons launcher will always run with only one core, no matter what sconsCores is set to. It is very important that scons launcher never run with multiple cores, as if it ever has any dependencies, they must be build before the NSIS launcher builds.
    change the sconsCores variable to be --all-cores meaning that scons source, scons user_docs, scons client etc (but not scons launcher) will now run using all available cores). The biggest advantage here is signing files in scons source which dramatically decreases time as files can be signed in parallel to further source building.
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

Development

Successfully merging this pull request may close these issues.

ci: Debug symbols are built from incorrect dll/exe files, rendering them useless

5 participants