From 7ccb9992a626b6292bb1b8d9a4dee42004dc74c9 Mon Sep 17 00:00:00 2001 From: "Adam J. Stewart" Date: Tue, 9 Feb 2021 12:51:18 -0600 Subject: Procedure to deprecate old versions of software (#20767) * Procedure to deprecate old versions of software * Add documentation * Fix bug in logic * Update tab completion * Deprecate legacy packages * Deprecate old mxnet as well * More explicit docs --- lib/spack/docs/packaging_guide.rst | 56 +++++++++++++++++++++++++++++++++ lib/spack/spack/cmd/common/arguments.py | 7 +++++ lib/spack/spack/cmd/dev_build.py | 5 ++- lib/spack/spack/cmd/fetch.py | 5 ++- lib/spack/spack/cmd/info.py | 9 +++--- lib/spack/spack/cmd/install.py | 5 ++- lib/spack/spack/cmd/mirror.py | 5 ++- lib/spack/spack/cmd/patch.py | 6 +++- lib/spack/spack/cmd/stage.py | 6 +++- lib/spack/spack/directives.py | 3 ++ lib/spack/spack/package.py | 27 +++++++++++++++- lib/spack/spack/schema/config.py | 1 + 12 files changed, 124 insertions(+), 11 deletions(-) (limited to 'lib') diff --git a/lib/spack/docs/packaging_guide.rst b/lib/spack/docs/packaging_guide.rst index c989a48f92..321d636b8f 100644 --- a/lib/spack/docs/packaging_guide.rst +++ b/lib/spack/docs/packaging_guide.rst @@ -612,6 +612,62 @@ it executable, then runs it with some arguments. installer = Executable(self.stage.archive_file) installer('--prefix=%s' % prefix, 'arg1', 'arg2', 'etc.') + +^^^^^^^^^^^^^^^^^^^^^^^^ +Deprecating old versions +^^^^^^^^^^^^^^^^^^^^^^^^ + +There are many reasons to remove old versions of software: + +#. Security vulnerabilities (most serious reason) +#. Changing build systems that increase package complexity +#. Changing dependencies/patches/resources/flags that increase package complexity +#. Maintainer/developer inability/unwillingness to support old versions +#. No longer available for download (right to be forgotten) +#. Package or version rename + +At the same time, there are many reasons to keep old versions of software: + +#. Reproducibility +#. Requirements for older packages (e.g. some packages still rely on Qt 3) + +In general, you should not remove old versions from a ``package.py``. Instead, +you should first deprecate them using the following syntax: + +.. code-block:: python + + version('1.2.3', sha256='...', deprecated=True) + + +This has two effects. First, ``spack info`` will no longer advertise that +version. Second, commands like ``spack install`` that fetch the package will +require user approval: + +.. code-block:: console + + $ spack install openssl@1.0.1e + ==> Warning: openssl@1.0.1e is deprecated and may be removed in a future Spack release. + ==> Fetch anyway? [y/N] + + +If you use ``spack install --deprecated``, this check can be skipped. + +This also applies to package recipes that are renamed or removed. You should +first deprecate all versions before removing a package. If you need to rename +it, you can deprecate the old package and create a new package at the same +time. + +Version deprecations should always last at least one Spack minor release cycle +before the version is completely removed. For example, if a version is +deprecated in Spack 0.16.0, it should not be removed until Spack 0.17.0. No +version should be removed without such a deprecation process. This gives users +a chance to complain about the deprecation in case the old version is needed +for some application. If you require a deprecated version of a package, simply +submit a PR to remove ``deprecated=True`` from the package. However, you may be +asked to help maintain this version of the package if the current maintainers +are unwilling to support this older version. + + ^^^^^^^^^^^^^^^^ Download caching ^^^^^^^^^^^^^^^^ diff --git a/lib/spack/spack/cmd/common/arguments.py b/lib/spack/spack/cmd/common/arguments.py index f83708a7dd..e6a0edd599 100644 --- a/lib/spack/spack/cmd/common/arguments.py +++ b/lib/spack/spack/cmd/common/arguments.py @@ -277,6 +277,13 @@ def no_checksum(): help="do not use checksums to verify downloaded files (unsafe)") +@arg +def deprecated(): + return Args( + '--deprecated', action='store_true', default=False, + help='fetch deprecated versions without warning') + + def add_cdash_args(subparser, add_help): cdash_help = {} if add_help: diff --git a/lib/spack/spack/cmd/dev_build.py b/lib/spack/spack/cmd/dev_build.py index a681e2c2d7..2e7c51da42 100644 --- a/lib/spack/spack/cmd/dev_build.py +++ b/lib/spack/spack/cmd/dev_build.py @@ -26,7 +26,7 @@ def setup_parser(subparser): subparser.add_argument( '-i', '--ignore-dependencies', action='store_true', dest='ignore_deps', help="don't try to install dependencies of requested packages") - arguments.add_common_arguments(subparser, ['no_checksum']) + arguments.add_common_arguments(subparser, ['no_checksum', 'deprecated']) subparser.add_argument( '--keep-prefix', action='store_true', help="do not remove the install prefix if installation fails") @@ -98,6 +98,9 @@ def dev_build(self, args): if args.no_checksum: spack.config.set('config:checksum', False, scope='command_line') + if args.deprecated: + spack.config.set('config:deprecated', True, scope='command_line') + tests = False if args.test == 'all': tests = True diff --git a/lib/spack/spack/cmd/fetch.py b/lib/spack/spack/cmd/fetch.py index 4f3e0ad594..962bcefce6 100644 --- a/lib/spack/spack/cmd/fetch.py +++ b/lib/spack/spack/cmd/fetch.py @@ -16,7 +16,7 @@ level = "long" def setup_parser(subparser): - arguments.add_common_arguments(subparser, ['no_checksum']) + arguments.add_common_arguments(subparser, ['no_checksum', 'deprecated']) subparser.add_argument( '-m', '--missing', action='store_true', help="fetch only missing (not yet installed) dependencies") @@ -33,6 +33,9 @@ def fetch(parser, args): if args.no_checksum: spack.config.set('config:checksum', False, scope='command_line') + if args.deprecated: + spack.config.set('config:deprecated', True, scope='command_line') + specs = spack.cmd.parse_specs(args.specs, concretize=True) for spec in specs: if args.missing or args.dependencies: diff --git a/lib/spack/spack/cmd/info.py b/lib/spack/spack/cmd/info.py index 4762150c77..6b316f3cc4 100644 --- a/lib/spack/spack/cmd/info.py +++ b/lib/spack/spack/cmd/info.py @@ -189,10 +189,11 @@ def print_text_info(pkg): color.cprint(section_title('Safe versions: ')) for v in reversed(sorted(pkg.versions)): - if pkg.has_code: - url = fs.for_package_version(pkg, v) - line = version(' {0}'.format(pad(v))) + color.cescape(url) - color.cprint(line) + if not pkg.versions[v].get('deprecated', False): + if pkg.has_code: + url = fs.for_package_version(pkg, v) + line = version(' {0}'.format(pad(v))) + color.cescape(url) + color.cprint(line) color.cprint('') color.cprint(section_title('Variants:')) diff --git a/lib/spack/spack/cmd/install.py b/lib/spack/spack/cmd/install.py index eb7dc93f98..5e572f82f6 100644 --- a/lib/spack/spack/cmd/install.py +++ b/lib/spack/spack/cmd/install.py @@ -126,7 +126,7 @@ remote spec matches that of the local spec""") subparser.add_argument( '--source', action='store_true', dest='install_source', help="install source files in prefix") - arguments.add_common_arguments(subparser, ['no_checksum']) + arguments.add_common_arguments(subparser, ['no_checksum', 'deprecated']) subparser.add_argument( '-v', '--verbose', action='store_true', help="display verbose build output while installing") @@ -288,6 +288,9 @@ environment variables: if args.no_checksum: spack.config.set('config:checksum', False, scope='command_line') + if args.deprecated: + spack.config.set('config:deprecated', True, scope='command_line') + # Parse cli arguments and construct a dictionary # that will be passed to the package installer update_kwargs_from_args(args, kwargs) diff --git a/lib/spack/spack/cmd/mirror.py b/lib/spack/spack/cmd/mirror.py index 4552cce400..2f0611a2c4 100644 --- a/lib/spack/spack/cmd/mirror.py +++ b/lib/spack/spack/cmd/mirror.py @@ -28,7 +28,7 @@ level = "long" def setup_parser(subparser): - arguments.add_common_arguments(subparser, ['no_checksum']) + arguments.add_common_arguments(subparser, ['no_checksum', 'deprecated']) sp = subparser.add_subparsers( metavar='SUBCOMMAND', dest='mirror_command') @@ -371,4 +371,7 @@ def mirror(parser, args): if args.no_checksum: spack.config.set('config:checksum', False, scope='command_line') + if args.deprecated: + spack.config.set('config:deprecated', True, scope='command_line') + action[args.mirror_command](args) diff --git a/lib/spack/spack/cmd/patch.py b/lib/spack/spack/cmd/patch.py index e85541a535..14739b8d7d 100644 --- a/lib/spack/spack/cmd/patch.py +++ b/lib/spack/spack/cmd/patch.py @@ -16,7 +16,8 @@ level = "long" def setup_parser(subparser): - arguments.add_common_arguments(subparser, ['no_checksum', 'specs']) + arguments.add_common_arguments( + subparser, ['no_checksum', 'deprecated', 'specs']) def patch(parser, args): @@ -26,6 +27,9 @@ def patch(parser, args): if args.no_checksum: spack.config.set('config:checksum', False, scope='command_line') + if args.deprecated: + spack.config.set('config:deprecated', True, scope='command_line') + specs = spack.cmd.parse_specs(args.specs, concretize=True) for spec in specs: package = spack.repo.get(spec) diff --git a/lib/spack/spack/cmd/stage.py b/lib/spack/spack/cmd/stage.py index d6832b2f09..62073f10bd 100644 --- a/lib/spack/spack/cmd/stage.py +++ b/lib/spack/spack/cmd/stage.py @@ -16,7 +16,8 @@ level = "long" def setup_parser(subparser): - arguments.add_common_arguments(subparser, ['no_checksum', 'specs']) + arguments.add_common_arguments( + subparser, ['no_checksum', 'deprecated', 'specs']) subparser.add_argument( '-p', '--path', dest='path', help="path to stage package, does not add to spack tree") @@ -37,6 +38,9 @@ def stage(parser, args): if args.no_checksum: spack.config.set('config:checksum', False, scope='command_line') + if args.deprecated: + spack.config.set('config:deprecated', True, scope='command_line') + specs = spack.cmd.parse_specs(args.specs, concretize=True) for spec in specs: package = spack.repo.get(spec) diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py index 360448b851..30a45f1b06 100644 --- a/lib/spack/spack/directives.py +++ b/lib/spack/spack/directives.py @@ -278,6 +278,9 @@ def version(ver, checksum=None, **kwargs): The ``dict`` of arguments is turned into a valid fetch strategy for code packages later. See ``spack.fetch_strategy.for_package_version()``. + + Keyword Arguments: + deprecated (bool): whether or not this version is deprecated """ def _execute_version(pkg): if checksum is not None: diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py index 3e19264e24..0c09410c6d 100644 --- a/lib/spack/spack/package.py +++ b/lib/spack/spack/package.py @@ -1309,7 +1309,6 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)): tty.debug('No fetch required for {0}: package has no code.' .format(self.name)) - start_time = time.time() checksum = spack.config.get('config:checksum') fetch = self.stage.managed_by_spack if checksum and fetch and self.version not in self.versions: @@ -1331,8 +1330,34 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)): raise FetchError("Will not fetch %s" % self.spec.format('{name}{@version}'), ck_msg) + deprecated = spack.config.get('config:deprecated') + if not deprecated and self.versions.get( + self.version, {}).get('deprecated', False): + tty.warn("{0} is deprecated and may be removed in a future Spack " + "release.".format( + self.spec.format('{name}{@version}'))) + + # Ask the user whether to install deprecated version if we're + # interactive, but just fail if non-interactive. + dp_msg = ("If you are willing to be a maintainer for this version " + "of the package, submit a PR to remove `deprecated=False" + "`, or use `--deprecated` to skip this check.") + ignore_deprecation = False + if sys.stdout.isatty(): + ignore_deprecation = tty.get_yes_or_no(" Fetch anyway?", + default=False) + + if ignore_deprecation: + tty.debug("Fetching deprecated version. {0}".format( + dp_msg)) + + if not ignore_deprecation: + raise FetchError("Will not fetch {0}".format( + self.spec.format('{name}{@version}')), dp_msg) + self.stage.create() err_msg = None if not self.manual_download else self.download_instr + start_time = time.time() self.stage.fetch(mirror_only, err_msg=err_msg) self._fetch_time = time.time() - start_time diff --git a/lib/spack/spack/schema/config.py b/lib/spack/spack/schema/config.py index 3a97d79675..f0669600e1 100644 --- a/lib/spack/spack/schema/config.py +++ b/lib/spack/spack/schema/config.py @@ -78,6 +78,7 @@ properties = { 'install_missing_compilers': {'type': 'boolean'}, 'debug': {'type': 'boolean'}, 'checksum': {'type': 'boolean'}, + 'deprecated': {'type': 'boolean'}, 'locks': {'type': 'boolean'}, 'dirty': {'type': 'boolean'}, 'build_language': {'type': 'string'}, -- cgit v1.2.3-70-g09d2