* [gentoo-dev] Policy on conditional patching @ 2022-03-28 11:05 Thomas Bracht Laumann Jespersen 2022-03-28 11:13 ` Fabian Groffen 2022-03-29 3:43 ` Sam James 0 siblings, 2 replies; 6+ messages in thread From: Thomas Bracht Laumann Jespersen @ 2022-03-28 11:05 UTC (permalink / raw To: gentoo-dev Hi! I've been working on a new section in the devmanual regarding conditional patching. In a PR [0] Sam suggested adding a section to clarify that conditional patching should be avoided, because it can quickly become a maintenance and testing burden. The devmanual PR is here: [1] Ulrich points out that conditional patching is actually suggested elsewhere, and that actively discouraging conditional patching is a policy change, which should be brought up on this list. So this is what I'm doing now. He also pointed out a comment in eutils.eclass re [2] (the comment now lives in epatch.eclass). The overall policy (proposal, I guess) is something like: * Patches should be written such that they affect behaviour correctly based on e.g. build time definitions or config options, rather than USE flags directly. * They should be applied unconditionally, so that, for version bumps and other development happening with a package, any failure to apply the patch will be caught by the developer. * Patching is ideally only done to make the package in question build properly, and should not be done to modify the runtime behaviour of the package. (This is what USE flags and configuration options of the package are for.) Sam made a specific point re musl: "for e.g. musl patches, we want a portable fix, not a hack which is only applied for musl" Feedback on this very welcome. I'm grappling a bit with the exact wording to go for, so input on that is also appreciated. I think my question to this list is: Should it be policy that conditional patching is to be avoided? All the best, -- Thomas [0]: https://github.com/gentoo/gentoo/pull/24709#discussion_r832361402 [1]: https://github.com/gentoo/devmanual/pull/281 [2]: https://gitweb.gentoo.org/archive/repo/gentoo-2.git/tree/eclass/eutils.eclass?id=50e8beda904760c773e5c67fdfe8242255e13495#n175 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [gentoo-dev] Policy on conditional patching 2022-03-28 11:05 [gentoo-dev] Policy on conditional patching Thomas Bracht Laumann Jespersen @ 2022-03-28 11:13 ` Fabian Groffen 2022-03-28 12:09 ` Thomas Bracht Laumann Jespersen 2022-03-29 3:36 ` Sam James 2022-03-29 3:43 ` Sam James 1 sibling, 2 replies; 6+ messages in thread From: Fabian Groffen @ 2022-03-28 11:13 UTC (permalink / raw To: gentoo-dev [-- Attachment #1: Type: text/plain, Size: 2964 bytes --] Hi, On 28-03-2022 13:05:03 +0200, Thomas Bracht Laumann Jespersen wrote: > Hi! > > I've been working on a new section in the devmanual regarding conditional > patching. In a PR [0] Sam suggested adding a section to clarify that conditional > patching should be avoided, because it can quickly become a maintenance and > testing burden. > > The devmanual PR is here: [1] > > Ulrich points out that conditional patching is actually suggested elsewhere, and > that actively discouraging conditional patching is a policy change, which should > be brought up on this list. So this is what I'm doing now. He also pointed out a > comment in eutils.eclass re [2] (the comment now lives in epatch.eclass). > > The overall policy (proposal, I guess) is something like: > > * Patches should be written such that they affect behaviour correctly based on > e.g. build time definitions or config options, rather than USE flags > directly. > > * They should be applied unconditionally, so that, for version bumps and other > development happening with a package, any failure to apply the patch will be > caught by the developer. > > * Patching is ideally only done to make the package in question build properly, > and should not be done to modify the runtime behaviour of the package. (This > is what USE flags and configuration options of the package are for.) > > Sam made a specific point re musl: "for e.g. musl patches, we want a portable > fix, not a hack which is only applied for musl" > > Feedback on this very welcome. I'm grappling a bit with the exact wording to go > for, so input on that is also appreciated. > > I think my question to this list is: Should it be policy that conditional > patching is to be avoided? We really don't want conditional patches, but I from my experience reality is that sometimes you have to. Therefore this should remain possible. I have no problems with highly discouraging conditional patching. About your other points, I think they are kind of debatable. Gentoo wants to be close to upstream, but with your set of rules, one can't e.g. add hpn patches to ssh, which would be really silly, as the point of Gentoo is that since you build from source, you can actually apply such patches, conditionally if they don't have a means to be disabled. In other words, I think the gist of your points is to be in an ideal world, but unfortunately reality is far from it. That said, repeating myself, nothing wrong with discouraging quick 'n' dirty, for as long as it remains a big fat warning and advice. My €0.02 Fabian > [0]: https://github.com/gentoo/gentoo/pull/24709#discussion_r832361402 > [1]: https://github.com/gentoo/devmanual/pull/281 > [2]: https://gitweb.gentoo.org/archive/repo/gentoo-2.git/tree/eclass/eutils.eclass?id=50e8beda904760c773e5c67fdfe8242255e13495#n175 > -- Fabian Groffen Gentoo on a different level [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [gentoo-dev] Policy on conditional patching 2022-03-28 11:13 ` Fabian Groffen @ 2022-03-28 12:09 ` Thomas Bracht Laumann Jespersen 2022-03-29 3:36 ` Sam James 1 sibling, 0 replies; 6+ messages in thread From: Thomas Bracht Laumann Jespersen @ 2022-03-28 12:09 UTC (permalink / raw To: gentoo-dev > We really don't want conditional patches, but I from my experience > reality is that sometimes you have to. Therefore this should remain > possible. I have no problems with highly discouraging conditional > patching. > > About your other points, I think they are kind of debatable. Gentoo > wants to be close to upstream, but with your set of rules, one can't > e.g. add hpn patches to ssh, which would be really silly, as the point > of Gentoo is that since you build from source, you can actually apply > such patches, conditionally if they don't have a means to be disabled. Thanks for the input :-) I understand that it's not 100% avoidable, which is why I was careful to avoid the words like "rule" here. It's not my intention to propose any rules here. But that's maybe something that needs to be explicitly stated in the devmanual? "Conditional patching can sometimes be necessary, and as such it's not a hard rule that it shouldn't happen." > In other words, I think the gist of your points is to be in an ideal > world, but unfortunately reality is far from it. That said, repeating > myself, nothing wrong with discouraging quick 'n' dirty, for as long as > it remains a big fat warning and advice. Noted. Thanks again! -- Thomas ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [gentoo-dev] Policy on conditional patching 2022-03-28 11:13 ` Fabian Groffen 2022-03-28 12:09 ` Thomas Bracht Laumann Jespersen @ 2022-03-29 3:36 ` Sam James 1 sibling, 0 replies; 6+ messages in thread From: Sam James @ 2022-03-29 3:36 UTC (permalink / raw To: gentoo-dev [-- Attachment #1: Type: text/plain, Size: 3575 bytes --] > On 28 Mar 2022, at 12:13, Fabian Groffen <grobian@gentoo.org> wrote: > > Hi, > > On 28-03-2022 13:05:03 +0200, Thomas Bracht Laumann Jespersen wrote: >> Hi! >> >> I've been working on a new section in the devmanual regarding conditional >> patching. In a PR [0] Sam suggested adding a section to clarify that conditional >> patching should be avoided, because it can quickly become a maintenance and >> testing burden. >> >> The devmanual PR is here: [1] >> >> Ulrich points out that conditional patching is actually suggested elsewhere, and >> that actively discouraging conditional patching is a policy change, which should >> be brought up on this list. So this is what I'm doing now. He also pointed out a >> comment in eutils.eclass re [2] (the comment now lives in epatch.eclass). >> As I say below, I somewhat think this is already de-facto policy to avoid. But having the discussion is not a bad thing. >> [snip] >> >> I think my question to this list is: Should it be policy that conditional >> patching is to be avoided? > > We really don't want conditional patches, but I from my experience > reality is that sometimes you have to. Therefore this should remain > possible. I have no problems with highly discouraging conditional > patching. I'm not suggesting banning them -- just codifying that they're discouraged unless unavoidable, which is what we "soft advise" right now anyway in both proxy-maint reviews but in general culture like mentoring. > > About your other points, I think they are kind of debatable. Gentoo > wants to be close to upstream, but with your set of rules, one can't > e.g. add hpn patches to ssh, which would be really silly, as the point > of Gentoo is that since you build from source, you can actually apply > such patches, conditionally if they don't have a means to be disabled. OpenSSH is actually a fantastic example of why it's a bad idea but the maintainers choose to live with the compromises (which is fine). I say "bad idea" because it leads to poor UX. We have to avoid doing bumps until the patches are out or live with pkg_setup die()s when they're not yet available. And then we get bugs filed for it. (This isn't a criticism of the OpenSSH maintainers in Gentoo, it's a special case for sure, but it's a great example of why we shouldn't be doing it en-masse unless we must.) > > In other words, I think the gist of your points is to be in an ideal > world, but unfortunately reality is far from it. That said, repeating > myself, nothing wrong with discouraging quick 'n' dirty, for as long as > it remains a big fat warning and advice. > Yep, the plan for this is big fat warning & advice. Not unconditionally banning condiitonal patching. > My €0.02 Appreciated as ever -- especially given you work in interesting corners like Prefix and now increasingly musl (yay!) Indeed, this is part of why we can't really ban it absolutely (not that I'm advocating for that anyway) -- for prefix and musl, while we want upstreamable patches, it's not always easy. Especially for more stale components of the toolchain or system which are critical but upstreaming is not feasible due to inactivity or whatever. > Fabian > >> [0]: https://github.com/gentoo/gentoo/pull/24709#discussion_r832361402 >> [1]: https://github.com/gentoo/devmanual/pull/281 >> [2]: https://gitweb.gentoo.org/archive/repo/gentoo-2.git/tree/eclass/eutils.eclass?id=50e8beda904760c773e5c67fdfe8242255e13495#n175 >> > Best, sam [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 618 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [gentoo-dev] Policy on conditional patching 2022-03-28 11:05 [gentoo-dev] Policy on conditional patching Thomas Bracht Laumann Jespersen 2022-03-28 11:13 ` Fabian Groffen @ 2022-03-29 3:43 ` Sam James 2022-03-29 3:43 ` Sam James 1 sibling, 1 reply; 6+ messages in thread From: Sam James @ 2022-03-29 3:43 UTC (permalink / raw To: gentoo-dev [-- Attachment #1: Type: text/plain, Size: 3149 bytes --] > On 28 Mar 2022, at 12:05, Thomas Bracht Laumann Jespersen <t@laumann.xyz> wrote: > > Hi! > > I've been working on a new section in the devmanual regarding conditional > patching. In a PR [0] Sam suggested adding a section to clarify that conditional > patching should be avoided, because it can quickly become a maintenance and > testing burden. > > The devmanual PR is here: [1] > > Ulrich points out that conditional patching is actually suggested elsewhere, and > that actively discouraging conditional patching is a policy change, which should > be brought up on this list. So this is what I'm doing now. He also pointed out a > comment in eutils.eclass re [2] (the comment now lives in epatch.eclass). > Not convinced it is a policy change explicitly as we do soft-discourage it, but having the discussion is worthwhile. I suppose it's harder to argue with the odd example of conditional patching in the devmanual not being accompanied with a warning though. :) > The overall policy (proposal, I guess) is something like: > > * Patches should be written such that they affect behaviour correctly based on > e.g. build time definitions or config options, rather than USE flags > directly. > > * They should be applied unconditionally, so that, for version bumps and other > development happening with a package, any failure to apply the patch will be > caught by the developer. Yep. Although we're somewhat accepting of this in Prefix land, it does still happen with odd use combinations. I have less of an issue with this for prefix & kind of musl (although musl stuff should really be done in a standards-compliant way & upstreamed) because especially for prefix, sometimes there is no easy way to do this properly, and at least a patch for prefix users will fail hard rather than silently breaking (e.g. sed). > > * Patching is ideally only done to make the package in question build properly, > and should not be done to modify the runtime behaviour of the package. (This > is what USE flags and configuration options of the package are for.) > I have another reason for why this is important: a conditional patch can be pretty much never be upstreamed. There might be some cases where we're conditionally applying it out of conservatism (which we should probably re-evaluate) even though it has e.g. appropriate configure options, but most of the time, a conditional patch is unconditionally applying functionality and therefore isn't upstreamable in its current state. > Sam made a specific point re musl: "for e.g. musl patches, we want a portable > fix, not a hack which is only applied for musl" Yep. I'd note that sometimes (as I say in my reply to grobian), it's okay, but we want to discourage it for sure. > > Feedback on this very welcome. I'm grappling a bit with the exact wording to go > for, so input on that is also appreciated. > > I think my question to this list is: Should it be policy that conditional > patching is to be avoided? > Thanks for bringing this up. > All the best, > -- Thomas Best, sam [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 618 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [gentoo-dev] Policy on conditional patching 2022-03-29 3:43 ` Sam James @ 2022-03-29 3:43 ` Sam James 0 siblings, 0 replies; 6+ messages in thread From: Sam James @ 2022-03-29 3:43 UTC (permalink / raw To: gentoo-dev [-- Attachment #1: Type: text/plain, Size: 605 bytes --] > On 29 Mar 2022, at 04:43, Sam James <sam@gentoo.org> wrote: > > > >> On 28 Mar 2022, at 12:05, Thomas Bracht Laumann Jespersen <t@laumann.xyz> wrote: >> >> Hi! >> >> I've been working on a new section in the devmanual regarding conditional >> patching. In a PR [0] Sam suggested adding a section to clarify that conditional >> patching should be avoided, because it can quickly become a maintenance and >> testing burden. >> ... and just to be clear, I'm not suggesting we ban it. Just discourage it so that people are aware it should be avoided where possible. Best, sam [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 618 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-29 3:43 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-03-28 11:05 [gentoo-dev] Policy on conditional patching Thomas Bracht Laumann Jespersen 2022-03-28 11:13 ` Fabian Groffen 2022-03-28 12:09 ` Thomas Bracht Laumann Jespersen 2022-03-29 3:36 ` Sam James 2022-03-29 3:43 ` Sam James 2022-03-29 3:43 ` Sam James
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox