#287 Add revert subcommand
Closed a year ago by gotmax23. Opened a year ago by gotmax23.
fedora-infra/ gotmax23/rpmautospec revert  into  main

file modified
+2 -2
@@ -5,10 +5,10 @@ 

  import sys

  import typing

  

- from .subcommands import changelog, convert, process_distgit, release

+ from .subcommands import changelog, convert, process_distgit, release, revert

  

  

- all_subcmds = (changelog, convert, process_distgit, release)

+ all_subcmds = (changelog, convert, process_distgit, release, revert)

  subcmd_modules_by_name = {}

  

  

@@ -0,0 +1,84 @@ 

+ # Copyright (c) 2023 Maxwell G <gotmax@e.email>

+ # SPDX-License-Identifier: MIT

+ 

+ import argparse

+ import fileinput

+ import logging

+ import re

+ import sys

+ import typing as t

+ from contextlib import suppress

+ from pathlib import Path

+ 

+ from ..misc import autochangelog_re as AUTOCHANGELOG_RE

+ from ..misc import check_specfile_features

+ from ..pkg_history import PkgHistoryProcessor

+ from .changelog import collate_changelog

+ 

+ log = logging.getLogger(__name__)

+ 

+ RELEASE_RE: t.Pattern[str] = re.compile(r"^((?i:Release\s*:\s*))(?:%\{?autorelease).*$")

This regular expression is far too naive. The %autorelease macro may not be present directly in this field. See https://src.fedoraproject.org/rpms/nodejs18/blob/rawhide/f/nodejs18.spec#_32 for an example.

+ 

+ 

+ def register_subcommand(subparsers: argparse._SubParsersAction) -> str:

+     subcmd_name = "revert"

+     revert_parser = subparsers.add_parser(

+         subcmd_name,

+         help="Convert an rpmautospec enabled specfile to use a static Release: and %%changelog",

%%changelog

I don't think it makes sense to use %% in a message. Just % is fine.

+     )

+     revert_parser.add_argument(

+         "spec_or_path",

+         default=Path("."),

+         type=Path,

+         nargs="?",

+         help="Path to package worktree or the spec file within",

+     )

+     revert_parser.add_argument(

+         "--no-backup",

+         action="store_false",

+         dest="backup",

+         help="Don't backup the pre-conversion specfile. Delete 'changelog' file if it exists",

+     )

+     return subcmd_name

+ 

+ 

+ def replace(

+     file: Path,

+     release: t.Optional[str],

+     changelog: t.Optional[str],

+ ) -> Path:

+     with fileinput.input(file, inplace=True, backup=".rpmautospec") as fi:

+         for index, line in enumerate(fi):

+             if release:

+                 match = RELEASE_RE.match(line)

+                 if match:

+                     log.info("%s:%s: replacing Release", file, index + 1)

+                     line = match.group(1) + release + "\n"

+             if changelog:

+                 if AUTOCHANGELOG_RE.match(line):

+                     log.info("%s:%s: replacing %%autochangelog", file, index + 1)

+                     sys.stdout.write(changelog)

+                     break

+             sys.stdout.write(line)

+     return Path(str(file) + ".rpmautospec")

+ 

+ 

+ def main(params: argparse.Namespace) -> None:

+     processor = PkgHistoryProcessor(params.spec_or_path)

+     features = check_specfile_features(processor.specfile)

+     if not features.has_autorelease and not features.has_autochangelog:

+         log.info(f"{str(processor.specfile)!r} does not use rpmautospec. Nothing to do.")

+         return

+     visitors = []

+     if features.has_autorelease:

+         visitors.append(processor.release_number_visitor)

+     if features.has_autochangelog:

+         visitors.append(processor.changelog_visitor)

+     result = processor.run(visitors=visitors)

+     release = result["release-complete"] + "%{?dist}" if features.has_autorelease else None

+     changelog = collate_changelog(result) if features.has_autochangelog else None

+     bakfile = replace(processor.specfile, release, changelog)

