remove pyd file in installation folder by clementb49 · Pull Request #10070 · nvaccess/nvda

clementb49 · 2019-08-11T11:52:24Z

Link to issue number:

Summary of the issue:

If you upgrade from NVDA python 2 to NVDA python 3, it is necessary to remove pyc, pyo, pyd files.
The problem is that the current alpha version of nvda removes just pyc, pyd files.

Description of how this pull request fixes the issue:

This PR adds the ability to the installer to remove pyd files.

Testing performed:

Placed a handyTech.pyd file in C:\Program Files (x86)\NVDA\brailleDisplayDrivers. Made sure it disappeared after installing the try build for this pr.

Known issues with pull request:

None

Change log entry:

None

clementb49 · 2019-08-11T11:54:44Z

@larry801 @LeonarddeR can you test this?

DataTriny · 2019-08-11T22:20:00Z

Hello @clementb49,
Could you change the second line of your PR description to: Fixes #10063 please? Otherwise GitHub wont automatically close the associated issue once this one gets merged. Thanks!

josephsl · 2019-08-11T22:26:17Z

Hi,

For this one, a changelog isn't quite necessary unless @LeonarddeR says otherwise.

Thanks.

lukaszgo1 · 2019-08-12T08:48:49Z

This approach would also remove .pyd files from systemConfig directory, right? If so it might break
add-ons included in systemConfig each time NVDA is updated. For pyc and pyo files this isn't a problem- they would be recompiled but .pyd files are different story altogether.

LeonarddeR · 2019-08-12T09:13:20Z

Yes, pyd files in systemConfig is definitely different. Thanks for pointing this out.
Having said that, I wonder whether there are any add-ons with pyd files in them. If so, that should be strongly discouraged anyway.

clementb49 · 2019-08-12T09:31:46Z

What do you think i need to detect if it is a pyd file in the systemconfig folder and not delete it or we leave it like this. Clément Boussiron

…

Le 12 août 2019 à 11:13, Leonard de Ruijter ***@***.***> a écrit : Yes, pyd files in systemConfig is definitely different. Thanks for pointing this out. Having said that, I wonder whether there are any add-ons with pyd files in them. If so, that should be strongly discouraged anyway. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

LeonarddeR · 2019-08-12T10:08:25Z

@josephsl might know whether there are add-ons supplying pyd files

lukaszgo1 · 2019-08-12T10:14:15Z

Looking at the add-ons which I have curenty installed two of them namely OCR, and WinWizard are bundling .pyd files. They aren't very useful in the systemConfig in my opinion,. Even if we don't know about any add-on doing this better to be safe than sorry.

LeonarddeR

As pointed out, the current code removes *.pyd from the systemCOnfig directory.

