1 |
> On 28 Mar 2022, at 12:05, Thomas Bracht Laumann Jespersen <t@×××××××.xyz> wrote: |
2 |
> |
3 |
> Hi! |
4 |
> |
5 |
> I've been working on a new section in the devmanual regarding conditional |
6 |
> patching. In a PR [0] Sam suggested adding a section to clarify that conditional |
7 |
> patching should be avoided, because it can quickly become a maintenance and |
8 |
> testing burden. |
9 |
> |
10 |
> The devmanual PR is here: [1] |
11 |
> |
12 |
> Ulrich points out that conditional patching is actually suggested elsewhere, and |
13 |
> that actively discouraging conditional patching is a policy change, which should |
14 |
> be brought up on this list. So this is what I'm doing now. He also pointed out a |
15 |
> comment in eutils.eclass re [2] (the comment now lives in epatch.eclass). |
16 |
> |
17 |
|
18 |
Not convinced it is a policy change explicitly as we do soft-discourage it, |
19 |
but having the discussion is worthwhile. |
20 |
|
21 |
I suppose it's harder to argue with the odd example of conditional patching |
22 |
in the devmanual not being accompanied with a warning though. :) |
23 |
|
24 |
> The overall policy (proposal, I guess) is something like: |
25 |
> |
26 |
> * Patches should be written such that they affect behaviour correctly based on |
27 |
> e.g. build time definitions or config options, rather than USE flags |
28 |
> directly. |
29 |
> |
30 |
> * They should be applied unconditionally, so that, for version bumps and other |
31 |
> development happening with a package, any failure to apply the patch will be |
32 |
> caught by the developer. |
33 |
|
34 |
Yep. Although we're somewhat accepting of this in Prefix land, it does |
35 |
still happen with odd use combinations. |
36 |
|
37 |
I have less of an issue with this for prefix & kind of musl (although musl stuff |
38 |
should really be done in a standards-compliant way & upstreamed) because |
39 |
especially for prefix, sometimes there is no easy way to do this properly, and |
40 |
at least a patch for prefix users will fail hard rather than silently breaking |
41 |
(e.g. sed). |
42 |
|
43 |
> |
44 |
> * Patching is ideally only done to make the package in question build properly, |
45 |
> and should not be done to modify the runtime behaviour of the package. (This |
46 |
> is what USE flags and configuration options of the package are for.) |
47 |
> |
48 |
|
49 |
I have another reason for why this is important: a conditional patch can be pretty |
50 |
much never be upstreamed. There might be some cases where we're conditionally |
51 |
applying it out of conservatism (which we should probably re-evaluate) even though |
52 |
it has e.g. appropriate configure options, but most of the time, a conditional patch |
53 |
is unconditionally applying functionality and therefore isn't upstreamable in its |
54 |
current state. |
55 |
|
56 |
|
57 |
> Sam made a specific point re musl: "for e.g. musl patches, we want a portable |
58 |
> fix, not a hack which is only applied for musl" |
59 |
|
60 |
Yep. I'd note that sometimes (as I say in my reply to grobian), it's okay, but |
61 |
we want to discourage it for sure. |
62 |
|
63 |
> |
64 |
> Feedback on this very welcome. I'm grappling a bit with the exact wording to go |
65 |
> for, so input on that is also appreciated. |
66 |
> |
67 |
> I think my question to this list is: Should it be policy that conditional |
68 |
> patching is to be avoided? |
69 |
> |
70 |
|
71 |
Thanks for bringing this up. |
72 |
|
73 |
> All the best, |
74 |
> -- Thomas |
75 |
|
76 |
Best, |
77 |
sam |