+     with suppress(OSError):

+         if not params.backup:

+             processor.path.joinpath("changelog").unlink()

+             bakfile.unlink()

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

This regular expression is far too naive. The %autorelease macro may not be present directly in this field. See https://src.fedoraproject.org/rpms/nodejs18/blob/rawhide/f/nodejs18.spec#_32 for an example.

Before I had RELEASE_RE: t.Pattern[str] = re.compile(r"^(Release\s*:\s*).*$", re.IGNORECASE) but I thought that was too naive. I've seen specfiles with conditional Release lines (e.g. https://src.fedoraproject.org/rpms/systemd/blob/8ed6e37eb47f5ee1ec084227fb1acd9c54a8054e/f/systemd.spec#_32) which this could clobber. I could go back to that if you'd like. The code already checks that the specfile uses %autorelease.

Hey Max, thanks for you contribution!

As Stephen said, this regex alone isn’t going to catch all instances of the %autorelease macro being used – it could e.g. be used in another macro which is in turn used in the release field.

I suggest that you use misc.check_specfile_features() to determine if a spec file uses rpmautospec and misc.autorelease_re to check which lines contain the macro. I don’t have an idea though what to do about arbitrarily placed instances of it, though, it could even be used in multiple places! I'm at a loss for ideas how to automatically revert this – maybe just notify the user without changing anything in that case.

Anyway, whatever the approach you take, would you please add tests for your new code? There's a repo pytest fixture which you can use to generate a temporary repository containing a spec file with two commits in it. The tests should change the spec file for the various corner cases, run revert on the results and check the contents of the spec file/repo afterwards.

%%changelog

I don't think it makes sense to use %% in a message. Just % is fine.

I gave this a spin, and it seems to work as advertised.
I think we don't need to cover every possible case — e.g. if people use some macro magic, it'd be hard to undo programatically. But such packages are very uncommon, and I think it's fine to not process them.

If we are doing the revert, it is useful to do this without commiting. But I think we should do a commit by default, similarly to rpmautospec convert.

rebased onto 7a10fc28caf5a98168a06f35e90bf67b76d8efab

a year ago

%%changelog

I don't think it makes sense to use %% in a message. Just % is fine.

That's needed to escape argparse's formatting.

TypeError: %c requires int or char

rebased onto 88bb442

a year ago

Build succeeded.

I think we don't need to cover every possible case — e.g. if people use some macro magic, it'd be hard to undo programatically. But such packages are very uncommon, and I think it's fine to not process them.

I agree. I think the current implementation strikes a good balance. Replacing every single instance of %autorelease seems to error prone to me. nodejs's usage is quite uncommon anyways. If anyone more familiar with rpmautospec's internals has concrete suggestion of how to better implement this, I'm all ears.

When I find time later this week, I'll write some tests. Also, I'll have the command print an error and exit 1 if check_specfile_features determines that a specfile has %autorelease but replace() finds no match and the same for %autochangelog.

I think we don't need to cover every possible case — e.g. if people use some macro magic, it'd be hard to undo programatically. But such packages are very uncommon, and I think it's fine to not process them.

I agree. I think the current implementation strikes a good balance. Replacing every single instance of %autorelease seems to error prone to me. nodejs's usage is quite uncommon anyways. If anyone more familiar with rpmautospec's internals has concrete suggestion of how to better implement this, I'm all ears.

I think it should refuse to revert spec files that are "too complicated" in this sense. E.g. you could beef up misc.SpecfileFeatures and misc.check_specfile_features() so the latter can distinguish these cases.

FTR, this is still on my radar, and I'll come back to it soon.

I don't really have the motivation to work on this anymore, and I'm trying to finish up another project. If someone wants to take this over and Co-authored-by me, I'd be fine with that.

I am interested in this, because I want to use this feature to fix rpkg#641.
I will take a look at the current status and see if I can finalize things.

Work continues at #292, please close this pull request.

Pull-Request has been closed by gotmax23

a year ago