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

Revised approach to addon compatibility #9055

Closed
feerrenrut opened this Issue Dec 12, 2018 · 2 comments

Comments

Projects
None yet
3 participants
@feerrenrut
Copy link
Contributor

commented Dec 12, 2018

This issue aims to discuss a revised approach to addon compatibility.

Issues with the existing Addon versioning checks

  • Noisy, likely to require user intervention every release.
  • Historically addons have not required many updates to stay compatible, now add-ons will require an update per release.
  • Concerns from addon authors: despite there being a breaking API change (such as speech refactor / wxPython 4) this does not affect my addon, can I override this?
  • We are expecting very few MAJOR breaking changes in the near future; can we limit required addon re-releases to these?
  • Auto updating to the first version of NVDA with the Addon compatibility check being active does not work well.
    • There is no addon compatibility check prior to installation, only on first start.
    • A dormant first addon compatibility check would provide a better experience.

User stories

Consider the following situations:

  • NVDA introduces a new feature, an addon is based on this feature.
  • NVDA has an update that does not break addon's.
  • NVDA has an update that breaks addons that use a portion of the API E.G. Speech
  • NVDA has an update that breaks all addons, E.G. Python 3

The following use cases can be derived:

  • As an addon author I can assert that my addon relies on versions of NVDA including and after a feature is introduced. Users will not be able to install my addon in versions of NVDA prior to this feature being present.
  • As an addon author I only need to re-publish my addon for NVDA releases that break backwards compatibility with the add-on API. So that I don’t have to re-publish for every release, this is to reduce the workload of publishing add-ons.
  • As an addon author I can assert that my addon is compatible for use with NVDA versions before and after a break in the API, so that I do not have to maintain two versions of an addon while users transition between these versions.
  • As a user I am unable to install an incompatible addon. So that I do not experience instability.
  • As a user I am warned about addons that will no longer be compatible before deciding to update NVDA. I am able to abort the installation, so that I can continue to use addons I rely on.
    As a user I can inspect a list of installed addons that are disabled due to incompatibility with my current version of NVDA so that I can look for new releases of the add-on and/or request the add-on author update them.

 

Addon API background

  • Consider the two ways that the API is likely to change:
    • API additions
    • API deletions
  • Incidental changes in behaviour behind the interface, which we attempt to minimise, have always been the case. We are not aware of this causing regular systemic issues for addons.

 

Version checks

An addon specifies its window of compatibility with two manifest values, and will be considered compatible if the following are all True:

  • Addon manifest min required API value <= NVDA's current API version
  • Addon manifest last tested API value >= NVDA's backwards compatible to API version
     

A more in depth look at these properties:

  • min required - allows the addon author to specify that their addon cannot run without the API additions present in an API version and later.
  • last tested - allows the addon author to specify that their addon is safe to be used, despite potentially requiring an API version less than NVDA's backwards compatible to version. Essentially saying, despite there being API deletions this addon is still compatible. It also may serve as an indicator of how well an add-on is maintained.
    • Previously we didn’t test the minor part of last tested, this was to reduce the number of warnings for users and the burden of re-releasing the addon for a minor update to NVDA given a minor update would be unlikely to break the addon API.
    • With this set of four values, and the stricter consequence (no allowing users to override), we will always test the minor version part. This is also possible because we are decoupling the API version from the release version.
  • These values are each an API version value
  • The logical relationship must be: last tested >= min required

NVDA specifies a window for the API versions in (a new file) addonAPIVersion.py

  • current - the current API version.
    • current will change with every NVDA update, something is added with every release.
  • backwards compatible to - oldest API version to be supported. This is the last version with an API deletion.
  • These values are each an API version value
  • The logical relationship must be current >= backwards compatible to
     

Examples

When the two windows of compatibility overlap, then the add-on is considered compatible and can be enabled. For the ease of discussion, we will assume API version values are consecutive integers, however the exact scheme used for API version values will be discussed later. Consider an installation of NVDA 2020.2, with current API version of 6 and backwards compatible to API version 3:

  • An addon with last tested API version of 3 is compatible, even if min required referred to a version prior to an API breaking change (for instance, API version 2). The addon author has considered the API breaking changes (deletion) and can assure us that the missing parts of the API are not required for the addon.
  • An addon with min required API version 6, is still compatible even if last tested API version is 10. The add-on has not started using any API additions since API version 6.
     

First version with addon version checks

Release 2019.1 with add-on version check ASAP

  • This allows the update mechanism to be ready to inform users BEFORE the installation of NVDA.
  • The update server will be modified to force all users to update via 2019.1
    • This only matters for updates complete via NVDA, manually downloading the launcher of any 2019.1 and later will have the addon compatibility check code.
  • 2019.1 will have no change to compatibility, no impact on addons
     