I think the following suggestion is what we want. It removes all pyd files from all folders (including systemConfig), except for the pyd files that are declared compatible with the python release NVDA is based on (i.e. ending with .cp37-win32.pyd

LeonarddeR · 2019-08-12T10:19:19Z

 	for curDestDir,subDirs,files in os.walk(destPath):
 		for f in files:
-			if f.endswith((".pyc", ".pyo")):
+			if f.endswith((".pyc", ".pyo", ".pyd")):


I think this would be the best approach.

Suggested change

if f.endswith((".pyc", ".pyo", ".pyd")):

if f.endswith((".pyc", ".pyo", ".pyd")) and not f.endswith(importlib.machinery.EXTENSION_SUFFIXES[0]):

make sure to import importlib.machinery at the top of the file.

josephsl · 2019-08-12T13:45:11Z

Hi, I know that Resource Monitor is one of those that bundle a C extension DLL (.pyd). Thanks. From: Leonard de Ruijter <notifications@github.com> Sent: Monday, August 12, 2019 3:08 AM To: nvaccess/nvda <nvda@noreply.github.com> Cc: Joseph Lee <joseph.lee22590@gmail.com>; Mention <mention@noreply.github.com> Subject: Re: [nvaccess/nvda] remove pyd file in installation folder (#10070) @josephsl <https://github.com/josephsl> might know whether there are add-ons supplying pyd files — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10070?email_source=notifications&email_token=AB4AXEADLKV43DRNLXPQP2DQEEZB3A5CNFSM4IK3T4TKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4CCYHQ#issuecomment-520367134> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AB4AXEBJ27FE7VWYU3SOPNTQEEZB3ANCNFSM4IK3T4TA> .

clementb49 · 2019-08-12T14:03:44Z

Ok, I update this PR with the change requested. Thanks for your feedback. Clément Boussiron

…

Le 12 août 2019 à 15:45, Joseph Lee ***@***.***> a écrit : Hi, I know that Resource Monitor is one of those that bundle a C extension DLL (.pyd). Thanks. From: Leonard de Ruijter ***@***.***> Sent: Monday, August 12, 2019 3:08 AM To: nvaccess/nvda ***@***.***> Cc: Joseph Lee ***@***.***>; Mention ***@***.***> Subject: Re: [nvaccess/nvda] remove pyd file in installation folder (#10070) @josephsl <https://github.com/josephsl> might know whether there are add-ons supplying pyd files — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10070?email_source=notifications&email_token=AB4AXEADLKV43DRNLXPQP2DQEEZB3A5CNFSM4IK3T4TKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4CCYHQ#issuecomment-520367134> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AB4AXEBJ27FE7VWYU3SOPNTQEEZB3ANCNFSM4IK3T4TA> . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

clementb49 · 2019-08-12T18:41:03Z

@LeonarddeR this PR is now updated with your requested change.

larry801 · 2019-08-13T00:39:30Z

Why not just remove all pyd file in destPath?

LeonarddeR

Thanks @clementb49

LeonarddeR · 2019-08-17T07:19:41Z

Why not just remove all pyd file in destPath?

That will still leave old, incompatible pyd files in systemConfig add-ons

CyrilleB79 · 2019-08-26T13:15:07Z

Hello

I am the author of Character Information addon which uses .pyd files (to get last version of unicodedata2).

Here are my thoughts about .pyd files removal in the addons.
The .pyd files were bundled and released by the addon author. So IMO, it is the responsibility of the addon's author to guarantee that no old .pyd file (Python 2) is called with new NVDA version (Python 3). With the compatibility flags, the author has the required tool to avoid that an old .pyd file (Py2) be called from a newer NVDA (py3) version.
As an example, Character Information addon updates the import path according to NVDA python version.
On the contrary:

.pyc and .pyo files were not provided by the addon's author but created at runtime, so it is safe to delete them
.pyd files of NVDA are provided within NVDA installer so it is the responsibility of this installer to remove old .pyd files if tey may cause bugs.
So I would recommand to delete .pyd files only among NVDA files, not in the addons.

Reading this ticket however, makes me think that I should look not only at python major version but also at mid version number (7) since I include unicodedata2.cp37-win32.pyd.

At least, here is a use case when removing .pyd of the extensions will fail in the hypothesis NVDA installer removes the .pyd files not linked to current python version.

NVDA 2019.3 runs on Pytnon 3.7
Let's say that an alpha version for NVDA 2022.2 runs Python 3.8
I update Character information to embed unicodedata2.cp37-win32.pyd and unicodedata2.cp38-win32.pyd (python 3.7 and 3.8 versions) to support the new alpha and test it.
A user that still runs an old NVDA 2021.4 installs character information.
Then this user upgrades NVDA to last stable version : NVDA 2022.1. This upgrade removes unicodedata2.cp38-win32.pyd (py3.8)
NVDA 2022.2 stable release is published.
The user upgrades NVDA to 2022.2. This installation removes unicodedata2.cp37-win32.pyd (py.37)
When using the addon, an error occurs sinc the py3.8 library is not present anymore.

LeonarddeR · 2019-08-26T13:54:10Z

Note that removing pyd files only applies to the systemConfig directory in the NVDA program files, in which directory add-ons are installed that are used at the logon screen. I doubt that your add-on would ever be used on the logon screen. Having said that, I don't think Python 3.7 would ever load a file that ends with .cp38-win32.pyd, so I can live with a patch that removes every pyd file except for those ending with .cp3*-win32.pyd

CyrilleB79 · 2019-08-26T15:11:38Z

Oh sorry, I did not realize that this is only for logon screen.
You are right, this add-on will probably not be used in such a screen; I even doubt it could work in this case.
In some case however, many add-ons however may be copied to system config even if the user does not use them. This is the case for example when the user has installed a new speech driver or braille driver on an NVDA version containing already many addons in the user config. The user may ask then to copy his config to system config (logon screen) and has no choice to copy all the other addons along with the new speech or .braille driver. Of course it is not a good practice but NVDA does not provide a better way to do it easily.
Anyway I am going off-topic...

lukaszgo1 · 2019-09-15T14:26:15Z

It turns out that .pyd files are also removed from userConfig in case of portable copies. If someone wants to provide support for versions of NVDA based on Python 3 but still make his add-on compatible with Python 2 it wouldn't work for users of portable copies. Also configurations aren't version dependent, so if someone installs Python 3 version of NVDA as a portable, then use this version to install NVDA, and then downgrade for some reason to Python 2 version his add-onns would not be functional. The quick fix for that would be not to touch userConfig at all.

michaelDCurran · 2019-09-15T21:09:52Z

@lukaszgo1 to be clear, are you suggesting this pr be reverted? That is fine if yes. I found your comment a little hard to follow though.

lukaszgo1 · 2019-09-16T14:48:10Z

@michaelDCurran I have created #10224 to fix the problem described above. As long as it would be merged before 2019.3 there is no reason to revert this one.

* Stop removing compiled Python files from user and system configs during update of NVDA. - In addition remove all .dll and manifest files. - stop removing mpr.dll explicitly - it is removed along with all other libraries. * Review actions.

remove pyd file in installation folder

98cdea4

LeonarddeR suggested changes Aug 12, 2019

View reviewed changes

not remove python 3 pyd files

fb6d1a7

feerrenrut added this to the 2019.3 milestone Aug 14, 2019

LeonarddeR approved these changes Aug 17, 2019

View reviewed changes

michaelDCurran approved these changes Sep 13, 2019

View reviewed changes

michaelDCurran merged commit a1086b1 into nvaccess:master Sep 13, 2019

lukaszgo1 mentioned this pull request Sep 16, 2019

Fix up of: remove pyd file in installation folder (#10070) #10224

Merged

	if f.endswith((".pyc", ".pyo", ".pyd")):
	if f.endswith((".pyc", ".pyo", ".pyd")) and not f.endswith(importlib.machinery.EXTENSION_SUFFIXES[0]):

Uh oh!

Uh oh!

Conversation

clementb49 commented Aug 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

Uh oh!

clementb49 commented Aug 11, 2019

Uh oh!

DataTriny commented Aug 11, 2019

Uh oh!

josephsl commented Aug 11, 2019

Uh oh!

lukaszgo1 commented Aug 12, 2019

Uh oh!

LeonarddeR commented Aug 12, 2019

Uh oh!

clementb49 commented Aug 12, 2019 via email

Uh oh!

LeonarddeR commented Aug 12, 2019

Uh oh!

lukaszgo1 commented Aug 12, 2019

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

LeonarddeR Aug 12, 2019

Choose a reason for hiding this comment

Uh oh!

josephsl commented Aug 12, 2019 via email

Uh oh!

clementb49 commented Aug 12, 2019 via email

Uh oh!

clementb49 commented Aug 12, 2019

Uh oh!

larry801 commented Aug 13, 2019

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

LeonarddeR commented Aug 17, 2019

Uh oh!

CyrilleB79 commented Aug 26, 2019

Uh oh!

LeonarddeR commented Aug 26, 2019 via email

Uh oh!

CyrilleB79 commented Aug 26, 2019

Uh oh!

lukaszgo1 commented Sep 15, 2019

Uh oh!

michaelDCurran commented Sep 15, 2019

Uh oh!

lukaszgo1 commented Sep 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

clementb49 commented Aug 11, 2019 •

edited

Loading