[vcpkg doc] Document patching cooldown period guidelines as discussed 2022-10-20

[BillyONeal]

With:

@BillyONeal @ras0219-msft @valeriaconde @JavierMatosD @vicroms @markle11m @danshaw

The intent is not to change anything about our patch acceptance criteria; merely to document what those criteria are so that different vcpkg maintainers give different results.

[BillyONeal]

This topic came up as a result of #27260

See also: #27379

[Neumann-A]

What about build system/script fixes/changes and about CMakelists.txt vcpkg owns (could also be considered patching)?

[dg0yt]

This topic came up as a result of #27260

What about build system/script fixes/changes

Exactly.
In the past there seemed to be a more relaxed policy with regard to patches to the build system in order to control the build of tests, examples, docs and tools, in contrast to features that modify the actual source code. In fact, I felt encouragement to move tools out of default features. Aka "vcpkg is a library manager".
Raising the requirements for interaction with upstream will raise the barrier for actively contributing. Given the current quality of ports and of reviews, a stricter policy may do more harm than good. In particular with platorms which are not first tier neither in vcpkg nor upstream.

[Thomas1664]

In the past there seemed to be a more relaxed policy with regard to patches to the build system in order to control the build of tests, examples, docs and tools

Especially tests and examples can drastically increase build time or introduce extra dependencies. Therefore, I think it is ok to patch them out. In the case of docs, it depends on things like the size of docs or to wich location the files are installed. Given that the maintainer guide doesn't say "don't build tools by default" or tools might be needed to use the port, there is less argument to patch them out.

In general, I think that it depends: We have to balance usability [size, build time, install paths (e.g. for docs)] and maintainability of a port. In cases where the patch is very small, I think it is ok to patch out docs, examples or tests. Overall, this shouldn't be cosmetic changes.

Raising the requirements for interaction with upstream will raise the barrier for actively contributing.

We could at least partially make it easier to handle patches that don't apply anymore after a port update by providing a guide on this topic.

Is there any evidence that ports with at least one patch receive less PRs and is there a correlation between the size of patches and the number of PRs?

[dg0yt]

Is there any evidence that ports with at least one patch receive less PRs and is there a correlation between the size of patches and the number of PRs?

There is evidenve of ports being in barely useable state (at least for some "supported" triplets) over extended periods of time despite having patches. This is an indication of too few people either using the port or contributing to it.
There is evidence that users switched to other package manager due to unhandled port issues.

There is no reason to look at the size of patches here. Of course, the current patch in #27260 is bug because it is as-upstreamed. For vcpkg, a minimal patch would avoid the formatting changes, and just add if/ endif etc.

[BillyONeal]

@Neumann-A

What about build system/script fixes/changes and about CMakelists.txt vcpkg owns (could also be considered patching)?

We tend to do that only for very simple resulting build systems, and we should be notifying upstream at least 30 days in advance before doing so in the future.

---

@dg0yt:

In the past there seemed to be a more relaxed policy with regard to patches to the build system in order to control the build of tests, examples, docs and tools, in contrast to features that modify the actual source code.

Yes, because it is less likely that upstream would disagree with such changes. The intent is that "including but not limited to" and the associated reasons we try to avoid patching would carry the burden here. The problem is that it's difficult to codify exactly what kinds of build system changes are acceptable. Patching out tests/examples or warnings? Clearly OK. Patching in huge build system changes to support another architecture or similar? Clearly a problem.

Of course, the current patch in #27260 is bug because it is as-upstreamed.

#27260 certainly straddles the line on this policy. I argued that it is over the line of needing upstream approval attempted because it adds an entirely new configuration of this thing that users may expect upstream to be supporting. It isn't "upstreamed" while upstream's maintainers have yet to look at it.

Raising the requirements for interaction with upstream will raise the barrier for actively contributing.

The intent is not to raise the requirements for interaction with upstream; the intent is to clarify what our principles are for when upstream interaction is required, so that it doesn't change according to which particular vcpkg maintainer is reviewing the PR. We have always requested that upstream be kept in the loop for substantial changes, this is just codifying that we do that.

There is evidenve of ports being in barely useable state (at least for some "supported" triplets) over extended periods of time despite having patches. This is an indication of too few people either using the port or contributing to it.

Being an official triplet means that we test for it and have an opportunity to fix it if it breaks, not an obligation on the part of any library that wants to be indexed that it must support that triplet. (Although we do require that it support at least one to be in our curated registry)

There is evidence that users switched to other package manager due to unhandled port issues.

I agree that that's unfortunate but the alternative where we do things upstream disagrees with, and thus claim to be them but are not them, is worse. The Debian OpenSSL key generation bug was caused by similar well-meaning-maintainer-patches.

There is no reason to look at the size of patches here.

The size of the patch can matter due to the license-entanglement and maintainability parts. However, it's easy to write short patches that upstream would disagree with, so we intentionally included nothing here about patch size. A 1 line patch is not presumed good. A 1000 line patch is not presumed bad (if the patch is easy to maintain across versions and the patched thing is MIT licensed).

In particular with platorms which are not first tier neither in vcpkg nor upstream.

If it is going to create headaches for upstream that they may not want, they deserve the ability to say no. They are, ultimately, the owners of the installed content.

---

@Thomas1664

Especially tests and examples can drastically increase build time or introduce extra dependencies.

Added

* Disabling irrelevant-in-vcpkg components of the build such as tests or examples.

to the list of presumed-minimal. Adding features that strip out dependencies still should be worked out with upstream.

Given that the maintainer guide doesn't say "don't build tools by default" or tools might be needed to use the port, there is less argument to patch them out.

Unfortunately what "tools" means in this context is difficult to codify into guidelines. However, most libraries-that-come-with-tools also come with an upstream that cares about what happens with those tools, and if they care, we should be making an attempt to keep them in the loop.

We could at least partially make it easier to handle patches that don't apply anymore after a port update by providing a guide on this topic.

I wish I had good advice on how to handle that in general; in general I've been replaying all such changes manually :(

Is there any evidence that ports with at least one patch receive less PRs and is there a correlation between the size of patches and the number of PRs?

I don't know :(

[Neumann-A]

#27260 certainly straddles the line on this policy.

How about not adding spaces so that the patch is smaller? (even though the patch wouldn't be accepted upstream in that form )

If it is going to create headaches for upstream that they may not want, they deserve the ability to say no. They are, ultimately, the owners of the installed content.

I disagree. The used license determines usage. They can say that they don't support x but it doesn't stop user to use it on x without support (of course problems then have to be solved by yourself; and x is probably often windows ;) ).

in general I've been replaying all such changes manually :(

I also do that manually. Don't see a way to fix patches automatically.

Is there any evidence that ports with at least one patch receive less PRs and is there a correlation between the size of patches and the number of PRs?

In the past the glib chain (atk,cairo,glib, etc.) up to GTK was very outdated since it was all vcpkg vendored CMakeLists.txt. With the switch to the upstream buildsystem, it at least appears to me, that the chain is kept more in sync. Another example is #27444 where I thought at least 10 times of updating it but having the vendored CMakeLists.txt really stop me doing it.

[BillyONeal]

@ras0219-msft suggests moving this line below the list of cases where it doesn't apply.

[JonLiu1993]

Close this PR and please track it with microsoft/vcpkg-docs#35.