Subject: Enforcing patch headers for patches

From: Rasmus Thomsen <[email-masked]>
Date: Mon, 27 Apr 2020 12:23:43 +0200

Hello,

I'd like to propose that we start enforcing patch headers for new
patches in aports, to make the flood of (downstream) patches more
manageable. With a patch header a patch would look like this:

$ cat xfce-screenshooter-1.7.9-dsofix.patch
Upstream: Yes
Reason: Without this patch linking with bfd fails due to missing linker
flags, as these libraries are implicitly required.
Url: https://bugzilla.xfce.org/show_bug.cgi?id=7985
--- xfce4-screenshooter-1.7.9.orig/Makefile.in	2010-02-07
14:45:15.000000000 +0100
+++ xfce4-screenshooter-1.7.9/Makefile.in	2010-02-16
23:57:31.000000000 +0100
@@ -282,7 +282,7 @@
 INTLTOOL_PERL = @INTLTOOL_PERL@
 INTLTOOL_UPDATE = @INTLTOOL_UPDATE@
 LD = @LD@
-LDFLAGS = @LDFLAGS@
+LDFLAGS = @LDFLAGS@ -lm -lX11
 LIBOBJS = @LIBOBJS@
 LIBS = @LIBS@
 LIBTOOL = @LIBTOOL@

Let me explain the purpose of the different fields:

* Upstream: This should indicate whether the patch is proposed
upstream/already merged or if it's specific to us. Should be "Yes" if
it has been proposed/merged already, or "No" with an explanation why it
can't be upstreamed. This helps tracking the status of a patch
upstream. Mustn't be omitted.

* Reason: This describes what the patch is for. That makes it easier
for others (or sometimes yourself at a later point) to figure out what
a patch actually does. This field may be omitted if the patch is
trivial and has a descriptive file name, or if the patch already has a
commit message in it (e.g. from git format-patch).