Addon API Version Value Scheme

So far, we have used strings formatted as NVDA version strings for addon API version values. This caused difficulty when pulling the NVDA version string from the file information in the launcher executable because of the presence of other information. We have decided to ignore the minor part when considering whether an addon has been tested.

Consider:

  • Whether decoupling NVDA release version from Addon API version is necessary.
  • Not having to parse an NVDA version string
    • Difficulties with dev / alpha strings etc
  • Does a user/addon author recognise and understand the timeline of the API version values.
  • What to do for missing manifest values?
  • Must be able to be made available via SCONS
    • So it can added to the launcher metadata / properties
    • So that the server can return the values with the update check response.
    • This will already have to be done for one of the values, since we now want to expose two values rather than just the version.
    • Doing so allows us to report on addon compatibility BEFORE downloading the update.

With regards to decoupling:

  • The current API version can not be incremented without an NVDA release.
  • There is no harm in it being updated with every release, it is likely there is some API addition in every release.
    • This may not be the case if we had an alternative, more static and constrained approach, to our addon API interface. Such an interface is not being proposed.

 

Alternatives to NVDA version string:

  • Consecutive (not necessarily contiguous) integers defaulting to 0
    • An API value of 0 for either of the manifest values signifies that it is missing.
    • Requires updating a value (current) at each release.
  • A hash/GUID type value
    • We would have to maintain an ordered list of the past GUIDs, since the ordering is required
    • Stops add-on authors from setting the last tested value to one far in the future, the GUID would only be known once the release is made.
    • A GUID is very hard for users / addon authors to grok, a reference would be essential.
  • A numerical (float) representation of the version string like 19.20 E.G. {year}.{major}{minor}
    • If there were more than 10 minor releases, ordering is violated.
      • E.G. 19.310 (2019.3.10) is greater than 19.40 (2019.4.0)
    • Does not require an extra manual step at release time current can be generated from the buildVersion tuple, and backwards compatible to would be hard-coded.
  • A tuple, structured in the same way as the build version tuple.
    • Easily recognised by users and add-on authors.
    • No difficulty with parsing, this is not a normal NVDA version string with the potential dev / alpha parts.
    • Rather than getting the file version info from the launcher executable, the server would provide the information with other update information.
    • The launcher would have access to the addonAPIVersion.py file to get the tuple directly.
    • The tuple would have to be parsed from the addon manifest (as it currently is), however we can be strict about the format of this.
    • Missing manifest keys will result in (0.0.0.0)
      • NVDA 2019.1 will have backwards compat to set to (0.0.0.0)
      • Reporting in the UI will have to replace this magic value with a message stating the key is missing.
    • They whole tuple will need to be provided in year.major.minor format. Missing parts will result in rejection.

My preference is to use the tuple representation.

Changes to the UX

When there is an incompatibility, we would no longer state merely that the addon is untested and allow the user to override the incompatibility state. Instead, we will warn that it is incompatible, and the addon will be disabled after installation. The user should always be able to abort the installation.

One improvement could be to specifically detect and highlight incompatible addons that are providing the active synth / braille display. Since these are considered critical to the user.

Addon publishing platforms

Those responsible for addon publishing platforms and reviewing the addons contained there-in should note the importance of the accuracy of the last tested addon manifest value. If it is set to as-yet undeveloped future API versions, it will likely result in instability and reflect badly on the add-on publishing platform and those who reviewed the addon.

@feerrenrut feerrenrut self-assigned this Dec 12, 2018

@leonardder

This comment has been minimized.

Copy link
Collaborator

commented Dec 12, 2018

Thanks for this extensive write-down!

* A numerical (`float`) representation of the version string like `19.20` E.G. `{year}.{major}{minor}`
  
  * If there were more than 10 minor releases, ordering is violated.

This is very unlikely to happen.

* A tuple, structured in the same way as the build version tuple.
  
  * Rather than getting the file version info from the launcher executable, the server would provide the information with other update information.

Note that we would still have to parse a string within updateCheck.checkForUpdate. Alternatively, the update server should return json data for versions of NVDA that support it.

  * The tuple would have to be parsed from the addon manifest (as it currently is), however we can be strict about the format of this.

I believe that configobj can parse them quite will. a definition would look like:
lastTestedAPIVersion = 2018, 4, 10
We can even enforce the length of the tuple, though I'd rather see a minimum length of 2 (year.major)

My preference is to use the tuple representation.

Agreed!

