nvaccess / nvda

NVDA, the free and open source Screen Reader for Microsoft Windows
https://www.nvaccess.org/
Other
2.1k stars 634 forks source link

Add-on packaging #213

Closed nvaccessAuto closed 8 years ago

nvaccessAuto commented 14 years ago

Reported by aleksey_s on 2008-10-27 14:38 as nvda goes much power many optional components might be added. for now we have only brltty and the newfon synth, further anything which will comes on. so i think we need to make some exact way for users to manage nvda components. it might be implemented with help of nsis or a standalone program. from user's side this will be a list of optional components which he can install or remove. currently, these might be followed components:

from developers side, it must be easy manageable list of packages. so, one database accessible from the website with some fields and packages theirselves. based on program with which to implement this feature, packages might to be in different formats. i don't know nsis features, but it may be script to run for optain package. it might just download and run executable (sapi runtime installation), or download, unzip and copy some files into nvda folder. each package might have different license agreement, which user must to accept.

nvaccessAuto commented 12 years ago

Attachment addons-213-1.diff added by ragb on 2012-02-07 21:43 Description: BZR bundle with first prototype of add-on integration.

nvaccessAuto commented 14 years ago

Comment 1 by jteh on 2008-11-02 20:09

nvaccessAuto commented 14 years ago

Comment 2 by Bernd on 2009-02-22 02:45 I think such a page, where users can upload and download there packages will be quite good. Should I open an extra Ticket? If yes, I think we could close this ticket as wontfix.

nvaccessAuto commented 12 years ago

Comment 3 by jteh on 2011-12-13 07:04 Narrowing the scope of this ticket to just cover the packaging part, rather than the repository. For now, the latter can be handled simply using a wiki page on the NVDA web site.

We need add-on packages because many users aren't comfortable with extracting files into specific locations and it is often difficult to explain where files should be extracted. My current thinking is outlined below.

Package format:

Installation:

Management:

Misc technical:

nvaccessAuto commented 12 years ago

Comment 4 by aleksey_s (in reply to comment 3) on 2011-12-13 18:47 quick thoughts: Replying to jteh:

Narrowing the scope of this ticket to just cover the packaging part, rather than the repository. For now, the latter can be handled simply using a wiki page on the NVDA web site.

may be a wiki page of specific format, to be easily programatically parsed for automatic discovering.

  • Packages would be a zip archive with a defined naming convention; e.g. blah.nvdaAddon.zip, nvdaAddon_blah.zip, or blah.nvdaAddon.

the later can be useful for automatic NVDA file association. Installer should associate .NVDAAddon file extension with NVDA for convenience.

  • There would be a small text file in the root dir of the archive providing metadata for the add-on, including name, version, publisher, minimum/maximum compatible NVDA version and description.

Description should be localizable. maybe it is worth making metadata inside the extension python code. This way, if developer does cary about localization, he has a way to return localized strings.

  • The archive would also contain folders containing Python modules for appModules, globalPlugins, synthDrivers and brailleDisplayDrivers as appropriate.

as well as other addon specific files (configuration, localization etc).

  • The package files will be placed in userConfig\addons\packageName.
  • Subdirectories of these directories will be added to the appModules, globalPlugins, etc. Python package paths as appropriate.

Anyway, we need some sort of registry of installed addons. therefore, we can store list of files related to each addon. So, is it worth creating a separate dir for each addon, considering that almost 100% of those will be only one thing (app module, synth, braille display driver or global plugin)?

nvaccessAuto commented 12 years ago