* Url: An URL to where upstream deals with the issue, e.g. a link to
upstream's issue report in their bugtracker or a link to a merge
request. This may only be omitted if Upstream is "No" (meaning the
patch is specific to Alpine Linux). This has the nice consequence that
all patches that aren't specific to Alpine Linux have to be submitted
to upstream, or the issue the patch tries to solve at least has to be
reported at upstream. This lessens the maintenance burden on us (since
we won't have to maintain as many downstream patches then) and makes
the patches available for other developers as well.

While these patch header seem like a lot of boilerplate up front, it
really does pay off in the long run. It probably takes less than a
minute to write a patch header for the submitter of the patch since all
of the required info is already readily available to the submitter:
They know what the patch is for, what the upstream status is etc. It
might take hours for other people who find out what a patch is for and
whether it has been proposed upstream already at a later point.

Regards,

Rasmus

---

From: Leo <[email-masked]>
Date: Mon, 27 Apr 2020 18:10:20 -0300

On Mon, 27 Apr 2020 12:23:43 +0200
Rasmus Thomsen <[email-masked]> wrote:

> Reason: Without this patch linking with bfd fails due to missing
> linker flags, as these libraries are implicitly required.
> it has been proposed/merged already, or "No" with an explanation why
> it can't be upstreamed. This helps tracking the status of a patch
> minute to write a patch header for the submitter of the patch since
> all of the required info is already readily available to the
> submitter: They know what the patch is for, what the upstream status
> is etc. It might take hours for other people who find out what a
> patch is for and whether it has been proposed upstream already at a
> later point.
> 
> Regards,
> 
> Rasmus

+1

---

From: Kevin Daudt <[email-masked]>
Date: Tue, 28 Apr 2020 10:06:47 +0200

> --- xfce4-screenshooter-1.7.9.orig/Makefile.in	2010-02-07

I think this is a good idea as well, just a few points:

* How are we going to handle the existing patches?
* What if there is already a commit message present in the patch? Are we
  going to prepend the header before that?

Kevin

---

From: Rasmus Thomsen <[email-masked]>
Date: Tue, 28 Apr 2020 10:28:46 +0200

> * How are we going to handle the existing patches?

I think we should just leave them as-is. It'd be a lot of work to
rework all patches now.

>   going to prepend the header before that?

Yes, the patch header should be at the top of the file. The "Reason"
field of the patch header can be omitted if there's a commit message in
the patch since that should explain the reason for having the patch
already. The "Url" and "Upstream" field should be added though, so that
we can track progress of getting things upstream.

Thanks,

Rasmus

---

From: Ariadne Conill <[email-masked]>
Date: Wed, 29 Apr 2020 21:57:44 -0600

> --- xfce4-screenshooter-1.7.9.orig/Makefile.in	2010-02-07

I am fine with this proposal with the modification that we can skip the patch 
header for changes cherry-picked directly from upstream.  My rationale is that 
the information is redundant for cherry-picks.

Ariadne

---

From: Rasmus Thomsen <[email-masked]>
Date: Thu, 30 Apr 2020 12:49:11 +0200

> the information is redundant for cherry-picks.

Hm, I think it's still useful to have a link to the upstream commit,
e.g.

Url: 
https://github.com/Cogitri/corecollector/commit/d18727d3b01996b523b73ead3ab577a723a7aa9d

But IMO it'd be fine to just have:

Git commit: d18727d3b01996b523b73ead3ab577a723a7aa9d

or something like that too.

Regards,

Rasmus

---

From: Natanael Copa <[email-masked]>
Date: Thu, 30 Apr 2020 10:28:23 +0200

> --- xfce4-screenshooter-1.7.9.orig/Makefile.in	2010-02-07

What should we set it to if the patch is suitable for upstreaming but
not yet submitted/reported upstream?

What if we are unsure if its suitable for upstreaming?

What happens if it was proposed upstream but later rejected or
reverted (eg. upstream finds ou they don't want support musl at all)

What if we lift patches from debian or fedora and they have different
syntax for similar do we need to convert it to our format?

I am afraid of needing to change the upstreaming state of the patch
when it changes or that the specified upstreaming state information
getting outdated. This info becomes useless if it cannot be trusted.

May be good to have some indication for patches that are not
upstreamable and alpine specific. Maybe this is what you mean with
"Upstream: no"

> the patches available for other developers as well.

There may be various different URLs. Where the patch comes from, URL to
bug tracker, URL to other distros bugtracker or mailing list
discussions.

> patch is for and whether it has been proposed upstream already at a

I agree that having some reference where the patch comes from, and
where to find upstream status is useful. I try (tried?) add that to
patches whenever I can.

---

From: Rasmus Thomsen <[email-masked]>
Date: Thu, 30 Apr 2020 12:45:40 +0200

> not yet submitted/reported upstream?

Then you should upstream it and add the Url to the patch. The "I'll
upstream it sometime later" thing often times doesn't work out and we
end up having yet another downstream patch.

> What if we are unsure if its suitable for upstreaming?

Then you should ask upstream (or someone else who knows it, e.g. the
reviewers of your MR) if it's suitable for upstreaming.

> reverted (eg. upstream finds ou they don't want support musl at all)

Then we'll have to keep it downstream, but because we have a "Url"
field we can track what happened to the thing, e.g. because upstream
commented on the issue we linked that they don't want to support musl.
The patch doesn't have to be updated then, that'd be a lot of effort,
but I don't think that this will happen too often.

> syntax for similar do we need to convert it to our format?

Hm, although I think it'd be best if everything followed that format,
so you don't have to spend too much time figuring out where to find
information about a patch as reviewer/future maintainer/etc. I
understand that that might be too much effort. I think if the
Debian/Fedora patches are similiar enough (as in they provide the
information we need, namely upstream status, an URL to track progress
and an explanation) we can use them too.

> getting outdated. This info becomes useless if it cannot be trusted.

That's what the URL is for - if you want more info you can easily look
it up by the URL value. Here is an example:

Upstream: Yes, pending
Url: https://github.com/ximion/appstream-generator/pull/75
Reason: Add support for scraping .apk packages for .desktop and
.appdata.xml files

Once this PR gets merged we don't have to touch the patch - interested
parties can just check the Url. After thinking about it for a bit,
maybe we could live with just an Upstream and a Reason field, like so:

Upstream: Yes, pending: 
https://github.com/ximion/appstream-generator/pull/75
Reason: Add support for scraping .apk packages for .desktop and
.appdata.xml files

The "Upstream" field isn't necessarily meant to indicate that a patch
has been accepted upstream, it's just an indicator that upstream knows
about the patch. This is the case when we opened an MR about it or we
cherry picked a commit from upstream etc.

> upstreamable and alpine specific. Maybe this is what you mean with

Yes, although it should be "Upstream: No, because $reason" :)

> bug tracker, URL to other distros bugtracker or mailing list

I think the URL most relevant to the patch should be used. E.g. if the
patch has been submitted upstream but not yet merged, link to the MR.
If the patch is cherry picked from upstream you can link to the commit.
If it's from a mailing list, link to that, etc.
The Url: field isn't meant to inform the reader about why a patch is
applied (that's what Reason: is for), it's meant so people looking at
the patch at a later point can check what the progress of getting the
patch upstreamed is or if there are new versions fo the patch.

> where to find upstream status is useful. I try (tried?) add that to

Yup, I've seen a few patches which already use a similiar scheme, but
since we don't enforce it currently we still have loads of patches
which don't have this and it's a lot of work to search for the
information that is easily accessible from the patch header later on.

---

From: Natanael Copa <[email-masked]>
Date: Thu, 30 Apr 2020 16:20:51 +0200

On Thu, 30 Apr 2020 12:45:40 +0200

> end up having yet another downstream patch.

I agree that the best thing is to upstream patches, but...

I'll be honest with you. I have upstreamed patches for a decade. I am
tired. Specially when I have something that needs to get done
relatively quick (lets say a security patch). But someone else broke
the builders. It is not uncommon that I fix whatever is blocking to get
my own work done, and I don't have time or energy to find out how to
upstream (eg register to yet another mailing list or create another
user in bugzilla, google how to use mercurial) and prepare the patch in
an upstream friendly way (eg make sure it does not change behavior on
glibc) and test that it didnt break BSDs and glibc and then spend a
week on following up discussions with upstream. If this is enforced,
then I will revert stuff that breaks. (which does not always work
either, because old version may not build either in case breakage was
caused by other component such as compiler).

This does not mean that I never upstream patches. I often do. I'm just
tired and would appreciate help with upstreaming patches.

I just don't want it to be a hard requirement.

> reviewers of your MR) if it's suitable for upstreaming.

A lot of patches solves immediate issues for alpine or musl, but need
special care to be portable and not break GNU libc. And it is not
uncommon that upstream don't understand their own code or the problem
at hand. For example ruby main stack size calculation patch. their
comments indicate that they don't understand the problem nor the fix,
so I neeed to rearrange the patch to make it more *visible* that it
does not change behavior on glibc.

https://bugs.ruby-lang.org/issues/14387 (I have already prepared an
updated patch but I dont think I have taken the time to submit it yet).

What I'm trying to say is that sometimes even small and simple patches
that does not cost us much to carry can require significant effort to
upstream.

> cherry picked a commit from upstream etc.

What I don't want is keep track of the upstream state and update the
patch file everytime upstream state changes. For example if the pending
patch gets applied, then I don't want be expected to go back to the
patch in our aports tree and change "Upstream: pending" to "Upstream:
applied". Rather, I just want let it be til next time I update the
package at which time the patch fails to apply. I can then look at the
upstream state (git log, issue tracker whatever) and remove the patch
in our tree.

So I don't think "pending" is useful info. But an URL to upstream state
is definitively useful.
 
> Yes, although it should be "Upstream: No, because $reason" :)

