Add option to install and uninstall specified files for IDA plugins #1390
Conversation
There was a problem hiding this comment.
Thanks for the contribution @Schamper! 💐 can you please increase the version in the nuspec of common.vm.nuspec as explained in our documentation? We need increase the version, otherwise the existent version from MyGet is still used. The linter is complaining about it. 😉
I'll review the rest thoughtfully next week, I am sorry but I won't have time this week.
| description: | | ||
| SHA256 hash of the file or ZIP downloaded from the download url introduced in the previous field. The hash is used for verification purposes. Example: `62af5cce80dbbf5cdf961ec9515549334a2112056d4168fced75c892c24baa95` | ||
| - type: input | ||
| id: files |
There was a problem hiding this comment.
@Schamper are you aiming for this field to be used by our automation? Have you tested the GH action new_package.yml with this change? I think it does not work, as we are using template-path: .github/ISSUE_TEMPLATE/new_package.yml to parse the issue and I think fields not included there are ignored.
| attributes: | ||
| label: Files to copy | ||
| description: | | ||
| A comma separated list of files and/or directories to copy into the plugin directory. Only necessary for plugins with awkward repository layouts. Example: `awkward_directory/awesomeplugin.py, `awkward_directory/awesomeplugin_libs`, |
There was a problem hiding this comment.
[nitpick] I think it would be better to use an real example instead of awkward_directory/awesomeplugin.py.
| $pluginName = 'dereferencing.py' | ||
| $pluginUrl = 'https://github.com/danigargu/deREferencing/archive/ab106c1fa5969a5d7c27c8037b4aec446ad55a4d.zip' | ||
| $pluginSha256 = '83c0f9420075dbe5217f5fbf2413c7b1b72eadfaedfe2474b306e1e3eaae0aa8' | ||
| $pluginFiles = @('dereferencing', 'dereferencing.py') |
There was a problem hiding this comment.
I think this is a bad example to use the $pluginFiles argument as this plugin is installed correctly with the default behavior and this change unnecessarily complicates the code. What about removing this change and run scripts/utils/create_package_template.py to create the IDA plugin ida.plugin.hexrayspytools.vm so that we can test the new argument with an IDA plugin that needs it?
There was a problem hiding this comment.
It fails to uninstall correctly though, how do you propose to deal with that? I figured consistency between installation/uninstallation is more important than the "magicness" of it.
There was a problem hiding this comment.
does dereferencing fails to uninstall with the current implementation? 🤔
There was a problem hiding this comment.
Well technically it doesn't fail, but it doesn't properly uninstall, since it will only remove dereferencing.py:
The dereferencing module folder will remain: https://github.com/danigargu/deREferencing/tree/259685bce8ab4ba0371ec6d13446cc553df2689b/dereferencing
Trying to reinstall the deferencing plugin will then fail because that folder already exists.
| $pluginFiles = @({files}) | ||
| VM-Uninstall-IDA-Plugin -pluginName $pluginName -pluginFiles $pluginFiles |
There was a problem hiding this comment.
I do not like that we know always use the pluginFiles argument even if it is not needed. I think this complicates the code for the default case making it also confusing. What about adding an if to check if files is empty in create_ida_plugin_template and having two different templates (the previous one and the purposed one)?
| del args.type | ||
|
|
||
| # fixup the files argument | ||
| args.files = ", ".join(repr(name.strip()) for name in args.files.split(",")) |
There was a problem hiding this comment.
This fails if files is not provided (it is not required). I purpose to move this code inside create_ida_plugin_template (after checking if files is empty) as it is only used by that function.
2 participants
As discussed in #1230.
Adds the
-pluginFilesargument to bothVM-Install-IDA-PluginandVM-Uninstall-IDA-Pluginto more finely manage the installed plugin files. This allows for more flexible plugin packages without having to make a customchocolateyInstall.ps1for those.Currently a bunch of plugins will fail to reinstall after an uninstall because of leftover files (a reinstall will complain about already existing files), so if this gets merged we probably want to update those plugins with this argument.
Updated
ida.plugin.dereferencing.vmas example with this.