Comment 5 by ragb (in reply to comment 3) on 2011-12-19 19:07 My thoughts on this, as it is something I find important: [to comment:3 jteh:

Narrowing the scope of this ticket to just cover the packaging part, rather than the repository. For now, the latter can be handled simply using a wiki page on the NVDA web site.

We need add-on packages because many users aren't comfortable with extracting files into specific locations and it is often difficult to explain where files should be extracted. My current thinking is outlined below.

+1

Package format:

  • Packages would be a zip archive with a defined naming convention; e.g. blah.nvdaAddon.zip, nvdaAddon_blah.zip, or blah.nvdaAddon.

When I read this the first thing that came to mind was python egg files: http://mrtopf.de/blog/en/a-small-introduction-to-python-eggs/

Those are much coppled with setuptools and such but, if things are done right in those libraries, maybe stuff can be reused. More on that billow.

  • There would be a small text file in the root dir of the archive providing metadata for the add-on, including name, version, publisher, minimum/maximum compatible NVDA version and description.

Do you think that something similar a setup.py won't work? I can see some security problems but the same already applies for any plugin that can execute python code. That would possibly solve the description localization issue pointed by alexei in another comment.

  • The archive would also contain folders containing Python modules for appModules, globalPlugins, synthDrivers and brailleDisplayDrivers as appropriate.

One can add those to python path using pkg_resources machinery quite easily. Actually I think that it is even not necessary to extract the zip file (assuming that will be a zip file) for many packages since python can import from zip files. However that can possibly create problems with loading of not-python data such as dlls, sounds, etc.

Installation:

  • NVDA will provide a menu item to install an add-on package.
  • When activated, the user will be asked to locate and choose the add-on package file.
  • Assuming all checks are okay, the user will be prompted for confirmation to install. Information about the package will be provided here.

Seems good.

Management:

  • A dialog will be provided to manage packages, including uninstallation.
  • Updates could be handled here too, although I think this should be implemented later if we implement it at all.

When I thought about setuptools/distutils/distribute it was also for this purpose. It is very reasonable that some add-on would depend on another one so updating and dependency tracking could be eventually needed. Because one does not need to reenvent things when they exist (unless they don't exist) I think that those might be a starting point.

Misc technical:

  • The package files will be placed in userConfig\addons\packageName.
  • Subdirectories of these directories will be added to the appModules, globalPlugins, etc. Python package paths as appropriate.

Ok, the pkg_resources mentioned above.

Regards,

Rui Batista

nvaccessAuto commented 12 years ago

Comment 6 by ragb on 2011-12-28 22:39 Reading trhough this again:

In my previous comment when it reads pkg_resources it should be pkgutils. pkg_resources another different thing.

Anyway, I've been reading some parts of NVDA source code (to update myself on things) and in my opinio, with the current state of the code, this packaging thing is not as hard as I thought before.

If needed I can work on this since I have to do something else than my master thesis to not get mad... I don't think I have accessibility related skills to help with more iaccessible and friends related stuff but on this field I can colaborate. So, if you want I can try to implement this packaging thing, or at least some parts of it.

nvaccessAuto commented 12 years ago

Comment 7 by jteh (in reply to comment 4) on 2011-12-28 23:09 Replying to aleksey_s:

the later can be useful for automatic NVDA file association. Installer should associate .NVDAAddon file extension with NVDA for convenience.

This isn't possible for portable users, so I was a bit reluctant to do something special for the installer, as it makes documentation tricky; i.e. you have to document one method for the installer and one method for the portable. However, I admit it would be simpler this way. Also, I'd prefer an extension like nvdaAddon.zip so that it's clear it's a zip archive and developers don't have to rename it, but Windows extension association may not handle this.

Description should be localizable. maybe it is worth making metadata inside the extension python code.

I thought of doing this with Python code. It would be simpler for a lot of reasons. However, it presents a major security problem. The module could contain harmful code which would be executed before the user had even confirmed installation of the package, which is very bad. Perhaps the metadata file will have to contain text for each locale. Ugly.

Anyway, we need some sort of registry of installed addons. therefore, we can store list of files related to each addon. So, is it worth creating a separate dir for each addon

That's what I said: userConfig\addons\packageName. App modules would be in userConfig\addons\packageName\appModules, etc.

nvaccessAuto commented 12 years ago

Comment 8 by jteh (in reply to comment 5) on 2011-12-28 23:18 Replying to ragb:

When I read this the first thing that came to mind was python egg files:

Those are much coppled with setuptools and such but, if things are done right in those libraries, maybe stuff can be reused. More on that billow.

I'm reluctant to use setuptools because it contains a lot of extra stuff that we don't need, which would therefore unnecessarily increase the size of the NVDA distribution.

Do you think that something similar a setup.py won't work? I can see some security problems but the same already applies for any plugin that can execute python code.

That's true, but this will be loaded before the user even accepts installation of the package to display the name, publisher, description, etc. of the package. If the user accepts installation, they accept the possible security risk. However, loading code before the user even accepts this risk is unacceptable.

That would possibly solve the description localization issue pointed by alexei in another comment.

One can add those to python path using ![pkgutils] machinery quite easily.

We already have code to handle this. See config.addConfigDirsToPythonPackagePath. We'd need to modify this slightly.

Actually I think that it is even not necessary to extract the zip file (assuming that will be a zip file) for many packages since python can import from zip files. However that can possibly create problems with loading of not-python data such as dlls, sounds, etc.

I think it's best to always extract, as a lot of packages will depend on data which must be extracted.

When I thought about setuptools/distutils/distribute it was also for this purpose. It is very reasonable that some add-on would depend on another one so updating and dependency tracking could be eventually needed. Because one does not need to reenvent things when they exist (unless they don't exist) I think that those might be a starting point.

I'm happy for you to investigate this and report here. However, I am very concerned about unnecessary code in setuptools. Also, handling dependencies is problematic, as then you have to be concerned about automatic downloads, etc., not to mention the GUI for this. That said, I haven't actually investigated setuptools at all yet.

nvaccessAuto commented 12 years ago

Comment 9 by ragb (in reply to comment 7) on 2011-12-29 18:15 Replying to jteh:

Replying to aleksey_s:

Description should be localizable. maybe it is worth making metadata inside the extension python code.

I thought of doing this with Python code. It would be simpler for a lot of reasons. However, it presents a major security problem. The module could contain harmful code which would be executed before the user had even confirmed installation of the package, which is very bad. Perhaps the metadata file will have to contain text for each locale. Ugly.

I guess that is the simplest option. NVDA installer already does something like this. Maybe this implies the usage of a more structured format for the plugin manifest data (say xml, json, etc). This can also be more extensible for the future than .ini files for instance.

nvaccessAuto commented 12 years ago

Comment 10 by ragb (in reply to comment 8) on 2011-12-29 18:20 Replying to jteh:

Replying to ragb:

When I read this the first thing that came to mind was python egg files:

Those are much coppled with setuptools and such but, if things are done right in those libraries, maybe stuff can be reused. More on that billow.

I'm reluctant to use setuptools because it contains a lot of extra stuff that we don't need, which would therefore unnecessarily increase the size of the NVDA distribution.

That is really something to take in consideration.

Do you think that something similar a setup.py won't work? I can see some security problems but the same already applies for any plugin that can execute python code.

That's true, but this will be loaded before the user even accepts installation of the package to display the name, publisher, description, etc. of the package. If the user accepts installation, they accept the possible security risk. However, loading code before the user even accepts this risk is unacceptable.

Yes. Python code for the plugin data does not work.

One can add those to python path using ![pkgutils] machinery quite easily.

We already have code to handle this. See config.addConfigDirsToPythonPackagePath. We'd need to modify this slightly.

Yes, I checked that. That is great.

Actually I think that it is even not necessary to extract the zip file (assuming that will be a zip file) for many packages since python can import from zip files. However that can possibly create problems with loading of not-python data such as dlls, sounds, etc.

I think it's best to always extract, as a lot of packages will depend on data which must be extracted.

Yes, it esimplifies things alot. Python zipfile module has all it is needed, I think.

When I thought about setuptools/distutils/distribute it was also for this purpose. It is very reasonable that some add-on would depend on another one so updating and dependency tracking could be eventually needed. Because one does not need to reenvent things when they exist (unless they don't exist) I think that those might be a starting point.

I'm happy for you to investigate this and report here. However, I am very concerned about unnecessary code in setuptools. Also, handling dependencies is problematic, as then you have to be concerned about automatic downloads, etc., not to mention the GUI for this. That said, I haven't actually investigated setuptools at all yet.

Will do.

nvaccessAuto commented 12 years ago

Comment 11 by jteh on 2012-01-05 08:04 I investigated setuptools a bit.

nvaccessAuto commented 12 years ago

Comment 12 by jteh on 2012-01-05 08:05 I think NVDA's version numbering scheme is incompatible with the scheme required by Python, setuptools, etc., so this will cause problems if we want to use setuptools.

nvaccessAuto commented 12 years ago

Comment 13 by jteh on 2012-01-05 08:06 Changes: Changed title from "EAdd-on packaging" to "Add-on packaging"

nvaccessAuto commented 12 years ago

Comment 14 by ragb (in reply to comment 12) on 2012-01-05 11:37 Replying to jteh:

I think NVDA's version numbering scheme is incompatible with the scheme required by Python, setuptools, etc., so this will cause problems if we want to use setuptools.

Is this also valid for stable releases or just snapshots from BZR? I can't get with anything in i.g. 2011.3 or 2012.1beta1 or any of the variations used that goes against the versions schema described in setuptools. With snapshots that is true, starting from the usage of dashes between thre brance's name and revision number.

nvaccessAuto commented 12 years ago

Comment 15 by ragb on 2012-01-10 01:19 Reflecting further about this, I am more inclined to wards setuptools and egs. The y2exe problems may be solved later, eg-info may be included in the package and put on the eg path some how. To be sure I must really try it.

Regarding the versionnning schema, can't you change the snapshots versioning to comply with setuptools? It may provide mroe benefits later. With the repackaging code things will change in some ways so that could be an oportunity. Just an opinion though.

So, shall I start coding something in this setuptools / eg ppackages path?

nvaccessAuto commented 12 years ago

Comment 16 by jteh (in reply to comment 15) on 2012-01-10 01:29 Replying to ragb:

Reflecting further about this, I am more inclined to wards setuptools and egs.

Any particular reason? I'm starting to think eggs are far more trouble than they're worth. We still have to mess about with modifying path on all packages unless we use entry points. Using entry points requires a lot of change which will either break backwards compatibility or make things confusing; e.g. different way of developing for internal versus add-ons.

Regarding the versionnning schema, can't you change the snapshots versioning to comply with setuptools?

This must take code running from source into account as well. Snapshot and source versions would end up being something like 2012.1a0.dev-bzr-main-4883, which is just hideous. It would probably change the output filename as well, which would require significant changes to the automatic build scripts. Branches other than main can be between releases (i.e. not yet 2012.2 but not 2012.1 either. Finally, 2012.1a0 is obviously considered to be before 2012.1, but this means that any add-ons for snapshots have to specify 2012.1a0, which is pretty ugly.

I'm also not convinced that dependencies are even a good idea. This is going to become incredibly complicated, as users will then expect that dependencies download automatically, etc. This must be handled when a package is removed as well.

nvaccessAuto commented 12 years ago

Comment 17 by ragb (in reply to comment 16) on 2012-01-18 23:06 Sorry for the late response, real life got in the way.. Replying to jteh:

Replying to ragb:

Reflecting further about this, I am more inclined to wards setuptools and egs.

Any particular reason? I'm starting to think eggs are far more trouble than they're worth.

My rational is to got things more pythonic. Developpers used to python, setuptools, and easy_install might benefit from this kind of tools. However in the windows world things are not so straight-forward (I've been doing some python web development lately and in this case easy_install/distribute/... realy shine). NVDa itself is not packaed in that decoupled way, and honestly I don't think it would be worthwhile the effort to make it. yI believe ou have invested much time on the build infrastructure.

We still have to mess about with modifying path on all packages unless we use entry points. Using entry points requires a lot of change which will either break backwards compatibility or make things confusing; e.g. different way of developing for internal versus add-ons.

Yes. And changing to entry-points would break backwards compatibility. The current appmodules/brailleDisplayDrivers/... code must be kept arround. And the step curve for casual developers may be to high for casual developers (most appmodule developers are, so I believe).

Regarding the versionnning schema, can't you change the snapshots versioning to comply with setuptools?

This must take code running from source into account as well.

Hmmm. Ok. Forgot that.

Snapshot and source versions would end up being something like 2012.1a0.dev-bzr-main-4883, which is just hideous.

Many projects use that though... Sounds funy but not practical. And might confuse users used to current versioning schema. Snapshots are used by many people dispite beeing declared not stable. And user support nightmare would come...

It would probably change the output filename as well, which would require significant changes to the automatic build scripts. Branches other than main can be between releases (i.e. not yet 2012.2 but not 2012.1 either. Finally, 2012.1a0 is obviously considered to be before 2012.1, but this means that any add-ons for snapshots have to specify 2012.1a0, which is pretty ugly.

HMM Ok. Got it. Confusing...:)

I'm also not convinced that dependencies are even a good idea. This is going to become incredibly complicated, as users will then expect that dependencies download automatically, etc. This must be handled when a package is removed as well.

Ok. The main usecase is for stand-alone snapshots and may be bettter to stick to that for now.

nvaccessAuto commented 12 years ago

Comment 18 by ragb on 2012-01-18 23:27 My ideas about the addon format, based on the discussion until now:

What do yout think?

nvaccessAuto commented 12 years ago

Comment 19 by jteh on 2012-01-18 23:54 Changes: Milestone changed from 2012.1 to 2012.2

nvaccessAuto commented 12 years ago

Comment 20 by ragb on 2012-01-29 15:08 Regarding the manifest file format I think that config file sintax would work. I didn't too well the configobj library so I was a bit reticent to that. But as it support lists and such the extensibility needs I was thinking are well-covered. I'm prototyping some of these ideas right now. Will point for a branch when I find somewhere to host it (probably launchpad).

nvaccessAuto commented 12 years ago

Comment 21 by ragb on 2012-01-30 23:56 Hi,

Here it is a preliminary specification of the manifest for an NVDA add-on. This only considers the add-on directory similar to what already exists on userconfig: pre-defined forlders and names for specific extensibility points. This is in configobj configspec format, because it is what I'm using for prototyping the idea (see previous comments).

# NVDA Add-on Manifest configuration specification
# Add-on name
name = string()
# Quick description of the add-on to show to users.
description = string()
# Long description with further information and instructions
long_description = string(default=None)
# Name of the author or entity that created the add-on
author = string()
# Version of the add-on. Should preferably in some standard format such as x.y.z
version = string()
# URL for more information about the add-on. New versions and such.
url= string(default=None)
# Categories where this add-on belongs to. See list (to be defined)
categories = list(default=list())

# Compatibility with NVDA releases
# Only considering stable versions.
[Minimum version compatible with this add-on.
minVersion = string(default=None)
# Maximum compatible version.
maxVersion = string(default=None)

# Extra data to bundle with the add-on.
# Python files and packages are bundled by default.
# Must be relative to the base directory where are this manifest.
[data](compatibility]
#)
# Extra files to include. Can use glob expressions.
include = list(default=list())
# Directories to include recursively.
include_recursive = list(default=list())

This is a draft. I'm not considering compatibility with snapshots (how to do that?) nor explicit entry-points (something it is worth think about in my opinion) but as I said, this is just a prototype. Already implemented bundling of add-ons based on this manifest spec and I'm in the process of implementing extraction for userconfig/packages/ and patching NVDA to use code from those packages. Possible next steps may be, according to feedback:

nvaccessAuto commented 12 years ago

Comment 22 by jteh on 2012-01-31 00:34 As per comment:11, I'm wondering whether we should just not bother with textual package metadata at all, as it creates a localisation issue that isn't easy to solve. Initially, the author would just provide information on whatever web site in their preferred language (English for the official web site, but other communities could I guess host package indexes as well). Eventually, there may be some sort of global package index which provides information in multiple languages.

I'm also not sure there's really a need for a manifest of files. I'd think any files in the archive would just be extracted and the manager would handle tracking of installed files.

nvaccessAuto commented 12 years ago

Comment 23 by ragb (in reply to comment 22) on 2012-01-31 11:42 Replying to jteh:

As per comment:11, I'm wondering whether we should just not bother with textual package metadata at all, as it creates a localisation issue that isn't easy to solve. Initially, the author would just provide information on whatever web site in their preferred language (English for the official web site, but other communities could I guess host package indexes as well). Eventually, there may be some sort of global package index which provides information in multiple languages.

I understand your concerns about the localization issue, it surely requires additional work. ut eventualy we will have the need for a package index and users will ask for localization... If present in package metadata one can already use it for the NVDA interface and the index, keepping that information centralized. The first solution that comes to mind for this purpose is extending the metadata file with a localization section, like the folowing:

...
[= Descrição do meu addon
long_description = """ uma descrição mais detalhada
do meu add-on"""
[[es](i18n]
[[pt_pt]]
description)]
... # the same for spanish, I won't try :)...

This requires the translator to edit the metadata file or provide it to the author. But gettext integration can be investigated, as it is already done in intltool-merge on Linux. I think the first solution is easier to everyone though. In the meantime I think that, for an initial phase we could have this localization issue open but try to come with something working first, without thinking too much about localization.

Regarding the metadata itself (name, description, url...) I think it is really necessary so the user can get more information about the installed add-ons or some add-on he wants to install. Sadly most existing plugins that are hanging around are distributed on mailling-lists, twitter, drobox, etc. As we can not change that so easily at least plugin makers can provide some information without having users consult a website. This is just a rational. Compatible versions and such information must be provided somewhere though.

I'm also not sure there's really a need for a manifest of files. I'd think any files in the archive would just be extracted and the manager would handle tracking of installed files.

I'm not sure of any of the alternatives: bundle whatever tat is on the add-on directory or explicitly require the data to be bundled. Bundling everything creates the the situation where some less experienced people would bundle many stuff that is nod needed: hiden files, backup files (those ending with ~ or .swp), python source files (python bytecode alone works), gettext source translations, and so on. Not providing the source may be seen as bad, but add-on makers can delete .py files before zipping for the same purpose... Explicitly requiring metadata listing (python modules and packages ofcourse are implicit) puts more work on the hands of the add-on maker though.

nvaccessAuto commented 12 years ago

Comment 24 by ragb on 2012-02-07 21:50 This patch is the first prototype at integrating add-ons.

For now addons are read from ./addons or userConfig/addons. Directories with a manifest.ini file (spec defined on AddonManifest class) are considered valid and loaded. If the add contains the usual extension directories (brailleDisplayDrivers, appModules,...) those are add to the modules python path. The problem is that for testing must be created by hand. A sample manifest.ini for these purposes is as follows:

name = sampleAddon
description = My sample addon package
author = sample author
version = 0.1

In the mentioned addons directory create a sampleAddon directory and a manifest.ini file with those contents. Then put some plugin or something there to test.

Note that this is not done, I'm submitting for early reviewing.

nvaccessAuto commented 12 years ago

Comment 25 by ragb on 2012-02-20 18:29 Hi,

I' m slowly making progress on this. Some features I thinkk are good to implement:

I'm still developing on a local branch. I'm having issues with launchpad. Is it possible and easy for you to provide a branch on nvaccess servers?

nvaccessAuto commented 12 years ago

Comment 26 by jteh (in reply to comment 25) on 2012-02-20 20:56 Replying to ragb:

I' m slowly making progress on this. Some features I thinkk are good to implement:

These are all interesting ideas, but they are outside the scope of this ticket. They should all be addressed in separate tickets (and therefore separate branches).

I'm still developing on a local branch. I'm having issues with launchpad. Is it possible and easy for you to provide a branch on nvaccess servers?

Can you be more specific about the issues you're having with Launchpad?

nvaccessAuto commented 12 years ago

Comment 27 by ragb (in reply to comment 26) on 2012-02-20 21:50 Replying to jteh:

Replying to ragb:

I' m slowly making progress on this. Some features I thinkk are good to implement:

These are all interesting ideas, but they are outside the scope of this ticket. They should all be addressed in separate tickets (and therefore separate branches).

Ok. Will create those when applicabe.

I'm still developing on a local branch. I'm having issues with launchpad. Is it possible and easy for you to provide a branch on nvaccess servers?

Can you be more specific about the issues you're having with Launchpad?

I can't commit to launchad under windows. It's something related with my ssh publi key, but I never understood the problem. I didn't create a nvda specific branch yet, but it happened on other projects. Under linux everything goes well. Will try again shortly. Btw, the lp:nvda branch on launchpad is not beeing mirrored since 2011-07-13.

nvaccessAuto commented 12 years ago

Comment 28 by jteh (in reply to comment 27) on 2012-02-20 21:57 Replying to ragb:

I can't commit to launchad under windows. It's something related with my ssh publi key, but I never understood the problem.

You will probably have the same problem on whatever server you use. I'd suggest further debugging on this. I'm happy to help via nvda-dev if you can provide error messages and the like.

Btw, the lp:nvda branch on launchpad is not beeing mirrored since 2011-07-13.

Damn. Thanks for reporting. I shall investigate.

nvaccessAuto commented 12 years ago

Comment 29 by ragb (in reply to comment 28) on 2012-02-21 00:09 Replying to jteh:

Replying to ragb:

I can't commit to launchad under windows. It's something related with my ssh publi key, but I never understood the problem.

You will probably have the same problem on whatever server you use. I'd suggest further debugging on this. I'm happy to help via nvda-dev if you can provide error messages and the like.

Ok, problem solved. I lost my commit messages when merging (bzr seems to be a bit different from git) but have created a branch with my changes. Hopefully no more problems.

Branch is on

lp:~ruiandrebatista/nvda/addons-packaging

nvaccessAuto commented 12 years ago

Comment 30 by ragb on 2012-02-21 19:41 Report of current situation, as per revision 5018 of lp:~ruiandrebatista/nvda/addons-packaging

Addons are directories with a similar structure to actual userConfig directory: they contain !brailleDisplayDrivers, !globalPlugins, etc. An add-on is installed to userConfig\packageName. When the addon files are installed the !brailleDisplayDrivers, !globalPlugins, etc... directories are added to the respective package paths, as it is already done for the userConfig directory. config.addConfigDirsToPythonPackagePath was slightly changed for this purpose.

Metadata for each addon is contained in \manifest.ini. This file is a common .ini file, loaded using configObj. The specification is as follows:

# NVDA Ad-on Manifest configuration specification
# Add-on name
name = string()
# Quick description of the add-on to show to users.
description = string()
# Long description with further information and instructions
long_description = string(default=None)
# Name of the author or entity that created the add-on
author = string()
# Version of the add-on. Should preferably in some standard format such as x.y.z
version = string()
# URL for more information about the add-on. New versions and such.
url= string(default=None)
# Categories where this add-on belongs to.
categories = list(default=list())

# Compatibility with NVDA releases
# Only considering stable versions.
[compatibility]
# Minimum version compatible with this add-on.
minVersion = string(default=None)
# Maximum compatible version.
maxVersion = string(default=None)

NVDA add-ons are distributed in packages called bundles wich are simple zipfiles with the add-on directory contents, including the manifest file. Addon bundle name must be in the form -.nvdaadon. Functions are provided to install and extract add-ons to userConfig/addons and to create a new add-on bundle from a directory. See below. For now all present add-ons are considered active but an interface must be provided for the user to manage all add-ons, install, activate, deactivate, and uninstall.

The !addonHandler.py module manages the addOns subsystem. Method initialize loads all available add-ons and activates them. terminate does the oposite. installAddonBundle receives a path to a addon bundle file and install and extracts its contents to userConfig/addons. createAddonBundleFromPath creates a add-on package file suitable for distribution from a directory that contains a manifest.ini file. Last two methods work fine from the NVDA Python console, while no interface is produced.

Some classes are worth mention: addonHandler.Addon represents an installed add-on. It has methods to activate, deactivate, get the manifest metadata and more. addonHandler.AddonBundle represents a bundle package (zipfile) and has methods to extract, retrieve manifest data without extracting, etc. addonHandler.AddonManifest is the manifest metatada, a simple subclass of configobj.ConfigObj.

In my opinion, the only thing that is missing for the complition of this task (not refering testing and such) is the itnerface to manage add-ons. All the other ideas are worth separate tickets. Do you agree?

nvaccessAuto commented 12 years ago

Comment 31 by ragb on 2012-03-22 17:49 Should this be targeted for 2012.2??

nvaccessAuto commented 12 years ago

Comment 32 by jteh on 2012-04-08 08:28 @ragb: Just so you know, I do plan to review your code, probably in the next few weeks. We've been very snowed under with other work for a while. :)

This is already scheduled for 2012.2.

nvaccessAuto commented 12 years ago

Comment 33 by jteh on 2012-04-18 06:33 This code looks very good and functional. Very nice work.

Questions:

Problems:

Some code review:

Mionr code nits:

I'll probably have more later.

nvaccessAuto commented 12 years ago

Comment 34 by jteh on 2012-04-18 10:43 I forgot to mention that I was also thinking it'd be nice to have gettext translations for add-ons. To do this, there would be a locale directory inside the add-on package with the usual structure (locale\xx_xx\LCMESSAGES\nvda.mo). There would also be a function (e.g. addonHandler.initTranslation) which would set everything up for the current module so that translatable strings (e.g. ("foo")) would just work. I have ideas on how to do this last part, so am happy to write the code if this idea isn't insane.

nvaccessAuto commented 12 years ago

Comment 35 by ragb (in reply to comment 33) on 2012-04-18 11:42 Hi, Replying to jteh:

This code looks very good and functional. Very nice work.

Thank you.

Questions:

  • What do you imagine hooks being used for? One possibility I see is an installation hook; e.g. to gather data from the user, etc.

That is one of my main use cases. Another one is for nvda to tell the plugin that it's user interface is started so the addon can add to menus, etc. One can also do that using a globalPlugin, but in my opinion that would be clearer. Moreover, beeing hook names strings, add-on developers can define their own hooks, and addons can communicate with one another (do we realy want that to happen? :)). I think the system is extensible for use cases we can't even imagine but if you think that it is too overkill that is fine for me.

Problems:

  • If we are going to include text strings in the manifest, they definitely need to be translatable.
    • One possibility I've been considering is to have a locale/xx/manifest.ini file which contains just the translatable strings for each language. The main manifest.ini would still contain the English strings, as they are the default. Having them in separate files makes translation easier when there are multiple people working on a single addon.

This seems a good idea. However I only see the need to translate description and possibly the name of the addon. But we can think of more and that approach will account for the need.

  • I'm not sure unloading add-ons or removing add-ons without restart is something we should support at all initially. Aside from the fact that it makes things more complicated, NVDA can't currently unload synth drivers or braille display drivers and we'd have to implement checks to ensure an add-on didn't contain an active driver, etc.

Makes sence. I thought about that a bit but was not sure how to approach that. Shall we force a restart on addon installation? If having a GUI to isntall and remove addons, we may only do the restart when changes are "commited" (pressing the ok or save button for instance).

  • I'm not saying we should never support this, but if we do in the initial version, it's going to delay inclusion and imo it isn't that important.

Agreed.

Some code review:

  • In the copyright header, you can specify your own name rather than NVDA contributors. This way, we know who owns the copyright for various sections. Others can be added if others contribute to this file.

Ok.

  • Any reason you're using StringIO instead of cStringIO? If not, use cStringIO for better performance.

No reason that I recall, probably a typo.

  • I'd prefer the extension ".nvda-addon" instead of ".nvdaadon".

Ok, that is good.

  • I don't think we should allow addons in the source directory. Otherwise, they aren't really add-ons and should be made part of the distribution.

My idea when doing that was to allow easy building of portable version, already including some addons. But with the current unified launcher that might impose problems (load the addons already on the temporary copy?). Honestly I now do agree that it doesn't make much sence and if someone needs that later it can be handled at that time.

  • That said, you should still leave the support for multiple add-on directories, as we may have a system wide configuration directory eventually.

I agree.

  • The hook for unloading is named deunload. Should this just be unload?

Yes. Typo. I was calling it deactivate and forgot the "d" there.

  • Addon.loadModule creates a full name of addonName.moduleName. I think addons.addonName.moduleName might be better to avoid possible conflicts.

Good point.

  • We definitely need to find a better way to handle the hack in config.addConfigDirsToPythonPackagePath. Among other things, this can't handle dynamically loading new add-ons at runtime.

Yes. However, if we do the restart at addon loading this might not be a problem, at least for now. But I agree that it is a hack.

Mionr code nits:

  • No need to explicitly import os, as os is implicitly imported by os.path.
  • When a docstring refers to a Python identifier within NVDA, it should link it; e.g. L{Addon}.
  • There's no need for a pass statement if there is a docstring; e.g. the !AddonError class.
  • When documenting that a function raises an exception, the docstring should use @raise; e.g. @raise IOError: If removing a file fails.
  • When raising exceptions, the parenthesised version is preferred over the comma separated version; e.g. raise AddonError("blah") instead of raise AddonError, "blah"

Ok. Thanks.

Regards,

Rui Batista

nvaccessAuto commented 12 years ago

Comment 36 by ragb (in reply to comment 34) on 2012-04-18 11:56 Hi again, Replying to jteh:

I forgot to mention that I was also thinking it'd be nice to have gettext translations for add-ons. To do this, there would be a locale directory inside the add-on package with the usual structure (locale\xx_xx\LCMESSAGES\nvda.mo). There would also be a function (e.g. addonHandler.initTranslation) which would set everything up for the current module so that translatable strings (e.g. ("foo")) would just work. I have ideas on how to do this last part, so am happy to write the code if this idea isn't insane.

I fully agree with this feature. However we may encounter some problems:

Do you have any other idea?

nvaccessAuto commented 12 years ago

Comment 37 by jteh (in reply to comment 36) on 2012-04-18 13:21 Replying to ragb:

  • The approach people are using to translate plugins is to directly setup gettext on the modules that need translation (pointing it to e specific locale dir and domain). But that has the problem that every module must have some sort of setup, even if it is just one line.

I agree this is a minor nuisance, but as long as that line isn't too ugly (I'm imagining something as simple as addonHandler.initModuleTranslation()), I think this is an acceptable situation.

  • As far as I know, NVDA installs gettext on the __builtin__ module. I don't know if it is possible to have different gettext instances installed for different modules...

If _ (and potentially other identifiers) are defined in the module, they will override the builtin. The init function will handle this.

  • We could recursively monkey-patch al python modules on the addon to add the _ function, but it might not be diserable to import all python modules at once.

This isn't an option. There could be translatable strings at module and class level. Translation of these will happen at import time, meaning that monkey patching will happen too late.

Do you have any other idea?

One nasty idea I just came up with is to write our own _ function (and put it in builtins) which looks at the stack to determine what module the call came from and selects the correct gettext instance accordingly. I'm not sure about performance here. However, I think I still prefer the initialisation approach.

nvaccessAuto commented 12 years ago

Comment 38 by jteh (in reply to comment 35) on 2012-04-18 13:29 Replying to ragb:

  • What do you imagine hooks being used for? One possibility I see is an installation hook; e.g. to gather data from the user, etc.

That is one of my main use cases. Another one is for nvda to tell the plugin that it's user interface is started so the addon can add to menus, etc.

I am against this and think this should still be done in a global plugin. Otherwise, the lines between global plugins, app modules and drivers become too blurred, which could lead to confusion and other problems. Nevertheless, I'm happy to leave the hook mechanism present, even if it's not used much.

  • If we are going to include text strings in the manifest, they definitely need to be translatable.
    • One possibility I've been considering is to have a locale/xx/manifest.ini file which contains just the translatable strings for each language.

This seems a good idea. However I only see the need to translate description and possibly the name of the addon.

Agreed. I know having separate files seems like overkill, but I figured that individual translators would find it easier if their strings were kept separate. I'd appreciate feedback from others on this as well. Doing it that way certainly makes the code more complicated, so I'm happy to keep them all in the one manifest if everyone is happy with that. :)

  • I'm not sure unloading add-ons or removing add-ons without restart is something we should support at all initially.

Makes sence. I thought about that a bit but was not sure how to approach that. Shall we force a restart on addon installation? If having a GUI to isntall and remove addons, we may only do the restart when changes are "commited" (pressing the ok or save button for instance).

Something like that sounds good. Let's cover that when we start working on the GUI. :)

Thanks for your work.

nvaccessAuto commented 12 years ago

Comment 39 by ragb (in reply to comment 38) on 2012-04-18 14:10 Replying to jteh:

Replying to ragb:

  • What do you imagine hooks being used for? One possibility I see is an installation hook; e.g. to gather data from the user, etc.

That is one of my main use cases. Another one is for nvda to tell the plugin that it's user interface is started so the addon can add to menus, etc.

I am against this and think this should still be done in a global plugin. Otherwise, the lines between global plugins, app modules and drivers become too blurred, which could lead to confusion and other problems. Nevertheless, I'm happy to leave the hook mechanism present, even if it's not used much.

The only small problem I see with having GUI construction (and destruction if dynamic loading is to be implemented) on a globalPlugin is that we are relying on the fact that the GlobalPlugin constructor is called after GUI initialization. That is, we are relying on module initialization order. At least this should be documented or, in alternative, create something like a {{create_guimethod on theGlobalPlugin``` class for plugins to override, wich is guarantied to be called at the right time, and may be more explicit. This may belong to a new ticket, though.

nvaccessAuto commented 12 years ago

Comment 40 by jteh (in reply to comment 39) on 2012-04-18 14:20 Replying to ragb:

The only small problem I see with having GUI construction (and destruction if dynamic loading is to be implemented) on a globalPlugin is that we are relying on the fact that the GlobalPlugin constructor is called after GUI initialization. That is, we are relying on module initialization order.

That's a good point. I can't see any reason we would ever initialise gui after plugins, though, so:

At least this should be documented

I agree.

nvaccessAuto commented 12 years ago

Comment 41 by jteh on 2012-04-19 08:43 Rui, are you likely to have a chance to look at implementing these changes over the next few days? If you're busy, I'm happy to do this myself, as I'd like to move this further forward. Thanks!

nvaccessAuto commented 12 years ago

Comment 42 by ragb (in reply to comment 41) on 2012-04-19 11:29 Hi, Replying to jteh:

Rui, are you likely to have a chance to look at implementing these changes over the next few days? If you're busy, I'm happy to do this myself, as I'd like to move this further forward. Thanks!

The code cleanups you sugested and some other corrections are done, I will commit them shortly. I think I can implement the gettext support you sugested this wikend. However, implementing something more complex like a GUI would take a bit more time, so I can not tell exactly when I have time to work on that. If you want to start on that path we could work in paralle, but I don't know if there something more to do before starting working on GUIs and stuff.

regarding manifest translation, I was thinking about something. Supose we want to have some sort of add-on repository on the future. That repository could use manifest information directly to read add-on information. However, if we have translation scathered among many files that sort of processing would be harder. Anyway, this doesn't make things impossible by any means, just a bit more complex. Just a thought.

nvaccessAuto commented 12 years ago

Comment 43 by ragb (in reply to comment 42) on 2012-04-19 13:43 Replying to ragb:

The code cleanups you sugested and some other corrections are done, I will commit them shortly.

Pushed to lp:~ruiandrebatista/nvda/addons-packaging. revision 5025.

Regards,

Rui Batista

nvaccessAuto commented 12 years ago

Comment 44 by ragb (in reply to comment 42) on 2012-04-20 16:16 Hi,

I coded a prototype for the translation support on lp:~ruiandrebatista/nvda.

This code may be a bit hackish please review... Inspection and such is something new for me so I might be doing things somehow wrong.

nvaccessAuto commented 12 years ago

Comment 45 by ragb on 2012-04-24 20:02 Just to note that manifest translation was implemented e r5134 of the launchpad branch. Data in locale\xx\manifest.ini on the addon. See the revision commit message for details.

nvaccessAuto commented 12 years ago

Comment 46 by mdcurran on 2012-04-30 05:39 Recently I changed things so that addons that tried to get removed but failed were renamed to blah.removed and then cleaned up on NVDA restart. I think though that either deleting or renaming addon directories is extremely dangerous unless the addon is definitely not being used. In the ase of a synth driver this is a little tricky, and globalLugins... they can have their terminate method called but there's no guarantee that pulling the files out from underneath it its not going to completely break the running NVDA. Therefore, I would like to propose a different change:

Thoughts?

nvaccessAuto commented 12 years ago

Comment 47 by jteh (in reply to comment 46) on 2012-04-30 09:44 I'm happy with this idea. Just a couple of minor suggestions: Replying to mdcurran:

  • If an addon is added, its copied in to the addons dir, but with the string "-pending" appended to its directory name.

Consider changing this to ".pending". The reason is that people are less likely to use "." than "-" in add-on names, so we could reserve "." for special things like this.

  • The Addon class could have two new properties: isNew and isRemoved. isNew would be true if the path has -pending on the end.

I wonder whether isPending is a more accurate name here.

nvaccessAuto commented 12 years ago

Comment 48 by mdcurran on 2012-04-30 10:16 either isPending, or or us .new instead of .pending for the dir name. I'd possible prefer new over pending as its a little more descriptive ... pending what exactly? :) Re changing - to a . I agree with that.

nvaccessAuto commented 12 years ago

Comment 49 by jteh on 2012-04-30 10:22 The problem with "new" is that it's not clear that it isn't yet actually installed; e.g. it could just be new as in recently added. However, I agree pending is equally ambiguous. Being pedantic, we probably want ".pendingInstall", but I'm happy to go with "new" for brevity as long as the isNew property is clearly documented. :)