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.

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

Fix up of: New addon api versioning approach (PR #9151) #10284

Merged

Conversation

JulienCochuyt
Copy link
Collaborator

@JulienCochuyt JulienCochuyt commented Sep 26, 2019

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

@josephsl
Copy link
Collaborator

josephsl commented Sep 26, 2019

@Brian1Gaff
Copy link

Brian1Gaff commented Sep 27, 2019

@JulienCochuyt
Copy link
Collaborator Author

JulienCochuyt commented 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?

There could potentially be, but I guess it would be out of scope for these small straightforward fixes.
At least, with this PR, the produced log is much more explicit about what's going on.

@lukaszgo1
Copy link
Contributor

lukaszgo1 commented Sep 27, 2019

@Brian1Gaff wrote:

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?

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.

Copy link
Member

@feerrenrut feerrenrut left a comment

This looks good to me. Thanks @JulienCochuyt

@feerrenrut feerrenrut merged commit 78c584f into nvaccess:master Oct 2, 2019
1 check passed
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Oct 2, 2019
@JulienCochuyt JulienCochuyt deleted the pr9151-addonManifestValidation branch Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants