#300 RPM: Guideline for patches on Source?
Closed: Fixed None Opened 7 years ago by dougsland.

Hi,

I have been reviewing bsd-mailx package (bz#971103) and the reporter have used this style for applying some patches:

Source1: http://ftp.de.debian.org/debian/pool/main/b/%{name}/%{name}_%{version}-%{cvs_tag}-1.debian.tar.gz

apply debian patches

for i in debian/patches/*.patch; do
%{__patch} -p1 < $i
done

IMHO, spec should be always simple and clear using the correct macros.
My comment: https://bugzilla.redhat.com/show_bug.cgi?id=971103#c8

Please let me know if we have any guideline that I am missing.

Thanks
Douglas


If you have a question or want advice, just ask onlist,
https://lists.fedoraproject.org/mailman/listinfo/packaging

I'll add my opinion in the review.

Discussed at today's FPC meeting. A few of us remembered some previous discussion but no one could remember more or find urls. There seem to be several axes about which we were stuck:

  • Do patches belong in git?
  • Pro: git allows visualizing changes to textual files including patches.
  • Con: truly huge patches (the single kernel patch was given as an example) aren't better in git because the single hug file contains many unrelated changes with multiple sources.
  • If we can consider this as upstream's method of shipping their code, does that mean we'd then be comfortable sticking it in lookaside cache?
  • There is precedent to use git when packagers use a tarball release and then apply patches to bring it up to a current snapshot of upstream.

Side questions:
what do we consider the upstream for this specific package?
* One person was for OpenBSD being upstream but others seemed more flexible
How do we feel about blindly applying the patches?
* Opinion was split over whether maintainers should be reviewing any patch they apply vs accepting Debian as the upstream (and thus, their patches are the basis in code for what we're packaging).

I'll write up a draft based on these which probably can't address all of these concerns but hopefully will be as close to a compromise as possible.

If anyone has links to the previous discussion about patches and looping, please feel free to mention them as well.

Ah -- here's the guideline for the previous related discussion:

https://fedoraproject.org/wiki/Packaging:RPM_Source_Dir

It was about SourceN, not PatchN but has some similarities to the current discussion. May see if I can dig up the discussion related to that Guideline change before putting together a draft.

I've read through https://fedorahosted.org/fpc/ticket/12 and the linked discussion from 2007. After reviewing that past discussion, I no longer think there is precedent for forcing patches to be used only from %patch macros. However, the other quesetions raised during the meeting should still be considered.

Draft:
"""

Normally, patches to a package should be listed in PatchN: tags in the RPM spec file and applied using the %patch macro. The files must then be checked into the Fedora Package revision control system (currently the git repos on pkgs.fedoraproject.org and commonly accessed via fedpkg). Storing the files in this way allows people to use standard tools to visualize the changes between revisions of the files and track additions and removals without a layer of indirection (as putting them into lookaside would do).

Maintainer may deviate from this when the upstream of the package is being shipped as a tarball of patches against a base release. In this case the tarball of patches may be listed as a SourceN: line and the patches would be applied by untarring the archive and then applying each of the patches in turn using the regular /usr/bin/patch command. Additional patches to the package (for instance, generated by the Fedora maintainer to fix bugs) would still be listed in PatchN: lines and be applied by %patch macros '''after''' the patches from the tarball were applied. Maintainers and reviewers should be cautious when exercising this exception as shipping an update as a patchset may be a sign that the patchset is not from the actual upstream or that the patches should be reviewed for correctness rather than simply accepted as the upstream code base.

"""

Feel free to treat this as a strawman... I'm not happy with the vagueness of it.

Also, the specific review that lead to this falls squarely into the vague, grey area. That probably leaves the review in the same practical place as now.... There's certainly no current policy that would say a reviewer must continue a review if they disagree with the packager. It's up to the maintainer to make a case for their view which either persuades that reviewer or wait for a different reviewer who agrees with that viewpoint to pick up the review.

action abadger1999's draft, plus additional exception wording approved (+1:7, 0:0, -1:0)

The two additions to the draft were:

  • |Do not apply patches directly from RPM_SOURCE_DIR| Applying patches directly from RPM_SOURCE_DIR (as opposed to those from a tarball) is not allowed. Please see Packaging:RPM_Source_Dir for the complete rationale
  • This exception also applies if you're taking a large pre-compressed patch from upstream as patches that are extremely large are not going to be auditable anyway.

The first was to make sure people understood that this does not override the concerns raised against using RPM_SOURCE_DIR directly. (In essence, the tarball is the manifest that is listed as an exception in the RPM_SOURCE_DIR guidelines)

The second was to allow things like the kernel rc patchset to use this exception as the sheer volume of (unrelated) changes negates the benefits of checking into revision control.

I don't think this was ever written up.

Finally written up. I did the best I could given the lack of a real draft in this ticket, but it required a bit of rewording (and I also had to allow %autosetup).

Announcement text:

Guidelines for the application of patches have been added: https://fedoraproject.org/wiki/Packaging:Guidelines#Applying_patches

Metadata Update from @rdieter:
- Issue assigned to tibbs

3 years ago

Login to comment on this ticket.

Metadata