diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index fe7697b5cc2578e626a15df4eedc3a04c73dd912..79c1368575b053de3fd75004e2d05ae64b140b91 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -1,5 +1,5 @@ include: - - project: fsl/fsl-ci-rules + - project: fsl/conda/fsl-ci-rules file: .gitlab-ci.yml stages: @@ -10,9 +10,9 @@ stages: - fsl-ci-test - fsl-ci-deploy -test:3.7: +test:3.8: stage: test - image: python:3.7 + image: python:3.8 tags: - docker @@ -20,9 +20,9 @@ test:3.7: script: - bash ./.ci/unit_tests.sh -test:3.8: +test:3.9: stage: test - image: python:3.8 + image: python:3.9 tags: - docker @@ -30,9 +30,19 @@ test:3.8: script: - bash ./.ci/unit_tests.sh -test:3.9: +test:3.10: stage: test - image: python:3.9 + image: python:3.10 + + tags: + - docker + + script: + - bash ./.ci/unit_tests.sh + +test:3.11: + stage: test + image: python:3.11 tags: - docker @@ -40,9 +50,10 @@ test:3.9: script: - bash ./.ci/unit_tests.sh + build:dist: stage: build - image: python:3.8 + image: python:3.10 tags: - docker diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b9520171329820ed213d1d43fc42be3d4337eb1..1c2ca5eacd9a4eb429380c3f645237f2ec3fb69e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,19 @@ # `fsl_add_module` changelog +## 0.4.0 (Tuesday 24th January 2023) + + - Changed the default behaviour so that only files in the `fsl_course_data` + category are downloaded. This can be overridden by specifying `--category + all`. + - Prefix downloaded files with their category name to reduce the likelihood + of collisions across different categories (e.g. `registration.tar.gz` from + the FSL course and from grad course data sets). Nothing is done to prevent + collisions in extracted paths from different archives though. + - The `--force` option can now be used without specifying any modules - it + causes all modules in the default/selected cateogories to be downloaded. + + ## 0.3.2 (Wednesday 1st September 2021) - Changed the default manifest URL to diff --git a/fsl/add_module/__init__.py b/fsl/add_module/__init__.py index b32ffaa3265f2dc2b352971cd8f22560ca4cbdfe..83da9f81721da6fa8bad53e6120e0ac6ea9c0de5 100644 --- a/fsl/add_module/__init__.py +++ b/fsl/add_module/__init__.py @@ -9,5 +9,5 @@ """ -__version__ = '0.3.2' +__version__ = '0.4.0' """``fsl_add_module`` version number.""" diff --git a/fsl/add_module/messages.py b/fsl/add_module/messages.py index b1aa1a5c5f5148f9e839d45b811c742a719f52a4..ba7665d7edae7e0b7d4d751825a62a3d248fd973 100644 --- a/fsl/add_module/messages.py +++ b/fsl/add_module/messages.py @@ -10,6 +10,7 @@ to standard output, with colour, emphasis, line wrapping, and indentation. import enum +import shutil import textwrap from typing import Union @@ -64,14 +65,15 @@ def printmsg(msg : str, - Any ANSI codes in ``args`` are applied - If ``wrap`` is specified, ``msg`` is split across multiple lines, each having maximum length of ``wrap - indent`` characters. If - ``wrap`` is ``True``, it is set to 80 characters. + ``wrap`` is ``True``, it is set to the current terminal width + (via ``shutil.get_terminal_size()``). - Each line in ``msg`` is indented by ``indent`` space characters, All other keyword arguments are passed through to the ``print`` function. """ if wrap is True: - wrap = 80 + wrap = min((80, shutil.get_terminal_size()[0])) msgcodes = [ANSICODES[t] for t in msgtypes] msgcodes = ''.join(msgcodes) diff --git a/fsl/add_module/plugin_manifest.py b/fsl/add_module/plugin_manifest.py index 8b63c10b2e6b527e6c7e69f62ed103c67bfa8119..1da82cad645f2e314b0aa53765eabb84d098cd57 100644 --- a/fsl/add_module/plugin_manifest.py +++ b/fsl/add_module/plugin_manifest.py @@ -134,6 +134,16 @@ class Plugin: """ + @property + def fileName(self): + """Return a suitable name to use for the downloaded plugin file. + """ + fname = op.basename(urlparse.urlparse(self.url).path) + if self.category not in (None, UNCATEGORISED): + fname = f'{self.category}_{fname}' + return routines.sanitiseFileName(fname) + + class Manifest(dict): """The ``Manifest``class simply a container for :class:`Plugin` instances. A ``Manifest`` is a dict containing ``{name : Plugin}`` mappings. @@ -222,11 +232,14 @@ class Manifest(dict): return list(sorted(set(cats))) - def getCategory(self, category : str) -> List[Plugin]: + def getCategory(self, category : Optional[str]) -> List[Plugin]: """Returns a list of all available plugins in the specified - ``category``. + ``category``. If ``category is None``, all plugins are returned. """ - return list([p for p in self.values() if p.category == category]) + def test(p): + return category in (None, p.category) + + return list([p for p in self.values() if test(p)]) def downloadManifest(url : Union[str, pathlib.Path]) -> Manifest: diff --git a/fsl/add_module/routines.py b/fsl/add_module/routines.py index 59b95ea793064174fa63fec95e552ecd23f5f1a1..cf67997061f54744c0ac8db32a189dd6f47bb37d 100644 --- a/fsl/add_module/routines.py +++ b/fsl/add_module/routines.py @@ -15,6 +15,7 @@ import re import math import zlib import shutil +import string import hashlib import pathlib import urllib @@ -72,6 +73,20 @@ def expand(path : Union[str, pathlib.Path]) -> str: return op.abspath(expandvars(op.expanduser(path))) +def sanitiseFileName(filename : str) -> str: + """Generate a "sanitised" version of the given file name, with all silly + characters replaced with underscores. + """ + + newname = '' + allowed = string.ascii_letters + string.digits + '-_.' + for char in filename: + if char in allowed: newname += char + else: newname += '_' + + return newname + + class DownloadFailed(Exception): """Exception type raised by the :func:`downloadFile` function if a download fails for some reason. diff --git a/fsl/add_module/tests/test_fsl_add_module.py b/fsl/add_module/tests/test_fsl_add_module.py index 1fff141781a948ca3f6c03f085509c5d862b217a..66a45b3d8d2404afbe8ac0c3fc9c18709e80d4de 100644 --- a/fsl/add_module/tests/test_fsl_add_module.py +++ b/fsl/add_module/tests/test_fsl_add_module.py @@ -17,11 +17,6 @@ from . import make_archive, tempdir, server, mock_input, check_dir def test_parseArgs(): - # force can only be used when plugins are specified - fam.parseArgs('--force a.zip b.zip'.split()) - with pytest.raises(SystemExit): - fam.parseArgs(['--force']) - # either one destination is specified, or # <nplugins> destinationsx are specified fam.parseArgs('-d dest'.split()) @@ -85,7 +80,7 @@ def test_loadManifest_different_sources(): assert list(m.keys()) == ['plugin.zip'] # default manifest - with mock.patch('fsl.scripts.fsl_add_module.MANIFEST_URL', url): + with mock.patch('fsl.scripts.fsl_add_module.DEFAULT_MANIFEST_URL', url): m, p = fam.loadManifest(fam.parseArgs([])) assert len(p) == 0 assert list(m.keys()) == ['abc', 'def'] @@ -163,7 +158,7 @@ def test_loadManifest_destination_specified(): def test_selectPlugins_from_filepath_and_url(): - with mock.patch('fsl.scripts.fsl_add_module.MANIFEST_URL', None): + with mock.patch('fsl.scripts.fsl_add_module.DEFAULT_MANIFEST_URL', None): args = fam.parseArgs('abc.zip def.zip -d dest'.split()) m, p = fam.loadManifest(args) plugins = fam.selectPlugins(args, m, p) @@ -193,7 +188,7 @@ def test_selectPlugins_name_from_manifest(): def test_selectPlugins_no_destination_specified(): - with mock.patch('fsl.scripts.fsl_add_module.MANIFEST_URL', None): + with mock.patch('fsl.scripts.fsl_add_module.DEFAULT_MANIFEST_URL', None): args = fam.parseArgs('abc.zip def.zip'.split()) m, p = fam.loadManifest(args) @@ -394,8 +389,9 @@ def test_main_noargs(): # patch os.environ in case FSLDIR is set fammod = 'fsl.scripts.fsl_add_module' - with mock.patch(f'{fammod}.MANIFEST_URL', f'{cwd}/manifest.json'), \ - mock.patch(f'{fammod}.ARCHIVE_DIR', f'{cwd}/archives'), \ + with mock.patch(f'{fammod}.DEFAULT_MANIFEST_URL', f'{cwd}/manifest.json'), \ + mock.patch(f'{fammod}.DEFAULT_CATEGORY', None), \ + mock.patch(f'{fammod}.ARCHIVE_DIR', f'{cwd}/archives'), \ mock.patch.dict(os.environ, clear=True): # user will be asked what plugins they want, @@ -418,7 +414,7 @@ def test_main_list(): with open('manifest.json', 'wt') as f: f.write(json.dumps(manifest)) - with mock.patch('fsl.scripts.fsl_add_module.MANIFEST_URL', + with mock.patch('fsl.scripts.fsl_add_module.DEFAULT_MANIFEST_URL', f'{cwd}/manifest.json'): fam.main(['-l']) fam.main('-l -v'.split()) @@ -468,11 +464,23 @@ def test_main_manifest_force(): f.write(json.dumps(manifest)) with mock.patch.dict(os.environ, clear=True): + + # give specific modules to download fam.main('-m manifest.json -a archives -f def'.split()) check_dir(cwd, ['archives/def.zip', 'e/f', 'g/h'], ['archives/abc.zip', 'a/b', 'c/d']) + shutil.rmtree('archives') + shutil.rmtree('e') + shutil.rmtree('g') + + # no modules specified - download all. + # note that abc.zip is from cwd, so + # is not copied into archive dir + fam.main('-m manifest.json -a archives -f'.split()) + check_dir(cwd, ['archives/def.zip', 'e/f', 'g/h', 'a/b', 'c/d']) + def test_main_category(): @@ -503,8 +511,8 @@ def test_main_plugin_paths(): args = f'./abc.zip http://localhost:{srv.port}/def.zip'.split() fammod = 'fsl.scripts.fsl_add_module' - with mock.patch(f'{fammod}.MANIFEST_URL', None), \ - mock.patch(f'{fammod}.ARCHIVE_DIR', f'{cwd}/archives'), \ + with mock.patch(f'{fammod}.DEFAULT_MANIFEST_URL', None), \ + mock.patch(f'{fammod}.ARCHIVE_DIR', f'{cwd}/archives'), \ mock.patch.dict(os.environ, clear=True): with mock_input(''): @@ -607,7 +615,7 @@ def test_main_skip_already_downloaded(): # direct download - if file exists, it is # assumed to be ok, and not re-downloaded. - with mock.patch('fsl.scripts.fsl_add_module.MANIFEST_URL', None): + with mock.patch('fsl.scripts.fsl_add_module.DEFAULT_MANIFEST_URL', None): fam.main(f'{url["abc"]} -a {archiveDir} -d {destDir} -f'.split()) assert os.stat(archivePath['abc']).st_mtime_ns == mtime['abc'] check_dir(destDir, ['a/b', 'c/d']) @@ -644,7 +652,7 @@ def test_main_customise_plugin_dir(): os.mkdir(defdest) os.mkdir(ghidest) - with mock.patch('fsl.scripts.fsl_add_module.MANIFEST_URL', None), \ + with mock.patch('fsl.scripts.fsl_add_module.DEFAULT_MANIFEST_URL', None), \ mock_input('c', abcdest, defdest, ghidest): fam.main(f'abc.zip def.zip ghi.zip'.split()) @@ -703,3 +711,76 @@ def test_main_abort_bad_destination(): with mock.patch.dict(os.environ, clear=True): fam.main('-m manifest.json -f abc'.split()) assert e.value.code != 0 + + +# In normal usage (when downloading the default +# manifest), fsl_add_module will only display +# modules in the "fsl_course_data" category. +# This can be overridden by passing +# --category <some-category> or +# --category all +def test_default_categories(): + with tempdir() as cwd, server() as srv: + manifest = [ + {'name' : 'a', + 'category' : 'fsl_course_data', + 'url' : f'http://localhost:{srv.port}/a.zip'}, + {'name' : 'b', + 'url' : f'http://localhost:{srv.port}/b.zip'}, + {'name' : 'c', + 'category' : 'patches', + 'url' : f'http://localhost:{srv.port}/c.zip'}, + {'name' : 'd', + 'category' : 'fsl_course_data', + 'url' : f'http://localhost:{srv.port}/d.zip'}, + ] + with open('manifest.json', 'wt') as f: + f.write(json.dumps(manifest)) + + make_archive('a.zip', 'a/a.txt') + make_archive('b.zip', 'b/b.txt') + make_archive('c.zip', 'c/c.txt') + make_archive('d.zip', 'd/d.txt') + + # default -> only fsl_course_data downloaded + with mock.patch('fsl.scripts.fsl_add_module.DEFAULT_MANIFEST_URL', + f'{cwd}/manifest.json'): + fam.main('-f -a archives -d .'.split()) + check_dir('.', + ['a/a.txt', 'd/d.txt', + 'archives/fsl_course_data_a.zip', + 'archives/fsl_course_data_d.zip'], + ['b/b.txt', 'c/c.txt', + 'archives/b.zip', 'archives/patches_c.zip']) + + shutil.rmtree('archives') + shutil.rmtree('a') + shutil.rmtree('d') + + # --category all -> all modules downloaded + with mock.patch('fsl.scripts.fsl_add_module.DEFAULT_MANIFEST_URL', + f'{cwd}/manifest.json'): + fam.main('-f -a archives -d . -c all'.split()) + check_dir('.', + ['a/a.txt', 'd/d.txt', 'b/b.txt', 'c/c.txt', + 'archives/fsl_course_data_a.zip', + 'archives/fsl_course_data_d.zip', + 'archives/b.zip', + 'archives/patches_c.zip']) + + shutil.rmtree('archives') + shutil.rmtree('a') + shutil.rmtree('b') + shutil.rmtree('c') + shutil.rmtree('d') + + # --category <something> -> <something> modules downloaded + with mock.patch('fsl.scripts.fsl_add_module.DEFAULT_MANIFEST_URL', + f'{cwd}/manifest.json'): + fam.main('-f -a archives -d . -c patches'.split()) + check_dir('.', + ['c/c.txt', 'archives/patches_c.zip'], + ['a/a.txt', 'b/b.txt', 'd/d.txt', + 'archives/fsl_course_data_a.zip', + 'archives/b.zip', + 'archives/fsl_course_data_d.zip']) diff --git a/fsl/add_module/tests/test_plugin_manifest.py b/fsl/add_module/tests/test_plugin_manifest.py index a81732691122ccb3bcaf4134809c1c24ca949126..d04c96b4114fff6843433e9bd0bf0e003843a2d9 100644 --- a/fsl/add_module/tests/test_plugin_manifest.py +++ b/fsl/add_module/tests/test_plugin_manifest.py @@ -52,14 +52,17 @@ def test_downloadManifest(): assert manifest.categories == sorted(['plgcat', plgman.UNCATEGORISED]) assert manifest['123'] .name == '123' assert manifest['123'] .url == 'http://abc.com/123' + assert manifest['123'] .fileName == '123' assert manifest['plugin456'].name == 'plugin456' assert manifest['plugin456'].url == 'http://abc.com/456' + assert manifest['plugin456'].fileName == '456' assert manifest['plugin789'].name == 'plugin789' assert manifest['plugin789'].url == 'http://abc.com/789' assert manifest['plugin789'].category == 'plgcat' assert manifest['plugin789'].checksum == '12345' assert manifest['plugin789'].description == 'Great plugin' assert manifest['plugin789'].destination == '/dest' + assert manifest['plugin789'].fileName == 'plgcat_789' def test_addPlugin(): diff --git a/fsl/add_module/tests/test_ui.py b/fsl/add_module/tests/test_ui.py index bac479782415546a6112183585b5160b2e24fbc8..da7fdfc4f8134751d6fd15bdc7694585a1b6414a 100644 --- a/fsl/add_module/tests/test_ui.py +++ b/fsl/add_module/tests/test_ui.py @@ -247,25 +247,42 @@ def test_downloadPlugin(): destdir = 'destination' make_archive('abc.zip', 'a/b/c.txt', 'd/e/f.txt') + make_archive('ghi.zip', 'g/h/i.txt', 'j/k/l.txt') os.mkdir(destdir) - plugin = plgman.Plugin( + plugin1 = plgman.Plugin( name='abc.zip', url=f'http://localhost:{srv.port}/abc.zip', checksum=routines.calcChecksum('abc.zip')) + plugin2 = plgman.Plugin( + name='ghi.zip', + url=f'http://localhost:{srv.port}/ghi.zip', + category='ghicat', + checksum=routines.calcChecksum('ghi.zip')) + # regular download - ret = ui.downloadPlugin(plugin, destdir) + ret = ui.downloadPlugin(plugin1, destdir) assert op.exists(op.join(destdir, 'abc.zip')) assert routines.calcChecksum(op.join(destdir, 'abc.zip')) == \ - plugin.checksum + plugin1.checksum assert op.abspath(ret[0]) == op.abspath(op.join(destdir, 'abc.zip')) assert not ret[1] + # regular download, checking + # that downloaded file is + # named <category>_<file> + ret = ui.downloadPlugin(plugin2, destdir) + assert op.exists(op.join(destdir, 'ghicat_ghi.zip')) + assert routines.calcChecksum(op.join(destdir, 'ghicat_ghi.zip')) == \ + plugin2.checksum + assert op.abspath(ret[0]) == op.abspath(op.join(destdir, 'ghicat_ghi.zip')) + assert not ret[1] + # file already exists and checksums # match - download should be skipped with mock.patch('fsl.add_module.ui.downloadFile') as mocked: - ret = ui.downloadPlugin(plugin, destdir) + ret = ui.downloadPlugin(plugin1, destdir) mocked.assert_not_called() assert op.abspath(ret[0]) == op.abspath(op.join(destdir, 'abc.zip')) assert ret[1] @@ -274,8 +291,8 @@ def test_downloadPlugin(): # do not match - download should be # repeated with mock.patch('fsl.add_module.ui.downloadFile') as mocked, \ - mock.patch.object(plugin, 'checksum', '1234'): - ret = ui.downloadPlugin(plugin, destdir) + mock.patch.object(plugin1, 'checksum', '1234'): + ret = ui.downloadPlugin(plugin1, destdir) mocked.assert_called() assert op.abspath(ret[0]) == op.abspath(op.join(destdir, 'abc.zip')) assert not ret[1] diff --git a/fsl/add_module/ui.py b/fsl/add_module/ui.py index 3923cfcc01a65a311016407232635c4652097313..d73af279d9654122f13af43fc29d7d176cf236d8 100644 --- a/fsl/add_module/ui.py +++ b/fsl/add_module/ui.py @@ -8,10 +8,9 @@ of the ``fsl_add_module`` program. """ -import os -import os.path as op -import pathlib -import urllib.parse as urlparse +import os +import os.path as op +import pathlib from typing import Tuple, List, Union @@ -355,7 +354,7 @@ def downloadPlugin(plugin : Plugin, destDir, **kwargs) -> Tuple[str, bool]: otherwise. """ - fname = op.basename(urlparse.urlparse(plugin.url).path) + fname = plugin.fileName destFile = op.join(destDir, fname) skipDownload = False diff --git a/fsl/scripts/fsl_add_module.py b/fsl/scripts/fsl_add_module.py index c6d59e0bef2509029fda09801b27705dd73d6cf9..edfb74450207a1c681c422ef658b9876874cffd2 100755 --- a/fsl/scripts/fsl_add_module.py +++ b/fsl/scripts/fsl_add_module.py @@ -7,6 +7,8 @@ """The ``fsl_add_module`` script is used for downloading and installing FSL "modules"/"plugins" - archive files (e.g. ``.tar.gz`` ``.zip``, etc.). +The primary use of this script is for downloading the FSL course data. + Normal execution of this script is as follows: 1. The user invokes the script, specifying the plugin they would like to @@ -50,7 +52,7 @@ Usage examples When called in one of the above forms, ``fsl_add_module`` will: - 1. Download a manifest file from the default :data:`MANIFEST_URL`, or + 1. Download a manifest file from the default :data:`DEFAULT_MANIFEST_URL`, or ``<manifest_url>`` if specified 2. Display a list of all available plugins to the user (restricting to those @@ -112,12 +114,18 @@ from fsl.add_module.messages import (info, UNDERLINE) -MANIFEST_URL = 'http://fsl.fmrib.ox.ac.uk/fslcourse/downloads/manifest.json' +DEFAULT_MANIFEST_URL = 'http://fsl.fmrib.ox.ac.uk/fslcourse/downloads/manifest.json' """Location of the official FSL plugin manifest file, downloaded if an alternate manifest file is not specified. """ +DEFAULT_CATEGORY = 'fsl_course_data' +"""Default value for the ``--category`` command-line option, when the +user does not specify an alternative ``--manifest`` or ``--category``. +""" + + ARCHIVE_DIR = op.expanduser(op.join('~', '.fsl_add_module')) """Location in which downloaded plugin files are cached. """ @@ -136,18 +144,18 @@ def parseArgs(argv : List[str]) -> argparse.Namespace: 'manifest' : 'URL to module manifest file.', 'archiveDir' : 'Directory to cache downloaded files in.', 'category' : 'Only display available modules from the specified ' - 'category. Ignored if modules are explicitly ' - 'specified on the command-line.', + 'category. Defaults to "fsl_course_data", unless ' + 'a custom --manifest is specified. Pass "all" to ' + 'display modules from all categories.', 'destination' : 'Destination directory to install module files. If ' 'used once, applies to all modules. Otherwise, must ' 'be specified for each module that is specified on ' 'the command-line. If not provided, you will be ' 'prompted to select the destination directory for ' 'each module.', - 'force' : 'If used, you will not be asked any questions, and ' - 'all default settings will be applied. Can only be ' - 'used when the modules you wish to have installed ' - 'are specified on the command-line. ' + 'force' : 'If used, you will not be asked any questions, ' + 'all default settings will be applied, and all ' + 'available modules will be downloaded.' } parser = argparse.ArgumentParser( @@ -162,10 +170,8 @@ def parseArgs(argv : List[str]) -> argparse.Namespace: parser.add_argument( '-l', '--list', action='store_true', dest='listPlugins', help=helps['list']) - parser.add_argument( - '-m', '--manifest', default=MANIFEST_URL, help=helps['manifest']) - parser.add_argument( - '-c', '--category', help=helps['category']) + parser.add_argument('-m', '--manifest', help=helps['manifest']) + parser.add_argument('-c', '--category', help=helps['category']) parser.add_argument( '-a', '--archiveDir', default=ARCHIVE_DIR, help=helps['archiveDir']) parser.add_argument( @@ -176,18 +182,25 @@ def parseArgs(argv : List[str]) -> argparse.Namespace: args = parser.parse_args(argv) + # If neither a custom manifest nor category are + # provided, we default to only showing the FSL + # course data. If we are using a custom manifest, + # we don't want to set a default category. + if args.manifest is None: + args.manifest = DEFAULT_MANIFEST_URL + if args.manifest == DEFAULT_MANIFEST_URL and args.category is None: + args.category = DEFAULT_CATEGORY + if args.category == 'all': + args.category = None + if args.archiveDir: - args.archiveDir = op.abspath(args.archiveDir) + args.archiveDir = op.abspath(op.expanduser(args.archiveDir)) if args.destination is not None: if len(args.destination) not in (1, len(args.module)): parser.error('The --destination option must either be specified ' 'exactly once, or once for every requested module.') - if args.force and len(args.module) == 0: - parser.error('The --force option may only be used when you specify ' - 'which module to install on the command line.') - # make all destination paths absolute, # and support tilde/envvar expansion if args.destination is not None: @@ -270,7 +283,8 @@ def selectPlugins(args : argparse.Namespace, # plugins, and ask them which ones they # want to install if len(plugins) == 0: - plugins = ui.selectPlugins(manifest, args.category) + if args.force: plugins = manifest.getCategory(args.category) + else: plugins = ui.selectPlugins(manifest, args.category) else: plugins = [manifest[p] for p in plugins] info('Installing the following modules:', EMPHASIS)