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

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

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

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

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

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEELUvHd/Gtp7LaU1vuzpXahU5EQpMFAmJBmHQACgkQzpXahU5E QpNDVgf/Z0ozQN4Q726qPm+i66mBxFk+DAB604hwW5hiJR127sUcwJBHSkvIO68W zMSOn9O/oRTbosDCEpQE+A5CoI+GJBZvteheL+Yxbx5KVMDPKYf92/vPISqeqi/t 9OY/JV7gh+xMOM7fdOz/A+b2KCj4LX5e+VZYkkVBkprlwPh+bUJJdKoHyOphTXmi kI0VN5Sb/dCYI16bE0VuMNtEzOSQrvQmuNpJMtAaLy3hldLmtJGYpXVQg+09WoxM nNz4I7R40easLwPkBaTYNeTK16ahI00X0/EglHIhVHFjLVUhsdFM6IP4f5IsXkJK H7Fc9Vo/YTHYfJb/4wcmwCeoMQ0rBw==
=qvPI
-----END PGP SIGNATURE-----

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 29 Mar 2022, at 04:43, Sam James <[email protected]> wrote:

On 28 Mar 2022, at 12:05, Thomas Bracht Laumann Jespersen <[email protected]> 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

-----BEGIN PGP SIGNATURE-----

iQGTBAEBCgB9FiEEYOpPv/uDUzOcqtTy9JIoEO6gSDsFAmJCgHFfFIAAAAAALgAo aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDYw RUE0RkJGRkI4MzUzMzM5Q0FBRDRGMkY0OTIyODEwRUVBMDQ4M0IACgkQ9JIoEO6g SDtVEwgAvGOM1sCKHvNcoaR10QeD+Rbetk+gvmuVoaCN/AczwRlJDv1dpseb9T4L zkA8M2BydevLcLEbu9JlN8ceDqsWvFT9sbNQbAhYv65JoJIQR/oyxTL9t2TLnZzQ BuATRXyOcVO4bg2n7OZxrUfTYCVqLNxt0lC9i3Q2RNxJ36x7D2+0o3GAq0z0Sy/6 eNdpi5I/g/tVpHI7LVqYj1WlDbye15+6xvf1tC4bYxXUogzrfeyml60BEMPy7L7G bqnXJFfc5PNhKtWYfzeQ+C7ISp9NJae4bzpKwFTYcbRZJnuyfXpCDJ0tIAaQtYbl 9R2ZePEaROShAFUDKPOhdgA2Ek/pqw==
=ykL3
-----END PGP SIGNATURE-----

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

On 28 Mar 2022, at 12:13, Fabian Groffen <[email protected]> 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

-----BEGIN PGP SIGNATURE-----

iQGTBAEBCgB9FiEEYOpPv/uDUzOcqtTy9JIoEO6gSDsFAmJCftRfFIAAAAAALgAo aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDYw RUE0RkJGRkI4MzUzMzM5Q0FBRDRGMkY0OTIyODEwRUVBMDQ4M0IACgkQ9JIoEO6g SDtraQf/fO1apVb6hJ9NUrDEuQA6Naz+PneX+yFJhKso7SNOINjhpTlo9v2LuAMY bATr7ZKOrbQpvJfmwL73Qb97i/BluIWbrDSiI4Njo8zvasx3FLfcbphYAx2hRRFV bpkuQ1GdbGfFjZ7So7y2acAsUvsYP77rsYxBQvrTm2JKxTY9HHTcCYZWIfoaj7l8 UmlDvFD8lv5vEjI8KX3bgSw/jsq8fvBIjWaji8hHyuevl+E7CqG64LP1ezNzjKpc Ate+/enWDuoDZPJsp8pRNliOvpFEwf0D89h/cu/yGAFQ8zqURAPEd8XlcHjUaWoU 6uXz1ar3+aZr9V64EJQg3vF3KZDiBw==
=Q1hj
-----END PGP SIGNATURE-----

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)