Installer: try to clean up old library files at the end of installation with out affecting the rest of a successful install #7649
Conversation
…vda_slave can log installer errors. this broke when libraries where moved to a version-specific directory.
source/installer.py
Outdated
| This uses Windows' MoveFileEx (either unicode or ascii depending on the given path) and also prepends'\\?\' to allow for long file names. | ||
| """ | ||
| MoveFileEx=windll.kernel32.MoveFileExW if isinstance(path,unicode) else windll.kernel32.MoveFileExA | ||
| return MoveFileEx("\\\\?\\"+path,None,4) |
There was a problem hiding this comment.
Can you replace the 4 with something like MOVEFILE_DELAY_UNTIL_REBOOT
source/installer.py
Outdated
| def copyFile(sourceFilePath,destFilePath): | ||
| """ | ||
| Copies the file at sourcePath to destPath. | ||
| This uses Windows' CopyFile and prepends'\\?\' to allow for long file names. |
There was a problem hiding this comment.
What does the return value mean?
source/installer.py
Outdated
| def deleteFileOnReboot(path): | ||
| """ | ||
| Marks a file for delete on reboot. | ||
| This uses Windows' MoveFileEx (either unicode or ascii depending on the given path) and also prepends'\\?\' to allow for long file names. |
There was a problem hiding this comment.
What is the type/meaning of the return value?
source/logHandler.py
Outdated
| h=ctypes.windll.kernel32.LoadLibraryExW(os.path.abspath(ur"lib\nvdaHelperRemote.dll"),0,0x8) | ||
| self._remoteLib=ctypes.WinDLL("nvdaHelperRemote",handle=h) if h else None | ||
| path=os.path.abspath(os.path.join(u"lib",versionInfo.version,u"nvdaHelperRemote.dll")) | ||
| h=ctypes.windll.kernel32.LoadLibraryExW(path,0,0x8) |
There was a problem hiding this comment.
replace 0x8 with something like LOAD_WITH_ALTERED_SEARCH_PATH
|
Does this mean that you have managed to duplicate these issues of non
deletion and not starting up after an update is successful then?
Kind of makes you wonder if some Windows installations are always using the
same updated copy of the registry.
Brian
bglists@blueyonder.co.uk
Sent via blueyonder.
Please address personal email to:-
briang1@blueyonder.co.uk, putting 'Brian Gaff'
in the display name field.
|
|
Backed out of next. Multiple people were seeing problems. Perhaps this is simply a timing issue. The older installer code took more time trying to remove the first lot of files and therefore the old NVDA had enough time to fully exit or something. Very very strnage. |
|
At very least we need to take the logging fixes from this PR (assuming that we keep #7535). |
…staller, but then just bolt on code to remove old lib files at the very end of installation.
michaelDCurran
changed the title
|
This pr is now much les drastic, in that old lib files are only removed at the very end of the instllation, and no rewriting of other installer code such as tryRemovefile or trycopyFile exists in this pr now. thus, the rhythm and logic of the install is exactly the same as NVDA 2017.3 except for removing of old lib files at the very end. If this part fails, it is not a hard error. |
|
This has also been incubated in Next again for further testing of Next snapshots by Brian1Gaff and others. Testing Master is not necessary at this stage as the change have not yet reached that branch. |
|
May I ask why these changes have been incubated into Next without review? |
|
Simply because this has been very hard to debug and has undergone many
changes and tests. This will definitely not land in Master until it is
reviewed, but at least for testing, it was more important this hit Next
as soon as possible. I could not wait for Reef on Monday.
This bug is of the highest priority as if we can't solve it, all of
#7535 would have to be backed out before 2017.4.
|
|
I think if I may comment here, because the update system was seriously
broken on many windows 7 systems and it needs to be right.
Unfortunately, its one of those weird issues that is hard to actually get to
happen to order and hence needs a lot of testing. In theory the changes
would seem to look like they have improved the folder substitution and
copy reliability mainly as they work as it used to work
I suppose yet another branch might have been created, but seeing as many
windows 10 users have not had the problems it should not really make any
difference to them, only windows 7, I'd suggest.
One big issue often is that if a particular person cannot duplicate an
issue, it does not mean it does not exist, it just means that on that
particular persons systems its not showing up.
The main issue for me now is the intermittent failure to run, mostly on
portable copies after update if run is selected in the checkbox at update
time. I think there could be a bug in the way the temp copy is shut down
before the reboot that is making this a bit of a lottery if it works or
not.
I'm seeing logs stopping without seeing the sort of termination one sees
when nvda is terminated at other times.
This seems to suggest its got bits of the old version running in one
folder for the temp copy and its trying to run bits in the portable copy
folder as well and getting a bit confused with the result windows has a
hissy fit and puts up an error which of course you cannot read as nvda is
stuck.
Brian
bglists@blueyonder.co.uk
Sent via blueyonder.
Please address personal email to:-
briang1@blueyonder.co.uk, putting 'Brian Gaff'
in the display name field.
-----
|
|
Today I could "finally" reproduce it also on Windows 10; first while updating to the latest next, then also when doing a fresh install after a complete uninstall of the previous version and even removing the user data if that would make any difference. INFO - main (21:13:29.977): But then after rebooting from this temporary version of the new install the new version works fine as if it was installed successfully. |
|
The most recent Next branch should hopefully have a fix for that unicode
decode error.
Please let me know if it fixes it for you.
|
|
Yes, it worked fine this time, thanks. |
|
@feerrenrut another review please. This time probably easiest to just review whole thing again as much of the code was removed and only left the most necessary bits, now isolated to the very end of the install. This now seems to work on all systems people have tested on, thankfully. |
|
Is the failure to restart a portable version now on a different issue or
this one?
That is thus far my only issue with updates now, fingers crossed of course.
Brian
bglists@blueyonder.co.uk
Sent via blueyonder.
Please address personal email to:-
briang1@blueyonder.co.uk, putting 'Brian Gaff'
in the display name field.
-----
|
|
I would treat that as a separate issue.
|
|
I think I raised an issue for it a while back but it seemed only some got
the problem.
I now can't recall the number so would need to go and find it.
I myself simply never tick it and restart manually, so I'm not overly
bothered, but it should be looked at before the next stable I'd say or that
option removed to stop it occurring.
Brian
bglists@blueyonder.co.uk
Sent via blueyonder.
Please address personal email to:-
briang1@blueyonder.co.uk, putting 'Brian Gaff'
in the display name field.
-----
|
There was a problem hiding this comment.
Could you outline what testing has been done on this? The permutations that I can think of are:
- Different OS versions
- Upgrade from installed 2017.3
- Clean install
- Upgrade of portable 2017.3
This is assuming that upgrade from last master or next is not so important.
| @@ -154,14 +184,6 @@ def removeOldProgramFiles(destPath): | |||
| else: | |||
| os.remove(fn) | |||
|
|
|||
| # #7546: Remove old version-specific libs | |||
There was a problem hiding this comment.
Is this issue number still relevant? If so we should probably include it.
There was a problem hiding this comment.
This issue number was totally wrong. Not sure why it was there in the first place. It was pointing to the one about not supporting XP anymore.
source/installer.py
Outdated
| @@ -432,6 +455,7 @@ def createPortableCopy(destPath,shouldCopyUserConfig=True): | |||
| tryCopyFile(os.path.join(destPath,"nvda_noUIAccess.exe"),os.path.join(destPath,"nvda.exe")) | |||
| if shouldCopyUserConfig: | |||
| copyUserConfig(os.path.join(destPath,'userConfig')) | |||
| removeOldLibFiles(destPath,rebootOK=False) | |||
There was a problem hiding this comment.
Why is rebootOK=False here, and rebootOK=True in def install?
There was a problem hiding this comment.
Because usually for portable copies it is impossible to delete after reboot as you need admin privilege to set this. However, there is no error if it doesn't work, so I've now allowed it anyway. Some people always running as admin this can work for. Plus, this also ensures that files remain renamed to temp files even if they can't be deleted on reboot, which keeps them out of the way of future installs.
|
Well, Since there has only been the one version with the full fix in it its
early to say all is well. However multiple updates with that version of
various ages of portable versions and an installed next and master have
worked, and files get zapped after a reboot.
Many programs I see do suggest one reboots before running stuff.
I only have windows 7, but on this machine the problem before was
repeatable every time, and now it is a little slower to update but has not
failed so far.
As you said in another thread, I did have issues with the .dicfiles if I'd
already tweaked the names manually but that worked in the case of an
untweaked install.
The only issue now is the intermittent lock up on reboot of the portable
version after an update, which I raised a new ticket for yesterday. This
looks like a timing issue of the shut down routine for nvda and is
irritating but only affects the program in a transient way really.
I'm happy to try some tests in 7 if anyone wants me to over the next few
days as I've figured out how to protect my settings now.
Brian
bglists@blueyonder.co.uk
Sent via blueyonder.
Please address personal email to:-
briang1@blueyonder.co.uk, putting 'Brian Gaff'
in the display name field.
-----
|
…ser *may* have admin access which means it would work. Also it ensures the file, deleted or not, will still be kept renamed as a temp file out of the way.
|
I have tested both on Windows 7 and Windows 10, for both installed and portable, for upgrading from 2017.3, and clean. |
source/installer.py
Outdated
| @@ -455,7 +455,7 @@ def createPortableCopy(destPath,shouldCopyUserConfig=True): | |||
| tryCopyFile(os.path.join(destPath,"nvda_noUIAccess.exe"),os.path.join(destPath,"nvda.exe")) | |||
| if shouldCopyUserConfig: | |||
| copyUserConfig(os.path.join(destPath,'userConfig')) | |||
| removeOldLibFiles(destPath,rebootOK=False) | |||
| removeOldLibFiles(destPath,rebootOK=True) | |||
There was a problem hiding this comment.
How about the case where a portable copy resides on a removable drive and that drive is no longer available after a reboot? I assume Windows silently ignores this?
|
Correct. Windows will silently ignore this. Plus, we were already doing
this in 2017.3 and before.
|
|
Well normally it just can't go to where you want it to can it so how can it
copy there? Another issue exists about stopping users accidentally
generating nvda files all over their desktop or in the root of a drive. How
is that going?
It needs sorting as 've had to sort out a lot of finger trouble portable
versions over the years¬!
Brian
bglists@blueyonder.co.uk
Sent via blueyonder.
Please address personal email to:-
briang1@blueyonder.co.uk, putting 'Brian Gaff'
in the display name field.
-----
|
|
You are right they now do end up as temp files, but I'm admin normally so no
issues.
Brian
bglists@blueyonder.co.uk
Sent via blueyonder.
Please address personal email to:-
briang1@blueyonder.co.uk, putting 'Brian Gaff'
in the display name field.
-----
|
|
Hi, this morning successfully used portable master and next and reinstalled
master as installed copy with no failure to restart, however I am not sure
whether this might still be a possibility as sometimes you get the
descending tones and sometimes not and I'm still seeing that log error
appearing in the main nvda folder so..
To my mind if that error dialogue about the error could somehow be
suppressed it might actually work, its that which stops nvda from rebooting.
There is an issue on .dic files being saved incorrectly again in master
though, in the roaming/nvda folder, with the new easpeak dic appearing
outside of the folder it should be in.
I was intrigued though at looking in the installed folder for nvda, just
how many of the files in there were locked even when you had run a portable
version afterwards, which seems odd to me. Still, a restart of windows
makes them deleteable.
Brian
bglists@blueyonder.co.uk
Sent via blueyonder.
Please address personal email to:-
briang1@blueyonder.co.uk, putting 'Brian Gaff'
in the display name field.
-----
|
|
I've been stress testing the updates now for some days on both branches with
no problems at all visible to the user.
Obviously the flaky restart still continues, but is actually easy to
escape from if it occurs.
ESC and reboot nvda manually.
Brian
bglists@blueyonder.co.uk
Sent via blueyonder.
Please address personal email to:-
briang1@blueyonder.co.uk, putting 'Brian Gaff'
in the display name field.
-----
|
|
Be very careful with this though as it became a nightmare as I recall with
all of those failed installs due to old versions of files being locked etc
stopping it from rebooting. Even now I get the odd failed reboot when the
system seems unable to rename the log file and start a new one.
Brian
bglists@blueyonder.co.uk
Sent via blueyonder.
Please address personal email to:-
briang1@blueyonder.co.uk, putting 'Brian Gaff'
in the display name field.
-----
|
Link to issue number:
Fixes #7616
Fixes #7639
Summary of the issue:
With the merging of #7535 (which causes NVDA to now store libs in a version-specific directory), NVDA updates were constantly failing for certain users.
One or more of these users seemed to be running some kind of Intell security software which would occasionally lock files.
In most situations though, the majority of users were not experiencing the failing updates.
However, #7535 did introduce two specific mistakes. these were:
Description of how this pull request fixes the issue:
Fix logging
logHandler now looks for nvdaHelperRemote in its version-specific directory, allowing logging to work from nvda_slave again. It also fails much earlier now if that dll cannot be found.
Old lib files are deleted only at the very end
Rather than deleting old library files at the beginning of the installation (which meant if this did introduce an error, the rest of the install would fail), instead try to delete them at the very end.
Also, if one of these files fails to be deleted, don't treet this as a hard error, only log a warning. The installer next time it is run can try and get rid of them again.
Testing performed:
Multiple NVDA updates on Windows 10 and Windows 7, with reboots. the updates succeeed, and all locked files are removed on reboot.
This also included performing the update with the old copy of NVDA still running on another session on the machine. The installer still successfully installed the new copy.