When there is an incompatibility, we would no longer state merely that the addon is untested and allow the user to override the incompatibility state. Instead, we will warn that it is incompatible, and the addon will be disabled after installation. The user should always be able to abort the installation.

That basically means the following:

  • The incompatible add-ons dialog no longer utilizes a checkable list.
  • The install/update process only warns, but it never touches the addons state, as it does now. Actually the addon compatibility state can be removed.

Apart from that, I think that the gui wording and experience is pretty ok as it is now. Thus, I propose keeping the "I understand that these add-ons will be disabled" checkboxes, etc.

One improvement could be to specifically detect and highlight incompatible addons that are providing the active synth / braille display. Since these are considered critical to the user.

This makes sense, and is pretty straight forward when the add-on is running:

>>> import addonHandler
>>> import braille
>>> addonHandler.getCodeAddon(braille.handler.display.__class__)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "addonHandler\__init__.pyc", line 534, in getCodeAddon
AddonError: Code does not belong to an addon package.
>>> addonHandler.getCodeAddon(speech.getSynth().__class__)
<addonHandler.Addon object at 0x04296DD0>

In my case, this only applies to the speech synthesizer.

@feerrenrut

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

Note that we would still have to parse a string within updateCheck.checkForUpdate

Yes, but I'm not so worried about this, since we can be strict about the formatting of this. Unlike the version string coming from the executable's file info.

I believe that configobj can parse them quite will.

I'm happy for the definitions to remain as strings in the addon manifest, and parse them with regex as we currently do. I suggest that we strictly validate that there are 3 numbers provided.

@nvaccessAuto nvaccessAuto added this to the 2019.1 milestone Jan 31, 2019

feerrenrut added a commit that referenced this issue Jan 31, 2019

New addon api versioning approach (PR #9151)
Closes #9055

* The incompatible addons dialog is no longer shown on startup. Since there is no saved state, this would warn about any installed incompatible addons every time NVDA was started.
* Replace compatibility checks
   - NVDA now uses two (internal) values to determine addon compatibility, in addition to the `lastTestedNVDAVersion` and `minimumNVDAVersion` from the addonManifest. These new values are `CURRENT` and `BACK_COMPAT_TO`, in the addonAPIVersion module.
  - An addon is considered compatible if there is an overlap of the two ranges formed by the addon manifest values and the internal NVDA values.
  - Renamed `_showAddonUntestedDialog`
    - Clarified the name of the function to report that an addon is no longer supported by NVDA. The addon is considered "too old". As opposed to NVDA being "too old" for a newly developed addon.
* Introduce a new method of stopping incompatible addons from "running".
  - No longer "disable" them to stop them from running.
  - When being set to disabled, the state was being saved. This meant the launcher could interfere with an installed version of NVDA, even if the installation was aborted.
  - We rely on `_getAvailableAddonsFromPath` to be setting the blocked state.
  - Improve comment in `addToPackagePath`, which is currently the end point for "enabling" an addon.
* Addon API version values are now used as tuples throughout code.
  - Read from addon manifest and validate and convert to tuple.
  - Validate manifest condition: `minRequiredVersion <= lastTested`
    - It's not sensible to have a minRequiredVersion later than the version
tested against.
* Update info for NVDA now keeps `APIVersion` and `backCompatibleToVersion` as tuples rather than strings.
  - When an update is postponed it is saved as a tuple.
  - This saves confusion about whether its a string or a tuple.
* Fix freeze on exit when destroy not called manually for `AutoWidthColumnCheckListCtrl`
   - Removes need to manually call destroy on `IncompatibleAddonsDialog`
   - `accPropServices` needs to be cleaned up when the control is destroyed. The `Destroy` method is not called by the wx framework, but we can register to receive the event see wxWidgets/Phoenix#630
* Allow the possibility of using a parent window other than the addon manager when installing an addon. Useful in the case of installing via a shell extension, where gui.mainframe should be the parent.
  - Removed coupling between the `installAddon` function and `AddonManager` class
  - Reduced nesting of the `installAddon` function
  - Moved methods to module level. These had no dependence on instance, or class level members.
* Use windows sounds for addon install warning / error dialogs.
* Use context manager for writing to file in `_download` of class `UpdateDownloader` in `updateCheck.py`
  - There were several ways that the file could fail to be closed.
* Unified the way that NVDA versions / API versions are formatted to a string, added tests
  - One function is for outputting a full version string (including build part).
  - The other is for formatting a version string for the GUI (which will remove the minor part when it is zero).
* Moved the regex used to convert addon manifest API versions from strings to tuples into the `addonAPIVersion` module.
* No longer raise error for failed audio ducking when it failed due to "access denied"
  - Ensure that a warning is put in the log for access denied errors when setting audio ducking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.