Is "Upstream: No, because I don't have the energy to deal with upstream
this week" acceptable? :)
 
> If the patch is cherry picked from upstream you can link to the
> commit. If it's from a mailing list, link to that, etc.

What I mean is that we may want to have:

Upstream-bug: ...
Upstream-commit: ...

Instead of: 

URL: url-to-bugtracker
URL: url-to-commit

> information that is easily accessible from the patch header later on.

-nc

---

From: Nathan Angelacos <[email-masked]>
Date: Sat, 02 May 2020 22:13:54 -0400

On Thu, 2020-04-30 at 16:20 +0200, Natanael Copa wrote:

<valid rant>
<snip>
</valid rant>

I'm done.  Don't care either way.  

kamailio can't compile because qemu requires 1.4 nettle and a new
compile of kamailio requires nettle 1.5

I 100% agree that we should add headers to our patches to say "why does
this exist" 

Upstreaming has been a PITA for me, personally.

I think what Rasmus and Ariadane have been advocating the past few
weeks is "let's reduce friction" - and I 100% agree... but we are
missing something.  

I dunno what... but we are saying "you must do X", and the thing is
"the 'real world (what the heck is that anymore?)' works like Y".

I think the bigger issue is "how do we move forward, document what AND
WHY we need to patch, and ... move forward?"

Rasmus, Ariadane - I really, really appreciate your input.  Personally,
this has been an intelligent conversation.

Sorry.. extra grumpy tonight.