Fix up of: New addon api versioning approach (PR #9151)#10284
Merged
feerrenrut merged 2 commits intoOct 2, 2019
Conversation
Contributor
|
CC @LeonarddeR – partly explained by Python string handling differences I think. Thanks.
|
LeonarddeR
approved these changes
Sep 27, 2019
|
Is there any way at all that a simple to understand helpful message could be
made viewable explaining the reason it failed to a user to help them decide
what to do. I mean if it just needs a manifest tweak, or is a genuine non
starter for other reasons?
Brian
bglists@blueyonder.co.uk
Sent via blueyonder.
Please address personal E-mail to:-
briang1@blueyonder.co.uk, putting 'Brian Gaff'
in the display name field.
Newsgroup monitored: alt.comp.blind-users
----- Original Message -----
From: "Julien Cochuyt" <notifications@github.com>
To: "nvaccess/nvda" <nvda@noreply.github.com>
Cc: "Subscribed" <subscribed@noreply.github.com>
Sent: Thursday, September 26, 2019 11:29 PM
Subject: [nvaccess/nvda] Fix up of: New addon api versioning approach (PR
#9151) (#10284)
… <!--
Please fill in the following template, for an explanation of the sections
see:
https://github.com/nvaccess/nvda/wiki/Github-pull-request-template-explanation-and-examples
-->
### Link to issue number:
Fix up of PR #9151
This PR addresses two issues regarding add-on manifests:
### Report manifest validation errors also for add-ons already installed.
#### Summary of the issue:
Validation errors in an add-on manifest are currently only reported upon
installation from a bundle or when (re-)creating a bundle from an exploded
directory.
For add-on already installed, in some situations, this does not help much
in understanding validation issues due to an update of the manifest
specification.
Example log before this PR:
```
ERROR - addonHandler._getAvailableAddonsFromPath (23:02:04.972) -
MainThread (8180):
Error loading Addon from path:
C:\Users\Julien\AppData\Roaming\nvda\addons\failer_missingNVDAVersion
Traceback (most recent call last):
File "addonHandler\__init__.pyc", line 206, in
_getAvailableAddonsFromPath
File "addonHandler\addonVersionCheck.pyc", line 32, in
isAddonCompatible
File "addonHandler\addonVersionCheck.pyc", line 13, in
hasAddonGotRequiredSupport
TypeError: '<=' not supported between instances of
'str' and 'tuple'
```
#### Description of how this pull request fixes the issue:
Example log after this PR as of commit 967b90a:
```
WARNING - addonHandler._report_manifest_errors (22:59:11.930) - MainThread
(1960):
Error loading manifest:
{'name': True, 'summary': True, 'description':
True, 'author': True, 'version': True,
'minimumNVDAVersion': ValidateError('"None" is not a
valid API Version string: None'), 'lastTestedNVDAVersion':
ValidateError('"None" is not a valid API Version string:
None'), 'url': True, 'docFileName': True}
ERROR - addonHandler._getAvailableAddonsFromPath (22:59:11.930) -
MainThread (1960):
Error loading Addon from path:
C:\Users\Julien\AppData\Roaming\nvda\addons\failer_missingNVDAVersion
Traceback (most recent call last):
File "addonHandler\__init__.py", line 195, in
_getAvailableAddonsFromPath
a = Addon(addon_path)
File "addonHandler\__init__.py", line 297, in __init__
raise AddonError("Manifest file has errors.")
addonHandler.AddonError: Manifest file has errors.
```
Thus, an add-on with an invalid manifest now produces the same log when
already installed than when attempting to install it fro ma bundle.
### Flag add-on as incompatible if lastTestedNVDAVersion is literally set
to "None"
#### Summary of the issue:
The add-on template nvdaaddons/AddonTemplate, when not modified, produces
a manifest with lastTestedNVDAVersion literally set to the string
"None".
One may argue whether this template is an authoritative source or not, but
it is definitely an advisable starting point used by many add-on authors.
While it is obvious that add-on authors should modify the provided
`buildVars.py` file to set an appropriate value for this entry of the
manifest, several add-ons are already out in the wild with the
incriminated literal string "None".
The current master behaves differently with such add-ons than 2019.2 does.
With 2019.2, the value "None" is replaced for evaluation by the
default "0.0.0" and these add-ons are properly reported as
incompatible.
On the current master, however, these add-ons are just missing from the
list, without much notice except in the log (see example log above).
#### Description of how this pull request fixes the issue:
Commit 0739a06 makes the add-on manifest validator consider
"None" as the default 0.0.0, and these add-ons are again
properly reported as incompatible.
### Testing performed:
- Loaded a valid compatible installed add-on.
- Attempted to load a valid incompatible installed add-on.
- Attempted to load an invalid installed add-on.
- Installed a valid compatible add-on.
- Attempted to install a valid incompatible add-on.
- Attempted to install an invalid add-on.
### Known issues with pull request:
This PR groups two related fixes.
Please let me know if you prefer I file two separate PR for easier
evaluation.
### Change log entry:
I do not think the first fix deserves much attention, or maybe:
Bug fixes: Errors in the manifest of add-ons already installed are now
properly reported in the log.
The second fix restores the behavior as of the last release, and thus IMHO
should not be announced.
cc @feerrenrut
You can view, comment on, or merge this pull request online at:
#10284
-- Commit Summary --
* Report manifest validation errors also for add-ons already installed.
* Flag add-on as incompatible if lastTestedNVDAVersion is literally set
to "None"
-- File Changes --
M source/addonHandler/__init__.py (5)
-- Patch Links --
https://github.com/nvaccess/nvda/pull/10284.patch
https://github.com/nvaccess/nvda/pull/10284.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#10284
|
Contributor
Author
There could potentially be, but I guess it would be out of scope for these small straightforward fixes. |
Contributor
|
@Brian1Gaff wrote:
It isn't possible. At the installation stage the only criterion by which NVDA judges if add-on is install-able or not are values in its manifest. It is of course quite logical there is no crystal ball which could predict how it would behave if installed. |
feerrenrut
approved these changes
Oct 2, 2019
feerrenrut
left a comment
Contributor
There was a problem hiding this comment.
This looks good to me. Thanks @JulienCochuyt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link to issue number:
Fix up of PR #9151
This PR addresses two issues regarding add-on manifests:
Report manifest validation errors also for add-ons already installed.
Summary of the issue:
Validation errors in an add-on manifest are currently only reported upon installation from a bundle or when (re-)creating a bundle from an exploded directory.
For add-on already installed, in some situations, this does not help much in understanding validation issues due to an update of the manifest specification.
Example log before this PR:
Description of how this pull request fixes the issue:
Example log after this PR as of commit 967b90a:
Thus, an add-on with an invalid manifest now produces the same log when already installed than when attempting to install it fro ma bundle.
Flag add-on as incompatible if lastTestedNVDAVersion is literally set to "None"
Summary of the issue:
The add-on template nvdaaddons/AddonTemplate, when not modified, produces a manifest with lastTestedNVDAVersion literally set to the string "None".
One may argue whether this template is an authoritative source or not, but it is definitely an advisable starting point used by many add-on authors.
While it is obvious that add-on authors should modify the provided
buildVars.pyfile to set an appropriate value for this entry of the manifest, several add-ons are already out in the wild with the incriminated literal string "None".The current master behaves differently with such add-ons than 2019.2 does.
With 2019.2, the value "None" is replaced for evaluation by the default "0.0.0" and these add-ons are properly reported as incompatible.
On the current master, however, these add-ons are just missing from the list, without much notice except in the log (see example log above).
Description of how this pull request fixes the issue:
Commit 0739a06 makes the add-on manifest validator consider "None" as the default 0.0.0, and these add-ons are again properly reported as incompatible.
Testing performed:
Known issues with pull request:
This PR groups two related fixes.
Please let me know if you prefer I file two separate PR for easier evaluation.
Change log entry:
I do not think the first fix deserves much attention, or maybe:
Bug fixes: Errors in the manifest of add-ons already installed are now properly reported in the log.
The second fix restores the behavior as of the last release, and thus IMHO should not be announced.
cc @feerrenrut