Make installer process safer#16174
Conversation
See test results for failed build of commit 146037363b |
michaelDCurran
left a comment
There was a problem hiding this comment.
At a minimum I think the psutil stuff is good. I wouldn't want the pr held up due to queries on changing the renames etc. We could do that in beta and or master. But the psutil stuff is just a simple check and fail with a static list of exes. Very safe.
|
Which milestone did you want to use? 2024.3.4 milestone is clearly erroneous. #16122 has to be retagged too. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
See test results for failed build of commit c889fbb97a |
|
I don't quite understand why we would have to roll out a patch release for this. While this issue was reported more frequently during the last few weeks, the current behavior has been there for almost a decade. This is neither a security fix nor a fix for a regression introduced in the 2023.3 cycle. |
|
Agreed it is not a security fix. However, recently there have been a
higher number of point releases, which has made the installer issue much
more common.
My firm opinion is that either we put this into a beta and withhold any
further patch releases to 2023.3, or we put it into a 2023.3 patch
release, before any further patch releases.
Although the potential for this bug has existed for a long time, it has
become more prevalent in the last few weeks. And it is at the point
where we can't in good conscience make any further patch releases with
out fixing it, as updates are leaving NVDA in a corrupt state for some.
|
I'd prefer the former. If the latter, I guess the fix can be applied as soon as a patch release is necessary (e.g. as part of a security fix) |
|
Hi, if it indeed involves introducing a new dependency (psutil, a dependency used by various add-ons), then I agree with Leonard that publishing beta 8 would be better. Thanks.
|
|
I have to agree with Leonard and Joseph, that adding this into a patch release is sub-optimal, though for different reasons. We have no idea if this fix is going to really help, verifying this is going to be time consuming and probably inconclusive, and frankly while it improves installer in a nice way the underlying problem (NVDA fails to exit in a reasonable time) still remains. It is also unclear how this will help users to upgrade NVDA, granted it will not become corrupted, but if it fails to exit restarting the machine and attempting upgrade once again will cause users to end up in the same state in which old copy of NVDA is running preventing the upgrade. I'd love to see #16123 investigated and will certainly support adding it into a patch release. It also will be preferable to make sure that 2024.1 is not going to be released with #16072 not solved. |
|
It is also unclear how this will help users to upgrade NVDA, granted
it will not become corrupted, but if it fails to exit restarting the
machine and attempting upgrade once again will cause users to end up in
the same state in which old copy of NVDA is running preventing the upgrade.
You are correct that it will not necessarily assist the user in
upgrading. However, we we really must avoid corrupting a user's existing
install if at all possible.
I'd love to see #16123 investigated and will certainly support adding
it into a patch release.
Absolutely. Any help would be appreciated here. Same with #16072, which
I myself am still trying to investigate further.
|
3802381 to
e5d5ee1
Compare
|
Please note that this bug has become a lot more common, and is a p1 issue causing installations to enter an unrecoverable state. 2024.1 is still a long way away. This fix will go through thorough rc, beta and alpha testing before a 2023.3.4 patch release comes out. |
|
@cary-rowen @hwf1324 - could you test this try build and see if you run into the installer bug still? |
This comment was marked as off-topic.
This comment was marked as off-topic.
Therefore, it is strongly recommended to allow upgrades to new patch versions. Although these updates may not be tested, in theory they should not cause a worse experience. |
It is allowed, but not recommended. Windows 7 support was dropped as of 2023.3. Any patch releases after will not be tested with Windows 7.
This is not true - the changes to HTML browsable message may break a large number of dialogs on Windows 7 |
|
@seanbudd This approach to dropping Windows 7 support is quite unusual. Given that 5% of your users are running it, the basic thing to do would be to either add the link to 2023.0 to NV Access's downloads web page (as soon as the first untested patch release was published), and clearly state that it should be used on Windows 7, or test these patch releases. While I understand that you have limited resources and performing this testing yourselves may be difficult, one of NVDA's strengths it its community, as far as I can tell you didn't reach out to us asking for testing. Also while at it can we please have a clarification of these patch releases and Windows Server 2012 which is still under extended support until 2026? |
|
Hi, Server 2012/2012 R2 are receiving extended security updates, and recently Python’s PEP 11 was edited to state that future Python releases will support Windows releases under extended support (as of now, Windows 10 Version 1607/Server 2016 in Python 3.13 onwards). Thanks.
|
|
I thought Windows 7 support was dropped as of 2024.1? (per the changelog). Any patch releases on the 2023.x branch should support 7, per my intuition and to remain consistent with the contract not to break APIs. Any subsequent full releases (2024.1 onwards) should not. |
|
To say things more clearly: For now (in its beta stage), according to its change log, the first version that drops Windows 7 compatibility is 2024.1. My recommendation would be to do as advertised largely in the change log and elsewhere in the community, i.e. 2024.1 to be the first version dropping Windows 7 support. |
|
Windows Server 2012 are still supported by Python, however we are not actively testing or fixing these bugs. I think there has been some confusion around NV Access development support and whether or not something runs. |
|
But there is no way to prevent Windows 7 users from manually installing 2023.3.X. As far as the current publicity is concerned, 2024.1 is the version that clearly does not support Windows 7. It may be natural for people to think that Windows 7 should choose the last version allowed to run on Windows 7. Few people know that NV Access has No longer recommended this. |
|
@cary-rowen - the release posts advertise this information. |
9884024 to
4292f9f
Compare
The base branch was changed.
…talled NVDA extra time to fully exit on the UAC screen (#16205) Fixes #16199 Fixes #16122 Summary of the issue: In pr #16174, effort was made to make the installer more robust, including better handling when the installed NVDA is still running, by trying to remove the NVDA exes first, and if any of them fail, restoring them all and not corrupting a user's install. Although it no longer corrupts the install, it was too fast at at trying to remove the exes and no longer tried multiple times, which did not take into account that the installed NVDA has just been running on the UAC screen, and may still be exiting. Description of user facing changes NVDA will try multiple times itself to remove the NVDA exes while waiting for the installed NvDA to fully exit. Description of development approach When the installer starts, it will now first wait an initial 1 second to give time for the installed NVDA to exit from the UAC screen. When trying to remove each of the NvDA exes, the installer will again try up to 6 times, waiting half a second before the next try. Again, giving much more chance for an installed NvDA to fully exit like it used to. NVDA will always clean up the temporary files it created while trying to remove all of the exes, whether or not it resulted in a retryableFailure.
Link to issue number:
Fixes #16122
Summary of the issue:
NVDA process sometimes fails to quit correctly (see #16123).
When running the installer, this causes the installer to fail partway through.
If the installer fails partway through, the installation may end up in an unrecoverable state.
For certain files, NVDA requests windows to move or delete these on reboot, if the move/delete fails initially.
However, if the Windows API request fails, NVDA does not handle the failure the same way as it handles other failures - raising a retryable failure and reverting the change if possible.
Description of user facing changes
The NVDA installation process should be more resilient if the previous process is still running.
Description of development approach
NVDA attempts to remove all known exes in a batch as the first operation in the installation. If any known NVDA exe is running, the installation is aborted with a retry able failure and all exes returned to their original state. This means the user can attempt the installation again - i.e. waiting a bit longer for the process to hopefully exit.
If NVDA fails to request windows to remove certain files on restart, this is handled the same as if the files are failed to be removed.
More logging has been added and improved.
Testing strategy:
Known issues with pull request:
The installation process should be atomic - i.e. if it fails, the whole operation should be reverted. This is not the case of the current design. In future code to NVDA we can specify more files we want to include in the atomic and reversible operation.
Code Review Checklist: