Skip to content

Update NSIS to 3.08#13398

Merged
seanbudd merged 11 commits into
masterfrom
NSISAsSubmodule
Mar 14, 2022
Merged

Update NSIS to 3.08#13398
seanbudd merged 11 commits into
masterfrom
NSISAsSubmodule

Conversation

@seanbudd

@seanbudd seanbudd commented Feb 28, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Link to issue number:

Fixes #13270, #9134

May fix #13329, #13222

Summary of the issue:

NSIS is outdated (version 2.51 is from 2016).
A variety of issues have been coming up with the installer:

  • If special characters are in the path, the NSIS fails to run the installer
  • Installer is failing to start on certain builds of Windows (Windows 7 SP1, Windows 11 ARM, Windows 10 21H2)

Description of how this pull request fixes the issue:

  • NSIS has been moved to its own submodule (currently private while reviewing the repository settings).
    • this includes adding steps for updating in the future
  • NSIS has been updated to 3.08.
  • The UAC plugin has been removed, as it is now redundant.
    • NSIS elevates the uninstaller automatically now, rather than needing the UAC plugin. This has been confirmed with testing.
  • Minor build warnings have been fixed.

Testing strategy:

Follow the testing strategy in the NSIS submodule readme

Testing confirmed on:

  • Windows 11 ARM
  • Windows 11
  • Windows 10
  • Windows 7 SP1

Known issues with pull request:

None

Change log entries:

Bug fixes

- The NVDA installer can now run from directories with special characters. (#31270)
- Other entries if #13329, #13222 being fixed is confirmed.

Changes

- NSIS has been updated to version 3.08. (#9134)

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English

@seanbudd
use latest nsis
d018ccf
@seanbudd
remove redundant plugin code
5797751
@seanbudd
Update NSIS link
5683e66
@seanbudd
Fix build warnings for the installer and uninstaller
969aadf 
Fixes the following build warnings

For the installer:
    warning: !warning: MUI_LANGUAGE[EX] should be inserted after the MUI_[UN]PAGE_* macros (macro:MUI_LANGUAGEEX:6)

For the uninstaller:
    9100: Generating version information for language "1033-English" without standard key "FileVersion"
@LeonarddeR

LeonarddeR commented Feb 28, 2022

Copy link
Copy Markdown
Collaborator

I think there are several reasons to no longer bundle nsis with NVDA:

  • It is part of the Appveyor image. This ensures we're always up to date with NSIS, which shouldn't be a problem as we no longer use a separate plugin now.
  • People who build launchers usually have the knowledge to keep nsis up to date. We can simply list it as an optional dependency if you want to create a launcher. To put it the other way around, why should we bother about bundling nsis while we want people to install Visual Studio and Python themselves? They can also install NSIS which is pretty straight forward. There is more to be said for bundling Python 3.7 now that installers are no longer being built and the version on appveyor is no longer up to date.
  • No longer bundling NSIS means there's no longer a maintenance burden to keep it up to date.

@lukaszgo1

lukaszgo1 commented Feb 28, 2022

Copy link
Copy Markdown
Contributor 
* The UAC plugin has been removed, as it is now redundant.

I would like to have more explanation on this preferably with the link to some relevant NSIS documentation. At the very least something more than "the plugin is redundant" as that does not answer "why".

@seanbudd

seanbudd commented Feb 28, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

I would like to have more explanation on this preferably with the link to some relevant NSIS documentation.

Sadly this is undocumented - or at least I could not find any after spending a long time searching.
NSIS elevates the uninstaller automatically now, rather than needing the UAC plugin.
This has been confirmed with testing.

@seanbudd

seanbudd commented Feb 28, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

No longer bundling NSIS means there's no longer a maintenance burden to keep it up to date.

I don't think this is true. Like any dependency, an update might break NVDA.
Without being able to pin a version, that maintenance cost becomes an immediate pressure, rather than an scheduled task.
As NSIS is used for the installer, updating it has relatively higher risk.
By using the latest NSIS on AppVeyor, without pinning to a version, we risk the installer breaking without us knowing.
For example, if NSIS updates around the time of a release, and the update breaks Windows 7 compatibility, we could push out a broken release.

On the other hand, the process of updating NSIS (once it is it's own submodule) is relatively straightforward.
Keeping it up to date should have a low maintenance cost moving forward.

@seanbudd seanbudd mentioned this pull request Mar 4, 2022
Merged
feerrenrut
feerrenrut approved these changes Mar 4, 2022
View reviewed changes

@feerrenrut feerrenrut 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.

Changes look good, though the miscDeps commit should be updated if there is a new commit created when nvaccess/nvda-misc-deps#24 is merged to ensure it is left in an easy to understand state (miscDeps submodule will then point to the most recent commit on miscDeps master)

This was referenced Mar 6, 2022
Closed
Closed
@seanbudd seanbudd marked this pull request as ready for review March 6, 2022 23:20
@seanbudd seanbudd requested a review from a team as a code owner March 6, 2022 23:20
@seanbudd seanbudd requested a review from michaelDCurran March 6, 2022 23:20
@seanbudd seanbudd merged commit e86e37f into master Mar 14, 2022
@seanbudd seanbudd deleted the NSISAsSubmodule branch March 14, 2022 23:27
@nvaccessAuto nvaccessAuto added this to the 2022.2 milestone Mar 14, 2022
@seanbudd seanbudd mentioned this pull request Mar 14, 2022
Closed
@CyrilleB79

CyrilleB79 commented Mar 15, 2022

Copy link
Copy Markdown
Contributor

@seanbudd, if it is confirmed that this upgrade fixes the various installation issues, would you consider cherry-pick this commit to beta for 2022.1?
It would seem strange to me not to provide a solution as soon as possible for people who can not install NVDA at all.
Cc @feerrenrut for his opinion.

@lukaszgo1

lukaszgo1 commented Mar 15, 2022

Copy link
Copy Markdown
Contributor

if it is confirmed that this upgrade fixes the various installation issues, would you consider cherry-pick this commit to beta for 2022.1? It would seem strange to me not to provide a solution as soon as possible for people who can not install NVDA at all.

This seems pretty risky to me. Granted for some people the situation may be improved but this is a rather significant change to a critical component, and there is no saying if installer would not become broken on some other configurations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelDCurran michaelDCurran michaelDCurran approved these changes

+1 more reviewer

@feerrenrut feerrenrut feerrenrut approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2022.2

Development

Successfully merging this pull request may close these issues.

problems installing nvda in windows 11 insider preview for ARM NVDA installer does not start if special characters are in the installer path

7 participants

@seanbudd @LeonarddeR @lukaszgo1 @CyrilleB79 @feerrenrut @michaelDCurran @nvaccessAuto