| Message ID | 20230919125757.1571746-2-rhi@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] contributor-guide: add docs for Upstream-Status patch headers | expand |
Hi again Roland On 19.09.23 at 14:57, Roland Hieber wrote: > Port and update the text from the OpenEmbedded wiki page [1], which has > since been deleted and replaced by a link to the Contributor Guide > (where not everything is available yet). The original text was available > under Creative Commons Attribution-ShareAlike 3.0, and the original > authors were wiki users Mhatle, Mariano, Benjamin, and McFrisk. > > [1]: https://www.openembedded.org/index.php?title=Commit_Patch_Message_Guidelines&oldid=10935 > > Signed-off-by: Roland Hieber <rhi@pengutronix.de> > --- > .../contributor-guide/recipe-style-guide.rst | 119 +++++++++++++++++++++ > 1 file changed, 119 insertions(+) Thanks for this update, but I told, I couldn't find your original submissions, and since then, I already merged such documentation into master-next. However, I see that the version on master-next (https://docs.yoctoproject.org/next/) doesn't cover CVE tags yet, and your patch did. Would you be interested in sending an update against master-next if necessary, knowing that there is also documentation on https://docs.yoctoproject.org/dev-manual/vulnerabilities.html#fixing-vulnerabilities-in-recipes? I'd also like to more details about the "Inappropriate" reasons. For example, I don't have a clue what "native" would mean. Why would be patch be rejected upstream because it only applies to the x86 host platform? Last but not least, we'd need to deprecate the "Accepted" reason and recommend "Backport" instead (see https://docs.yoctoproject.org/migration-guides/migration-3.2.html#miscellaneous-changes). I can work on this but your help would be much appreciated. Let me know :) Thanks Michael.
On Tue, 19 Sept 2023 at 16:07, Michael Opdenacker via lists.yoctoproject.org <michael.opdenacker=bootlin.com@lists.yoctoproject.org> wrote: > I'd also like to more details about the "Inappropriate" reasons. For > example, I don't have a clue what "native" would mean. Why would be > patch be rejected upstream because it only applies to the x86 host platform? For what it's worth I think the entire list of possible Inappropriate reasons in the original wiki and by extension in this patch is horrible, and should be completely deleted. The two valid reasons I can think of: - the issue is not yocto-specific, and should be fixed upstream but the patch in its current form is not suitable for merging upstream, and the author lacks sufficient expertise to developer a proper patch. Instead the issue is handled via a bug report (include link). This would be [upstream ticket <link>] with extended explanation in commit message. - the issue is specific to how yocto performs builds or sets things up at runtime, and can be resolved only with a patch that is not however relevant or appropriate for general upstream submission. This would be [oe specific] also with extended explanation in commit messages. Things like 'I'm not the author of the patch' or 'licensing fix' or whatnot aren't valid reasons to mark Inappropriate. We should also say clearly that adding Inappropriate patches is strongly discouraged, as they add to maintenance burden, and can't be allowed to grow indefinitely in their numbers. Hope this helps with a v2. Alex
On Tue, Sep 19, 2023 at 04:06:39PM +0200, Michael Opdenacker wrote: > Hi again Roland > > On 19.09.23 at 14:57, Roland Hieber wrote: > > Port and update the text from the OpenEmbedded wiki page [1], which has > > since been deleted and replaced by a link to the Contributor Guide > > (where not everything is available yet). The original text was available > > under Creative Commons Attribution-ShareAlike 3.0, and the original > > authors were wiki users Mhatle, Mariano, Benjamin, and McFrisk. > > > > [1]: https://www.openembedded.org/index.php?title=Commit_Patch_Message_Guidelines&oldid=10935 > > > > Signed-off-by: Roland Hieber <rhi@pengutronix.de> > > --- > > .../contributor-guide/recipe-style-guide.rst | 119 +++++++++++++++++++++ > > 1 file changed, 119 insertions(+) > > > Thanks for this update, but I told, I couldn't find your original > submissions, and since then, I already merged such documentation into > master-next. I resent them because apparently my first submission did not appear on the web archive (maybe because I was not yet subscribed to the group, but got auto-subscribed after the submission…?) and I wanted to make sure they made it through. > However, I see that the version on master-next > (https://docs.yoctoproject.org/next/) doesn't cover CVE tags yet, and your > patch did. Would you be interested in sending an update against master-next > if necessary, knowing that there is also documentation on https://docs.yoctoproject.org/dev-manual/vulnerabilities.html#fixing-vulnerabilities-in-recipes? Ah, I didn't see the master-next version yet! I'll rework my patches and send a new version for the one patch that was not applied yet. > I'd also like to more details about the "Inappropriate" reasons. For > example, I don't have a clue what "native" would mean. Why would be patch be > rejected upstream because it only applies to the x86 host platform? > > Last but not least, we'd need to deprecate the "Accepted" reason and > recommend "Backport" instead (see https://docs.yoctoproject.org/migration-guides/migration-3.2.html#miscellaneous-changes). Phew… I have the same questions as you… I only ported the existing text from the wiki… :D But I think what Alexander wrote in the other message in the thread seems reasonable, I'll keep that in mind for v2. > I can work on this but your help would be much appreciated. Let me know :) - Roland
Hi Roland On 20.09.23 at 10:56, Roland Hieber wrote: > I resent them because apparently my first submission did not appear on > the web archive (maybe because I was not yet subscribed to the group, > but got auto-subscribed after the submission…?) and I wanted to make > sure they made it through. > >> However, I see that the version on master-next >> (https://docs.yoctoproject.org/next/) doesn't cover CVE tags yet, and your >> patch did. Would you be interested in sending an update against master-next >> if necessary, knowing that there is also documentation on https://docs.yoctoproject.org/dev-manual/vulnerabilities.html#fixing-vulnerabilities-in-recipes? > Ah, I didn't see the master-next version yet! I'll rework my patches and > send a new version for the one patch that was not applied yet. Great, thanks! The changes are in master now, by the way :) > >> I'd also like to more details about the "Inappropriate" reasons. For >> example, I don't have a clue what "native" would mean. Why would be patch be >> rejected upstream because it only applies to the x86 host platform? >> >> Last but not least, we'd need to deprecate the "Accepted" reason and >> recommend "Backport" instead (see https://docs.yoctoproject.org/migration-guides/migration-3.2.html#miscellaneous-changes). > Phew… I have the same questions as you… I only ported the existing > text from the wiki… :D But I think what Alexander wrote in the other > message in the thread seems reasonable, I'll keep that in mind for v2. Perfect, thanks! Cheers Michael.
Hi Alex, Many thanks for your comments! On 19.09.23 at 16:23, Alexander Kanavin wrote: > On Tue, 19 Sept 2023 at 16:07, Michael Opdenacker via > lists.yoctoproject.org > <michael.opdenacker=bootlin.com@lists.yoctoproject.org> wrote: >> I'd also like to more details about the "Inappropriate" reasons. For >> example, I don't have a clue what "native" would mean. Why would be >> patch be rejected upstream because it only applies to the x86 host platform? > For what it's worth I think the entire list of possible Inappropriate > reasons in the original wiki and by extension in this patch is > horrible, and should be completely deleted. The two valid reasons I > can think of: > - the issue is not yocto-specific, and should be fixed upstream but > the patch in its current form is not suitable for merging upstream, > and the author lacks sufficient expertise to developer a proper patch. > Instead the issue is handled via a bug report (include link). This > would be [upstream ticket <link>] with extended explanation in commit > message. > - the issue is specific to how yocto performs builds or sets things up > at runtime, and can be resolved only with a patch that is not however > relevant or appropriate for general upstream submission. This would be > [oe specific] also with extended explanation in commit messages. > > Things like 'I'm not the author of the patch' or 'licensing fix' or > whatnot aren't valid reasons to mark Inappropriate. We should also say > clearly that adding Inappropriate patches is strongly discouraged, as > they add to maintenance burden, and can't be allowed to grow > indefinitely in their numbers. This definitely makes sense. We could propose documentation changes are the reason strings are not specified in the code. However, isn't this something we should discuss first with a wider audience, for example using the openembedded-core mailing list? Thanks again, Michael.
Hi again Roland On 20.09.23 at 10:56, Roland Hieber wrote: >> Last but not least, we'd need to deprecate the "Accepted" reason and >> recommend "Backport" instead (see https://docs.yoctoproject.org/migration-guides/migration-3.2.html#miscellaneous-changes). For this change, I think a separate commit would be better. We can actually remove "Accepted" from the list, as only "Backport" should be used. Thanks Michael.
On Wed, 20 Sept 2023 at 11:47, Michael Opdenacker <michael.opdenacker@bootlin.com> wrote: > This definitely makes sense. We could propose documentation changes are > the reason strings are not specified in the code. However, isn't this > something we should discuss first with a wider audience, for example > using the openembedded-core mailing list? I can't parse the second sentence :) We can notify about these changes outside of the docs list, but otherwise as long as the language makes room for other options (e.g. saying that there can be other reasons, but they must be clearly explained) I don't think this is particularly controversial. We should also explain that Pending is something that should not be misused as 'I can't be bother to submit upstream', and must have some genuine obstacle to immediate upstream submission - just need to find polite phrasing for that :) Alex
On 20.09.23 at 12:00, Alexander Kanavin wrote: > On Wed, 20 Sept 2023 at 11:47, Michael Opdenacker > <michael.opdenacker@bootlin.com> wrote: >> This definitely makes sense. We could propose documentation changes are >> the reason strings are not specified in the code. However, isn't this >> something we should discuss first with a wider audience, for example >> using the openembedded-core mailing list? > I can't parse the second sentence :) Oops, my first "are" was meant to be an "as". Sorry for the mind twisting! > We can notify about these changes outside of the docs list, but > otherwise as long as the language makes room for other options (e.g. > saying that there can be other reasons, but they must be clearly > explained) I don't think this is particularly controversial. Right, it makes sense! > > We should also explain that Pending is something that should not be > misused as 'I can't be bother to submit upstream', and must have some > genuine obstacle to immediate upstream submission - just need to find > polite phrasing for that :) I agree :) Thanks Michael.
On Wed, Sep 20, 2023 at 12:00:07PM +0200, Alexander Kanavin wrote: > On Wed, 20 Sept 2023 at 11:47, Michael Opdenacker > <michael.opdenacker@bootlin.com> wrote: > > This definitely makes sense. We could propose documentation changes are > > the reason strings are not specified in the code. However, isn't this > > something we should discuss first with a wider audience, for example > > using the openembedded-core mailing list? > > I can't parse the second sentence :) > We can notify about these changes outside of the docs list, but > otherwise as long as the language makes room for other options (e.g. > saying that there can be other reasons, but they must be clearly > explained) I don't think this is particularly controversial. > > We should also explain that Pending is something that should not be > misused as 'I can't be bother to submit upstream', and must have some > genuine obstacle to immediate upstream submission - just need to find > polite phrasing for that :) Ah – just saw this mail after having sent out the patch series already. Feel free to leave that one out for now if more discussion is needed. - Roland
On Wed, 20 Sept 2023 at 12:09, Roland Hieber <rhi@pengutronix.de> wrote: > > We can notify about these changes outside of the docs list, but > > otherwise as long as the language makes room for other options (e.g. > > saying that there can be other reasons, but they must be clearly > > explained) I don't think this is particularly controversial. > > > > We should also explain that Pending is something that should not be > > misused as 'I can't be bother to submit upstream', and must have some > > genuine obstacle to immediate upstream submission - just need to find > > polite phrasing for that :) > > Ah – just saw this mail after having sent out the patch series already. > Feel free to leave that one out for now if more discussion is needed.I If you can fold these two suggestions into the patch, I don't have any other feedback and it can go in from my POV :) Alex
diff --git a/documentation/contributor-guide/recipe-style-guide.rst b/documentation/contributor-guide/recipe-style-guide.rst index a0d513e8e9a4..d9e14d5d19a7 100644 --- a/documentation/contributor-guide/recipe-style-guide.rst +++ b/documentation/contributor-guide/recipe-style-guide.rst @@ -255,3 +255,122 @@ Tips and Guidelines for Writing Recipes - Use :term:`BBCLASSEXTEND` instead of creating separate recipes such as ``-native`` and ``-nativesdk`` ones, whenever possible. This avoids having to maintain multiple recipe files at the same time. + +.. _upstream-status: + +Patches +======= + +In order to keep track of patches applied by recipes and ultimately reduce the +number of patches that are required to be maintained, we need to track the +status of the patches that are created. In addition, we also want to track if +it's appropriate to get this particular patch into the upstream project. + +For each patch applied by a recipe, add an ``Upstream-Status:`` tag which +contains one of the following items. Be sure to include any URL to bug +tracking, mailing lists, commit IDs, or other reference material pertaining to +the patch. + +``Pending`` + No determination has been made yet or patch was not yet submitted to upstream. + +``Submitted [where]`` + Submitted to upstream, awaiting approval and/or review. Include where it was + submitted, such as the author, date, mailing list, pull request, etc. + +``Accepted [version]`` + Accepted in upstream, expect it to be removed at next update. + Include expected version info. + +``Backport [version]`` + Backported from a new upstream version. Include upstream version info. + +``Denied [reason]`` + Not accepted by upstream. Include reason in patch. + +``Inactive-Upstream [lastcommit: when (and/or) lastrelease: when]`` + The upstream is no longer available. + This typically means a defunct project where no activity has happened for a + long time – measured in years. To make that judgement, it is recommended to + look at not only when the last release happened, but also when the last + commit happened, and whether newly made bug reports and merge requests since + that time receive no reaction. It is also recommended to add any relevant + links to the commit message where the inactivity can be clearly seen. + +``Inappropriate [reason]`` + The patch is not appropriate for upstream. Include a brief reason on the same + line enclosed with ``[]``. Possible reason include: + + - ``not author``: You are not the author and do not intend to upstream this, + source must be listed in the comments + - ``native`` + - ``licensing`` + - ``configuration`` + - ``enable feature`` + - ``disable feature`` + - ``bugfix`` (add bug URL here) + - ``embedded specific`` + - ``other`` (give details in comments) + +The various *"Inappropriate [reason]"* status items are meant to indicate that +the person responsible for adding this patch to the system does not intend to +upstream the patch for a specific reason. Unless otherwise noted, another person +could submit this patch to the upstream – if they do, the status should be +changed to *"Submitted [where]"*, and an additional *Signed-off-by:* line +added to the patch by the person claiming responsibility for upstreaming. + +CVE patches +----------- + +In order to have a better control of vulnerabilities, patches that fix CVEs must +contain a *"CVE:"* tag. This tag list all CVEs fixed by the patch. If more than +one CVE is fixed, separate them using spaces. + +Examples +-------- + +Here's an example of a patch that has been submitted upstream:: + + rpm: Adjusted the foo setting in bar + + [RPM Ticket #65] -- http://rpm5.org/cvs/tktview?tn=65,5 + + The foo setting in bar was decreased from X to X-50% in order to + ensure we don't exhaust all system memory with foobar threads. + + Upstream-Status: Submitted [rpm5-devel@rpm5.org] + Signed-off-by: Joe Developer <joe.developer@example.com> + +A future commit can change the value to *"Accepted"* or *"Denied"* as +appropriate. + +Another example of a patch that is specific to OpenEmbedded:: + + Do not treat warnings as errors + + There are additional warnings found with musl which are + treated as errors and fails the build, we have more combinations + then upstream supports to handle + + Upstream-Status: Inappropriate [OE specific] + +Here's a patch that has been backported from a pull request:: + + include missing sys/file.h for LOCK_EX + + Upstream-Status: Backport [https://github.com/systemd/systemd/pull/28651] + +This should be the header of patch that fixes CVE-2015-8370 in GRUB2:: + + grub2: Fix CVE-2015-8370 + + [No upstream tracking] -- https://bugzilla.redhat.com/show_bug.cgi?id=1286966 + + Back to 28; Grub2 Authentication + + Two functions suffer from integer underflow fault; the grub_username_get() and grub_password_get()located in + grub-core/normal/auth.c and lib/crypto.c respectively. This can be exploited to obtain a Grub rescue shell. + + Upstream-Status: Accepted [http://git.savannah.gnu.org/cgit/grub.git/commit/?id=451d80e52d851432e109771bb8febafca7a5f1f2] + CVE: CVE-2015-8370 + Signed-off-by: Joe Developer <joe.developer@example.com>
Port and update the text from the OpenEmbedded wiki page [1], which has since been deleted and replaced by a link to the Contributor Guide (where not everything is available yet). The original text was available under Creative Commons Attribution-ShareAlike 3.0, and the original authors were wiki users Mhatle, Mariano, Benjamin, and McFrisk. [1]: https://www.openembedded.org/index.php?title=Commit_Patch_Message_Guidelines&oldid=10935 Signed-off-by: Roland Hieber <rhi@pengutronix.de> --- .../contributor-guide/recipe-style-guide.rst | 119 +++++++++++++++++++++ 1 file changed, 119 insertions(+)