Hi
I am building software for misc distributions for over 11 years. And so far Fedora packages are the worst of those I played with (mostly OpenEmbedded and Debian).
Why? Because patches are mess. Let's take random one:
@@ -108,7 +108,7 @@ M = int(max(r, g, b)) m = int(min(r, g, b)) val = (2 * M + r + g + b) / 5 - p[:] = (val + r) / 2, (val + g) / 2, (val + b) / 2 + #p[:] = (val + r) / 2, (val + g) / 2, (val + b) / 2 if alpha[y][x] >= 250: alpha[y][x] = 255 - (M - m) * 3 / 4 del pixels
Who knows what it does and why? For some reason it has a name '64bitfix' but why it is needed? Did upstream ever saw it? No idea.
In Debian (or in OpenEmbedded) it is solved by implementing DEP-3 [1] which is set of requirements about extra metadata in patches such as:
- Description or Subject (required) - Origin (required except if Author is present) - Bug-<Vendor> or Bug (optional) - Forwarded (optional) - Author or From (optional) - Reviewed-by or Acked-by (optional) - Last-Update (optional) - Applied-Upstream (optional)
1. http://dep.debian.net/deps/dep3/
Examples:
------------------------------------------------------------------------------------------------ A patch cherry-picked from upstream:
From: Ulrich Drepper drepper@redhat.com Subject: Fix regex problems with some multi-bytes characters
* posix/bug-regex17.c: Add testcases. * posix/regcomp.c (re_compile_fastmap_iter): Rewrite COMPLEX_BRACKET handling.
Origin: upstream, http://sourceware.org/git/?p=glibc.git;a=commitdiff;h=bdb56bac Bug: http://sourceware.org/bugzilla/show_bug.cgi?id=9697 Bug-Debian: http://bugs.debian.org/510219
A patch created by the Debian maintainer John Doe, which got forwarded and rejected:
Description: Use FHS compliant paths by default Upstream is not interested in switching to those paths. . But we will continue using them in Debian nevertheless to comply with our policy. Forwarded: http://lists.example.com/oct-2006/1234.html Author: John Doe johndoe-guest@users.alioth.debian.org Last-Update: 2006-12-21
A vendor specific patch not meant for upstream submitted on the BTS by a Debian developer:
Description: Workaround for broken symbol resolving on mips/mipsel The correct fix will be done in etch and it will require toolchain fixes. Forwarded: not-needed Origin: vendor, http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=80;bug=265678 Bug-Debian: http://bugs.debian.org/265678 Author: Thiemo Seufer ths@debian.org
A patch submitted and applied upstream:
Description: Fix widget frobnication speeds Frobnicating widgets too quickly tended to cause explosions. Forwarded: http://lists.example.com/2010/03/1234.html Author: John Doe johndoe-guest@users.alioth.debian.org Applied-Upstream: 1.2, http://bzr.example.com/frobnicator/trunk/revision/123 Last-Update: 2010-03-29
------------------------------------------------------------------------------------------------
Are there any plans on adding/enforcing such requirements at least for new patches?
Maintainers are not the only persons who work on their packages. Sometimes some random developers go though random packages for several reasons (fixing ftbfs on secondary architectures, mass rebuilds etc). There is also "bus factor" which can wipe maintainer from existence or people just orphan own packages.
Why we have to check patch after patch for their reason or upstream status?
Marcin Juszkiewicz mjuszkiewicz@redhat.com wrote:
Are there any plans on adding/enforcing such requirements at least for new patches?
Maintainers are not the only persons who work on their packages. Sometimes some random developers go though random packages for several reasons (fixing ftbfs on secondary architectures, mass rebuilds etc). There is also "bus factor" which can wipe maintainer from existence or people just orphan own packages.
Why we have to check patch after patch for their reason or upstream status?
Have you read the existent policy on this? http://fedoraproject.org/wiki/Packaging:Guidelines#Patch_Guidelines
Björn Persson
W dniu 28.08.2015 o 14:32, Björn Persson pisze:
Marcin Juszkiewicz mjuszkiewicz@redhat.com wrote:
Have you read the existent policy on this? http://fedoraproject.org/wiki/Packaging:Guidelines#Patch_Guidelines
Yes, I have read it. But lot of maintainers did not.
Example specfile:
Source1: %{name}.score Patch0: %{name}-0.7.1-userpmopts.patch Patch1: %{name}-0.7.1-64bitfix.patch Patch2: %{name}-0.7.1-blit-crash.patch
On 2015-08-28, 13:01 GMT, Marcin Juszkiewicz wrote:
Yes, I have read it. But lot of maintainers did not.
Example specfile:
Source1: %{name}.score Patch0: %{name}-0.7.1-userpmopts.patch Patch1: %{name}-0.7.1-64bitfix.patch Patch2: %{name}-0.7.1-blit-crash.patch
Well, perhaps instead of non-sensical (meaning, it cannot get to any meaningful resolution) discussion on devel, I would think that filing a bug could lead to something?
Matěj
W dniu 28.08.2015 o 18:02, Matěj Cepl pisze:
On 2015-08-28, 13:01 GMT, Marcin Juszkiewicz wrote:
Yes, I have read it. But lot of maintainers did not.
Example specfile:
Source1: %{name}.score Patch0: %{name}-0.7.1-userpmopts.patch Patch1: %{name}-0.7.1-64bitfix.patch Patch2: %{name}-0.7.1-blit-crash.patch
Well, perhaps instead of non-sensical (meaning, it cannot get to any meaningful resolution) discussion on devel, I would think that filing a bug could lead to something?
You mean mass bug reporting for each package where spec file lacks information about patches?
Prefer to spend that time on fixing those packages where aarch64 or other secondary architecture is not handled.
On 2015-08-28, 17:09 GMT, Marcin Juszkiewicz wrote:
You mean mass bug reporting for each package where spec file lacks information about patches?
No, just for the package where you are bothered by the lack of information. What I meant is that in the end all those tens of thousands of patches needs to be described one at the time. And by chatting about it on devel@ won't fix the description for one of them.
Matěj
Marcin Juszkiewicz wrote:
W dniu 28.08.2015 o 14:32, Björn Persson pisze:
Marcin Juszkiewicz mjuszkiewicz@redhat.com wrote:
Have you read the existent policy on this? http://fedoraproject.org/wiki/Packaging:Guidelines#Patch_Guidelines
Yes, I have read it. But lot of maintainers did not.
OK, so you're aware that the examples you point out don't comply with the policy. What is your proposal then? More policy? More manual reviews? Some kind of automated policy enforcement? Or do you expect that a different data format will result in better observance?
Björn Persson
Marcin Juszkiewicz wrote:
In Debian (or in OpenEmbedded) it is solved by implementing DEP-3 [1] which is set of requirements about extra metadata in patches such as:
- Description or Subject (required)
- Origin (required except if Author is present)
- Bug-<Vendor> or Bug (optional)
- Forwarded (optional)
- Author or From (optional)
- Reviewed-by or Acked-by (optional)
- Last-Update (optional)
- Applied-Upstream (optional)
I am opposed to any such scheme, because it means we could no longer produce our patches with diff without hand-editing them.
Such information, if needed, belongs into specfile comments.
Kevin Kofler
On Fri, 2015-08-28 at 14:40 +0200, Kevin Kofler wrote:
I am opposed to any such scheme, because it means we could no longer produce our patches with diff without hand-editing them.
Such information, if needed, belongs into specfile comments.
Let's do that then. openSUSE already has established required comments for patches in specfiles: https://en.opensuse.org/openSUSE:Packaging_Pa tches_guidelines#Patch_markup_.28also_called_.22Tagging_patches.22.29
It works well in practice. It's not hard to write, and it more helpful than the Fedora tradition of just adding a link to the upstream bug. Maintainers in openSUSE understand that it is required.
Michael Catanzaro wrote:
It works well in practice. It's not hard to write, and it more helpful than the Fedora tradition of just adding a link to the upstream bug. Maintainers in openSUSE understand that it is required.
To be honest, I don't see why we need to REQUIRE patch descriptions to begin with. And especially not more verbose ones than what we have now. A link to the upstream bug can be enough if the upstream bug contains the needed information (what the problem is, and what upstream release the fix will be included in). KDE bug reports usually contain this information, and also the author of the patch (which shouldn't be a required information though, it's enough to know that it comes from upstream) and a link to the upstream commit.
Kevin Kofler
W dniu 28.08.2015 o 14:40, Kevin Kofler pisze:
Marcin Juszkiewicz wrote:
In Debian (or in OpenEmbedded) it is solved by implementing DEP-3 [1] which is set of requirements about extra metadata in patches such as:
- Description or Subject (required)
- Origin (required except if Author is present)
- Bug-<Vendor> or Bug (optional)
- Forwarded (optional)
- Author or From (optional)
- Reviewed-by or Acked-by (optional)
- Last-Update (optional)
- Applied-Upstream (optional)
I am opposed to any such scheme, because it means we could no longer produce our patches with diff without hand-editing them.
Ever heard of "quilt"? It even has handling of sane spec files built-in.
My common way of tweaking Fedora patches is this:
cd ~/rpmbuild/fedora-packager/PACKAGENAME/ git up quilt setup -v *spec cd SOURCENAME-VERSION quilt push -a
And then I have upstream source with Fedora patches applied. 'quilt push/pop' allows me to apply/revert patches, 'quilt refresh' refreshes patch (amount of context lines etc can be configured, description is kept, diffstat can be added).
Try it one day?
Marcin Juszkiewicz wrote:
Ever heard of "quilt"? It even has handling of sane spec files built-in.
Sorry, but I'm opposed to requiring a specific tool to generate patches. Patches are made in many ways in the real world: plain diff, svn diff, git diff, … Requiring a tool that forces a specific workflow on us just because a normal diff is not good enough for you is a no go.
Kevin Kofler
On 08/30/2015 03:44 AM, Kevin Kofler wrote:
Marcin Juszkiewicz wrote:
Ever heard of "quilt"? It even has handling of sane spec files built-in.
Sorry, but I'm opposed to requiring a specific tool to generate patches. Patches are made in many ways in the real world: plain diff, svn diff, git diff, … Requiring a tool that forces a specific workflow on us just because a normal diff is not good enough for you is a no go.
You for got sed. (Look ma, no conflicts!)
And that alone is sufficient reason for me to wish for a switch to completely declarative patching. (I know it wont happen.)
On 08/28/2015 02:11 PM, Marcin Juszkiewicz wrote:
Hi
I am building software for misc distributions for over 11 years. And so far Fedora packages are the worst of those I played with (mostly OpenEmbedded and Debian).
Why? Because patches are mess. Let's take random one:
@@ -108,7 +108,7 @@ M = int(max(r, g, b)) m = int(min(r, g, b)) val = (2 * M + r + g + b) / 5
p[:] = (val + r) / 2, (val + g) / 2, (val + b) / 2
#p[:] = (val + r) / 2, (val + g) / 2, (val + b) / 2 if alpha[y][x] >= 250: alpha[y][x] = 255 - (M - m) * 3 / 4 del pixelsWho knows what it does and why? For some reason it has a name '64bitfix' but why it is needed? Did upstream ever saw it? No idea.
In Debian (or in OpenEmbedded) it is solved by implementing DEP-3 [1]
In reality, here's what the Debian version of this patch looks like:
I'm not sure if it's all that more helpful, to be honest. It does not follow DEP-3, sure, but neither do many other Debian packages. Even some critical server packages still do not have any broken-out patches at all.
(In general, if there is no upstream to contribute such fixes to, it's probably best not to ship such software at all.)
On Fri, Aug 28, 2015 at 8:44 AM, Florian Weimer fweimer@redhat.com wrote:
On 08/28/2015 02:11 PM, Marcin Juszkiewicz wrote:
Hi
I am building software for misc distributions for over 11 years. And so far Fedora packages are the worst of those I played with (mostly OpenEmbedded and Debian).
Why? Because patches are mess. Let's take random one:
@@ -108,7 +108,7 @@ M = int(max(r, g, b)) m = int(min(r, g, b)) val = (2 * M + r + g + b) / 5
p[:] = (val + r) / 2, (val + g) / 2, (val + b) / 2
#p[:] = (val + r) / 2, (val + g) / 2, (val + b) / 2 if alpha[y][x] >= 250: alpha[y][x] = 255 - (M - m) * 3 / 4 del pixelsWho knows what it does and why? For some reason it has a name '64bitfix' but why it is needed? Did upstream ever saw it? No idea.
In Debian (or in OpenEmbedded) it is solved by implementing DEP-3 [1]
In reality, here's what the Debian version of this patch looks like:
< http://sources.debian.net/src/monsterz/0.7.1-8/debian/patches/010_64-bit-ali...
I'm not sure if it's all that more helpful, to be honest. It does not follow DEP-3, sure, but neither do many other Debian packages. Even some critical server packages still do not have any broken-out patches at all.
(In general, if there is no upstream to contribute such fixes to, it's probably best not to ship such software at all.)
-- Florian Weimer / Red Hat Product Security -- devel mailing list devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/devel Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
If patches are exported from Mercurial or Git, you'd have all the information you'd want. However, most people I know aren't working from the hg/git repository when making packages. That said, I would seriously hope there would be comments in the spec indicating why the patch is needed. I know that when I have to make patches to packages, I will put comments right above the patch source line with information about the patch.
W dniu 28.08.2015 o 14:51, Neal Gompa pisze:
If patches are exported from Mercurial or Git, you'd have all the information you'd want.
Fully agree. Only info about upstream status is missing.
However, most people I know aren't working from the hg/git repository when making packages. That said, I would seriously hope there would be comments in the spec indicating why the patch is needed.
I know that when I have to make patches to packages, I will put comments right above the patch source line with information about the patch.
Often there is not even such information ;(
W dniu 28.08.2015 o 14:44, Florian Weimer pisze:
On 08/28/2015 02:11 PM, Marcin Juszkiewicz wrote:
Who knows what it does and why? For some reason it has a name '64bitfix' but why it is needed? Did upstream ever saw it? No idea.
In reality, here's what the Debian version of this patch looks like:
I'm not sure if it's all that more helpful, to be honest.
I would say that it is a bit better. Now I at least know that it is related to Python 2.5 (which we no longer ship) and some 64bit alignment issues due to it ;D
(In general, if there is no upstream to contribute such fixes to, it's probably best not to ship such software at all.)
But yes, with dead upstream it is not so funny.
Marcin Juszkiewicz wrote:
I would say that it is a bit better. Now I at least know that it is related to Python 2.5 (which we no longer ship)
It means the problem first showed up with 2.5, but chances are all later 2.x (and maybe also 3.x) are affected too.
Kevin Kofler
-
Björn Persson -
Florian Weimer -
Kevin Kofler -
Marcin Juszkiewicz -
Matěj Cepl -
Michael Catanzaro -
Neal Gompa