#114 Define compat shims for %patchN when building for EL <= 10
Merged 8 days ago by tdawson. Opened 9 days ago by churchyard.
centos/ churchyard/centpkg c8s-rpm4.20  into  develop

file modified
+52
@@ -16,10 +16,12 @@ 

  # the full text of the license.

  

  

+ import os

  import re

  import warnings

  

  import git

+ import rpm

  from pyrpkg import Commands, rpkgError

  from pyrpkg.utils import cached_property

  
@@ -201,6 +203,55 @@ 

                      self.branch_merge,

                  )

  

+     def _define_patchn_compatiblity_macros(self):

+         """

+         RPM 4.19 deprecated the %patchN macro. RPM 4.20 removed it completely.

+         The macro works on c8s, c9s, c10s, but does not work on Fedora 41+.

+         We can no longer even parse RPM spec files with the %patchN macros.

+         When we build for old streams, we define the %patchN macros manually as %patch -P N.

+         Since N can be any number including zero-prefixed numbers,

+         we regex-search the spec file for %patchN uses and define only the macros found.

+         """

+         # Only do this on RPM 4.19.90+ (4.19.9x were pre-releases of 4.20)

+         if tuple(int(i) for i in rpm.__version_info__) < (4, 19, 90):

+             return

+         # Only do this when building for CentOS Stream version with RPM < 4.20

+         try:

+             if int(self._distval.split("_")[0]) > 10:

+                 return

+         except ValueError as e:

+             self.log.debug(

+                 "Cannot parse major dist version as int: %s",

+                 self._distval.split("_")[0],

+                 exc_info=e,

+             )

+             return

+         defined_patchn = False

+         try:

+             specfile_path = os.path.join(self.layout.specdir, self.spec)

+             with open(specfile_path, "rb") as specfile:

+                 # Find all uses of %patchN in the spec files

+                 # Using a benevolent regex: commented out macros, etc. match as well

+                 for patch in re.findall(rb"%{?patch(\d+)\b", specfile.read()):

+                     # We operate on bytes becasue we don't know the spec encoding

+                     # but the matched part only includes ASCII digits

+                     patch = patch.decode("ascii")

+                     self._rpmdefines.extend(

+                         [

+                             "--define",

+                             # defines parametric macro %patchN which passes all arguments to %patch -P N

+                             "patch%s(-) %%patch -P %s %%{?**}" % (patch, patch),

+                         ]

+                     )

+                     defined_patchn = True

+         except OSError as e:

+             self.log.debug("Cannot read spec.", exc_info=e)

+         if defined_patchn:

+             self.log.warn(

+                 "centpkg defined %patchN compatibility shims to parse the spec file. "

+                 "%patchN is obsolete, use %patch -P N instead."

+             )

+ 

      # redefined loaders

      def load_rpmdefines(self):

          """
@@ -245,6 +296,7 @@ 

              "--eval",

              "%%undefine %s" % self._distunset,

          ]

+         self._define_patchn_compatiblity_macros()

          self.log.debug("RPMDefines: %s" % self._rpmdefines)

  

      def construct_build_url(self, *args, **kwargs):

no initial comment

rebased onto 4273360

9 days ago

Probably needs a guard to only do it if RPM version is >= 4.20

Done

rebased onto 4273360

9 days ago

rebased onto 4273360

9 days ago

rebased onto 4273360

9 days ago

Note on style: I prefer f-strings for this kind of thing, but all the other defines use the % thing, so I went with it for consistency.

rebased onto 4273360

9 days ago

I run this through black, as I see @sgallagh recently started using this here.

However, the surrounding code is not all blacked -- 441ee7a introduced plain quotes.

Would you mind adding some explanatory comments to this? I think from inspection what it's doing is

  • Read through the specfile, checking every line with a regex matching %patch\d+
  • For every match found, it adds a macro definition to self._rpmdefines so that it's "rewritten" into the %patch -P format.

Which looks like a really clever hack, honestly.

I wonder if this should also print a deprecation warning here, though. While it might still be "valid" on RHEL up to RHEL 10, we should strongly encourage migrating to the future-supported %patch -P format. Using centpkg is definitely preferable, but people probably still want to be able to use their own tools that might use RPM directly. If we hack around this in centpkg, those other tools will look like they're broken.

2 new commits added

  • fixup: warn
  • fixup: More comments
9 days ago

added 2 fixups. one with verbose comments, one with a warning. happy to squash if you want them

(at this point, the code is so long it probably deserves a method)

1 new commit added

  • fixup: separate method
9 days ago

at this point, the code is so long it probably deserves a method

done

4 new commits added

  • fixup: separate method
  • fixup: warn
  • fixup: More comments
  • Define compat shims for %patchN when building for EL <= 10
9 days ago

"%patchN is obsolete, use %patch N (or %patch -P N)" -> "%patchN is obsolete, use %patch N (or %patch -P N if this specfile needs to support RPM < 4.18)"

Switching to the %patch N syntax will break on RHEL 8 and 9. Maybe we should just advise use of %patch -P N universally?

1 new commit added

  • fixup: warning message
9 days ago

"%patchN is obsolete, use %patch N (or %patch -P N)" was copied from the RPM error message. It can be different I guess, committed.

Thanks. With that, I'm good with this. Thanks for putting it together.

Please go ahead and squash the patches together. I talked to @tdawson and he's going to do a final review (probably tomorrow morning) and then we'll get it merged and built.

rebased onto 4273360

9 days ago

rebased onto 4273360

9 days ago

Please go ahead and squash the patches together.

Done.

This LGTM, but I haven't tested it.
Give me an hour or so to test and verify.
If everything is looking good, I'll merge and get it out as soon as possible.

Well, that didn't take much time to find and test.
Looks very good to me.
Thank you for getting this done.

Pull-Request has been merged by tdawson

8 days ago

For the record, I tested this on python3, branch c8s, running centpkg on Fedora 41.

I have not tested if this is properly skipped on c11s, if it is skipped on Fedora 40, etc...

It tested it on RHEL9, and a couple Fedoras. We'll have to test it on c11s when we have one :).
But for now ... let's push it out so people are not blocked.
We might have to add a patch or two when people find edge cases.

Metadata