Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove pyd file in installation folder #10070

Merged
merged 2 commits into from
Sep 13, 2019
Merged

remove pyd file in installation folder #10070

merged 2 commits into from
Sep 13, 2019

Conversation

clementb49
Copy link
Contributor

@clementb49 clementb49 commented Aug 11, 2019 •
edited
Loading

Link to issue number:

Fixes #10063

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
Copy link
Contributor Author

@larry801 @LeonarddeR can you test this?

@DataTriny
Copy link
Contributor

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
Copy link
Collaborator

Hi,

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

Thanks.

@lukaszgo1
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor Author

clementb49 commented Aug 12, 2019 via email

@LeonarddeR
Copy link
Collaborator

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

@lukaszgo1
Copy link
Contributor

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
LeonarddeR suggested changes Aug 12, 2019
View reviewed changes
Copy link
Collaborator

@LeonarddeR LeonarddeR left a comment

Choose a reason for hiding this comment

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

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

source/installer.py Outdated
@@ -210,7 +210,7 @@ def removeOldProgramFiles(destPath):
# in a newer version of NVDA.
for curDestDir,subDirs,files in os.walk(destPath):
for f in files:
if f.endswith((".pyc", ".pyo")):
if f.endswith((".pyc", ".pyo", ".pyd")):
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

josephsl commented Aug 12, 2019 via email

@clementb49
Copy link
Contributor Author

clementb49 commented Aug 12, 2019 via email

@clementb49
Copy link
Contributor Author

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

@larry801
Copy link
Contributor

Why not just remove all pyd file in destPath?

@feerrenrut feerrenrut added this to the 2019.3 milestone Aug 14, 2019
LeonarddeR
LeonarddeR approved these changes Aug 17, 2019
View reviewed changes
Copy link
Collaborator

@LeonarddeR LeonarddeR left a comment

Choose a reason for hiding this comment

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

Thanks @clementb49

@LeonarddeR
Copy link
Collaborator

Why not just remove all pyd file in destPath?

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

@CyrilleB79
Copy link
Collaborator

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.

  1. NVDA 2019.3 runs on Pytnon 3.7
  2. Let's say that an alpha version for NVDA 2022.2 runs Python 3.8
  3. 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.
  4. A user that still runs an old NVDA 2021.4 installs character information.
  5. Then this user upgrades NVDA to last stable version : NVDA 2022.1. This upgrade removes unicodedata2.cp38-win32.pyd (py3.8)
  6. NVDA 2022.2 stable release is published.
  7. The user upgrades NVDA to 2022.2. This installation removes unicodedata2.cp37-win32.pyd (py.37)
  8. When using the addon, an error occurs sinc the py3.8 library is not present anymore.

@LeonarddeR
Copy link
Collaborator

LeonarddeR commented Aug 26, 2019 via email

@CyrilleB79
Copy link
Collaborator

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

@michaelDCurran michaelDCurran merged commit a1086b1 into nvaccess:master Sep 13, 2019
@lukaszgo1
Copy link
Contributor

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
Copy link
Member

@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 lukaszgo1 mentioned this pull request Sep 16, 2019
Merged
@lukaszgo1
Copy link
Contributor

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

michaelDCurran pushed a commit that referenced this pull request Oct 21, 2019
@lukaszgo1 @michaelDCurran
Fix up of: remove pyd file in installation folder (#10070) (#10224)
ba13d5c 
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LeonarddeR LeonarddeR LeonarddeR approved these changes

@michaelDCurran michaelDCurran michaelDCurran approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2019.3
Development

Successfully merging this pull request may close these issues.

Obsolete pyd file came from pywin32 after upgrade to alpha may crash nvda
9 participants
@clementb49 @DataTriny @josephsl @lukaszgo1 @LeonarddeR @larry801 @CyrilleB79 @michaelDCurran @feerrenrut