Message ID | 20200705214547.16485-1-mstamat@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | [FFmpeg-devel,v1,1/3] doc/developer.texi: Improvements in "Submitting patches" section. | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
On 2020-07-05 14:45, Manolis Stamatogiannakis wrote: > The section has been expanded to outline how to manage patch revisions. > --- > doc/developer.texi | 37 ++++++++++++++++++++++++++----------- > 1 file changed, 26 insertions(+), 11 deletions(-) Thank you for taking the time to improve the documentation about how to submit patches. Once these changes are accepted, you will have smoothed the way for many other developers. —Jim DeLaHunt, software engineer, Vancouver, Canada
Manolis Stamatogiannakis (12020-07-05): > The section has been expanded to outline how to manage patch revisions. > --- > doc/developer.texi | 37 ++++++++++++++++++++++++++----------- > 1 file changed, 26 insertions(+), 11 deletions(-) > > diff --git a/doc/developer.texi b/doc/developer.texi > index b33cab0fc7..dec27cb509 100644 > --- a/doc/developer.texi > +++ b/doc/developer.texi > @@ -491,18 +491,25 @@ as base64-encoded attachments, so your patch is not trashed during > transmission. Also ensure the correct mime type is used > (text/x-diff or text/x-patch or at least text/plain) and that only one > patch is inline or attached per mail. > -You can check @url{https://patchwork.ffmpeg.org}, if your patch does not show up, its mime type > -likely was wrong. > +You can check the most recently received patches on > +@url{https://patchwork.ffmpeg.org, patchwork}. > +If your patch does not show up, its mime type likely was wrong. > > -Your patch will be reviewed on the mailing list. You will likely be asked > +Your patch will be reviewed on the mailing list. Give us a few days to react. > +But if some time passes without reaction, send a reminder by email. > +Your patch should eventually be dealt with. However, you will likely be asked > to make some changes and are expected to send in an improved version that > incorporates the requests from the review. This process may go through > several iterations. Once your patch is deemed good enough, some developer > will pick it up and commit it to the official FFmpeg tree. > > -Give us a few days to react. But if some time passes without reaction, > -send a reminder by email. Your patch should eventually be dealt with. > - > +When preparing an updated version of you patch, make sure you increment > +its @emph{roll-counter}. This is achieved by adding a @code{-v <n>} argument > +to @code{git format-patch}/@code{git send-email} commands. I know many core developers do not bother with that, and I never found these "v3" very useful: if I am not involved in the discussion, I will not remember what number is the latest. And if I become involved in the discussions, my mail client can sort the patch by date and I take the latest. > +Additionally, it is recommended to register for a > +@uref{https://patchwork.ffmpeg.org, patchwork account}. > +This will allow you to mark previous version of your patches as "Superseded", > +and reduce the chance of someone spending time to review a stale patch. Is interacting with Patchwork to become mandatory when submitting patches? There is this classic scenario: 1. People work on something. 2. Somebody sets up a tool to make the work easier. 3. People start relying on the tool. 4. The tool proves imperfect. 5. People are required extra work to go around the flaws of the tool. 6. Overall, the tool has made the work not easier but harder. Are we going that way with Patchwork? If I am required to log into a website and do maintenance each time I send an updated patch, I will just send less updated patches. I am not alone in this. > > @chapter New codecs or formats checklist > > @@ -563,7 +570,19 @@ Did you make sure it compiles standalone, i.e. with > Does @code{make fate} pass with the patch applied? > > @item > -Was the patch generated with git format-patch or send-email? > +Was the patch generated with @code{git format-patch} or @code{git send-email}? > + > +@item > +If you are submitting a revised patch, did you increment the roll-counter > +with @code{-v <n>}? > + > +@item > +If you are submitting a revised patch, did you marked previous versions of > +the patch as "Superseded" on @uref{https://patchwork.ffmpeg.org/, patchwork}? > + > +@item > +Are you subscribed to ffmpeg-devel? > +(the list is subscribers only due to spam) > > @item > Did you sign-off your patch? (@code{git commit -s}) > @@ -576,10 +595,6 @@ Did you provide a clear git commit log message? > @item > Is the patch against latest FFmpeg git master branch? > > -@item > -Are you subscribed to ffmpeg-devel? > -(the list is subscribers only due to spam) > - > @item > Have you checked that the changes are minimal, so that the same cannot be > achieved with a smaller patch and/or simpler final code? Regards,
On 7/7/20, Nicolas George <george@nsup.org> wrote: > Manolis Stamatogiannakis (12020-07-05): >> The section has been expanded to outline how to manage patch revisions. >> --- >> doc/developer.texi | 37 ++++++++++++++++++++++++++----------- >> 1 file changed, 26 insertions(+), 11 deletions(-) >> >> diff --git a/doc/developer.texi b/doc/developer.texi >> index b33cab0fc7..dec27cb509 100644 >> --- a/doc/developer.texi >> +++ b/doc/developer.texi >> @@ -491,18 +491,25 @@ as base64-encoded attachments, so your patch is not >> trashed during >> transmission. Also ensure the correct mime type is used >> (text/x-diff or text/x-patch or at least text/plain) and that only one >> patch is inline or attached per mail. >> -You can check @url{https://patchwork.ffmpeg.org}, if your patch does not >> show up, its mime type >> -likely was wrong. >> +You can check the most recently received patches on >> +@url{https://patchwork.ffmpeg.org, patchwork}. >> +If your patch does not show up, its mime type likely was wrong. >> >> -Your patch will be reviewed on the mailing list. You will likely be >> asked >> +Your patch will be reviewed on the mailing list. Give us a few days to >> react. >> +But if some time passes without reaction, send a reminder by email. >> +Your patch should eventually be dealt with. However, you will likely be >> asked >> to make some changes and are expected to send in an improved version >> that >> incorporates the requests from the review. This process may go through >> several iterations. Once your patch is deemed good enough, some >> developer >> will pick it up and commit it to the official FFmpeg tree. >> >> -Give us a few days to react. But if some time passes without reaction, >> -send a reminder by email. Your patch should eventually be dealt with. >> - > >> +When preparing an updated version of you patch, make sure you increment >> +its @emph{roll-counter}. This is achieved by adding a @code{-v <n>} >> argument >> +to @code{git format-patch}/@code{git send-email} commands. > > I know many core developers do not bother with that, and I never found > these "v3" very useful: if I am not involved in the discussion, I will > not remember what number is the latest. And if I become involved in the > discussions, my mail client can sort the patch by date and I take the > latest. > >> +Additionally, it is recommended to register for a >> +@uref{https://patchwork.ffmpeg.org, patchwork account}. >> +This will allow you to mark previous version of your patches as >> "Superseded", >> +and reduce the chance of someone spending time to review a stale patch. > > Is interacting with Patchwork to become mandatory when submitting > patches? > > There is this classic scenario: > > 1. People work on something. > > 2. Somebody sets up a tool to make the work easier. > > 3. People start relying on the tool. > > 4. The tool proves imperfect. > > 5. People are required extra work to go around the flaws of the tool. > > 6. Overall, the tool has made the work not easier but harder. > > Are we going that way with Patchwork? > > If I am required to log into a website and do maintenance each time I > send an updated patch, I will just send less updated patches. I am not > alone in this. > >> >> @chapter New codecs or formats checklist >> >> @@ -563,7 +570,19 @@ Did you make sure it compiles standalone, i.e. with >> Does @code{make fate} pass with the patch applied? >> >> @item >> -Was the patch generated with git format-patch or send-email? >> +Was the patch generated with @code{git format-patch} or @code{git >> send-email}? >> + >> +@item >> +If you are submitting a revised patch, did you increment the >> roll-counter >> +with @code{-v <n>}? >> + >> +@item >> +If you are submitting a revised patch, did you marked previous versions >> of >> +the patch as "Superseded" on @uref{https://patchwork.ffmpeg.org/, >> patchwork}? >> + >> +@item >> +Are you subscribed to ffmpeg-devel? >> +(the list is subscribers only due to spam) >> >> @item >> Did you sign-off your patch? (@code{git commit -s}) >> @@ -576,10 +595,6 @@ Did you provide a clear git commit log message? >> @item >> Is the patch against latest FFmpeg git master branch? >> >> -@item >> -Are you subscribed to ffmpeg-devel? >> -(the list is subscribers only due to spam) >> - >> @item >> Have you checked that the changes are minimal, so that the same cannot >> be >> achieved with a smaller patch and/or simpler final code? > I remember days when I used patchwork and actually cared to keep it sync with reality (at least with my patches). But then it just stopped working - IIRC I was not able to log in and there my motivation for it was lost. Now that it reappeared again I'm not convinced it will stay for long so I just do not bother with it any more. > Regards, > > -- > Nicolas George >
On Tue, Jul 07, 2020 at 04:00:10PM +0200, Nicolas George wrote: > Manolis Stamatogiannakis (12020-07-05): > > The section has been expanded to outline how to manage patch revisions. > > --- > > doc/developer.texi | 37 ++++++++++++++++++++++++++----------- > > 1 file changed, 26 insertions(+), 11 deletions(-) > > > > diff --git a/doc/developer.texi b/doc/developer.texi > > index b33cab0fc7..dec27cb509 100644 > > --- a/doc/developer.texi > > +++ b/doc/developer.texi > > @@ -491,18 +491,25 @@ as base64-encoded attachments, so your patch is not trashed during > > transmission. Also ensure the correct mime type is used > > (text/x-diff or text/x-patch or at least text/plain) and that only one > > patch is inline or attached per mail. > > -You can check @url{https://patchwork.ffmpeg.org}, if your patch does not show up, its mime type > > -likely was wrong. > > +You can check the most recently received patches on > > +@url{https://patchwork.ffmpeg.org, patchwork}. > > +If your patch does not show up, its mime type likely was wrong. > > > > -Your patch will be reviewed on the mailing list. You will likely be asked > > +Your patch will be reviewed on the mailing list. Give us a few days to react. > > +But if some time passes without reaction, send a reminder by email. > > +Your patch should eventually be dealt with. However, you will likely be asked > > to make some changes and are expected to send in an improved version that > > incorporates the requests from the review. This process may go through > > several iterations. Once your patch is deemed good enough, some developer > > will pick it up and commit it to the official FFmpeg tree. > > > > -Give us a few days to react. But if some time passes without reaction, > > -send a reminder by email. Your patch should eventually be dealt with. > > - > > > +When preparing an updated version of you patch, make sure you increment > > +its @emph{roll-counter}. This is achieved by adding a @code{-v <n>} argument > > +to @code{git format-patch}/@code{git send-email} commands. > > I know many core developers do not bother with that, and I never found > these "v3" very useful: if I am not involved in the discussion, I will > not remember what number is the latest. And if I become involved in the > discussions, my mail client can sort the patch by date and I take the > latest. > > > +Additionally, it is recommended to register for a > > +@uref{https://patchwork.ffmpeg.org, patchwork account}. > > +This will allow you to mark previous version of your patches as "Superseded", > > +and reduce the chance of someone spending time to review a stale patch. > > Is interacting with Patchwork to become mandatory when submitting > patches? > > There is this classic scenario: > > 1. People work on something. > > 2. Somebody sets up a tool to make the work easier. > > 3. People start relying on the tool. > > 4. The tool proves imperfect. > > 5. People are required extra work to go around the flaws of the tool. > > 6. Overall, the tool has made the work not easier but harder. > > Are we going that way with Patchwork? > > If I am required to log into a website and do maintenance each time I > send an updated patch, I will just send less updated patches. I am not > alone in this. a lot of patchwork patch maintaince can be automated, ive written a script to do that. What this does is basically look at local git, fetch all patches from patchwork (and cache them locally) and then find stuff which is either invalid, superseeded, applied, ... but not marked correctly last it dumps the command line commands to the screen for a human to decide if they are correct and should be run as is or not. See: https://github.com/michaelni/patchwork-update-bot It worked fine until the server was updated or maybe the number of patches became too big. now it times out while fetching patches, the command line tool (pwclient) times out too (xmlrpclib.ProtocolError: <ProtocolError for patchwork.ffmpeg.org/xmlrpc/: 504 Gateway Time-out>) so the bug is not in patchwork-update-bot Someone has to investigate why this timeout happens and make it not timeout then run this automatically in regular intervalls thx [...]
On Tue, 7 Jul 2020 at 16:00, Nicolas George <george@nsup.org> wrote: > Manolis Stamatogiannakis (12020-07-05): > > The section has been expanded to outline how to manage patch revisions. > > --- > > doc/developer.texi | 37 ++++++++++++++++++++++++++----------- > > 1 file changed, 26 insertions(+), 11 deletions(-) > > > > diff --git a/doc/developer.texi b/doc/developer.texi > > index b33cab0fc7..dec27cb509 100644 > > --- a/doc/developer.texi > > +++ b/doc/developer.texi > > @@ -491,18 +491,25 @@ as base64-encoded attachments, so your patch is > not trashed during > > transmission. Also ensure the correct mime type is used > > (text/x-diff or text/x-patch or at least text/plain) and that only one > > patch is inline or attached per mail. > > -You can check @url{https://patchwork.ffmpeg.org}, if your patch does > not show up, its mime type > > -likely was wrong. > > +You can check the most recently received patches on > > +@url{https://patchwork.ffmpeg.org, patchwork}. > > +If your patch does not show up, its mime type likely was wrong. > > > > -Your patch will be reviewed on the mailing list. You will likely be > asked > > +Your patch will be reviewed on the mailing list. Give us a few days to > react. > > +But if some time passes without reaction, send a reminder by email. > > +Your patch should eventually be dealt with. However, you will likely be > asked > > to make some changes and are expected to send in an improved version > that > > incorporates the requests from the review. This process may go through > > several iterations. Once your patch is deemed good enough, some > developer > > will pick it up and commit it to the official FFmpeg tree. > > > > -Give us a few days to react. But if some time passes without reaction, > > -send a reminder by email. Your patch should eventually be dealt with. > > - > > > +When preparing an updated version of you patch, make sure you increment > > +its @emph{roll-counter}. This is achieved by adding a @code{-v <n>} > argument > > +to @code{git format-patch}/@code{git send-email} commands. > > I know many core developers do not bother with that, and I never found > these "v3" very useful: if I am not involved in the discussion, I will > not remember what number is the latest. And if I become involved in the > discussions, my mail client can sort the patch by date and I take the > latest. > The documentation aims to establish some good practices for people who want to start contributing to the project. By the patches arriving to the list, adding a roll counter seems to be an established good practice. So it should probably be included in the docs. But keep in mind that this is only a recommendation anyway. Maybe changing "make sure" to "should" would make that more clear. Core developers can stray off recommendations, since they are in position to make an informed decision when they are not relevant. In this case, you have a very well setup email-based workflow. Other people may not have that, and the roll counter may come handy. Would normalizing the verbs in developer.texi help to avoid confusion? Maybe it's just me, but when I'm reading any technical documents, I interpret the verbs according to RFC2119 to understand what is mandatory, what is recommended and what optional. > > > +Additionally, it is recommended to register for a > > +@uref{https://patchwork.ffmpeg.org, patchwork account}. > > +This will allow you to mark previous version of your patches as > "Superseded", > > +and reduce the chance of someone spending time to review a stale patch. > > Is interacting with Patchwork to become mandatory when submitting > patches? > Nicolas, why would you ever interpret "recommended" as mandatory? I think it's as clear as it gets that it is not mandatory. Again, this is a good practice which was not mentioned in the documentation. I have no attachment whatsoever to the tool, or intention to impose it to anyone. @Michael Niedermayer, since you seem to be the most involved with patchwork in the thread, what would be better for this? Keep the wording as a recommendation, or to move it outside the list as purely informational text? > There is this classic scenario: > > 1. People work on something. > > 2. Somebody sets up a tool to make the work easier. > > 3. People start relying on the tool. > > 4. The tool proves imperfect. > > 5. People are required extra work to go around the flaws of the tool. > > 6. Overall, the tool has made the work not easier but harder. > > Are we going that way with Patchwork? > So your recommendation is to not setup any tool ever, because people may start relying on it? Tools come and go. As long as use is optional and there's no data lock-in, I don't see why we should be wary of any tool that makes running the project smoother. > > If I am required to log into a website and do maintenance each time I > send an updated patch, I will just send less updated patches. You certainly don't need to do that each time. A weekly cleanup would probably be more than enough, and would let someone visiting patchwork to have a good picture on what people are working on. > I am not > alone in this. > > You probably aren't. But let other people speak for themselves. I'm happy to listen to their comments too. "I am not alone in this" sounds pretty divisive/passive aggressive. Best regards, Manolis
On Tue, 7 Jul 2020 at 19:14, Michael Niedermayer <michael@niedermayer.cc> wrote: > > a lot of patchwork patch maintaince can be automated, ive written a script > to do that. > What this does is basically look at local git, fetch all patches from > patchwork > (and cache them locally) > and then find stuff which is either invalid, superseeded, applied, ... but > not > marked correctly > last it dumps the command line commands to the screen for a human to decide > if they are correct and should be run as is or not. > See: > https://github.com/michaelni/patchwork-update-bot > > It worked fine until the server was updated or maybe the number of patches > became too big. > now it times out while fetching patches, the command line tool (pwclient) > times out too (xmlrpclib.ProtocolError: <ProtocolError for > patchwork.ffmpeg.org/xmlrpc/: 504 Gateway Time-out>) > so the bug is not in patchwork-update-bot > Someone has to investigate why this timeout happens and make it not timeout > then run this automatically in regular intervalls > > thx > > Since it's python, I can take a look, provided it doesn't need any special permissions. M.
On Thu, Jul 09, 2020 at 02:01:45AM +0200, Manolis Stamatogiannakis wrote: > On Tue, 7 Jul 2020 at 16:00, Nicolas George <george@nsup.org> wrote: > > > Manolis Stamatogiannakis (12020-07-05): [...] > > > +Additionally, it is recommended to register for a > > > +@uref{https://patchwork.ffmpeg.org, patchwork account}. > > > +This will allow you to mark previous version of your patches as > > "Superseded", > > > +and reduce the chance of someone spending time to review a stale patch. > > > > Is interacting with Patchwork to become mandatory when submitting > > patches? > > > > Nicolas, why would you ever interpret "recommended" as mandatory? I think > it's as clear as it gets that it is not mandatory. > > Again, this is a good practice which was not mentioned in the > documentation. I have no attachment whatsoever to the tool, or intention to > impose it to anyone. > > @Michael Niedermayer, since you seem to be the most involved with patchwork > in the thread, what would be better for this? Keep the wording as a > recommendation, or to move it outside the list as purely informational text? I think there are 2 aspects here First is a "mostly automatic patchwork with only people who want to play with it obtaining accounts and doing something with them" vs one where its recommanded to get accounts These are 2 different philosophies ;) Second is, time vs time Is the gain from manually working with patchwork saving us more time elsewhere while we achieve the same quality of code? and Third Can we not automate whatever we would do manually ? And describing the actual cases with examples what is wrong and needs manual update would be interresting. Are these caseses that are clear to human but unclear to a computer ? or we are just missing a line or 2 in the script ? or are they maybe unclear to humans too ? which could then lead a discussion on how to make that clear to humans first ... I think its needed to understand where automation fails to really discuss this all thx [...]
On Thu, 9 Jul 2020 at 22:39, Michael Niedermayer <michael@niedermayer.cc> wrote: > > > @Michael Niedermayer, since you seem to be the most involved with > patchwork > > in the thread, what would be better for this? Keep the wording as a > > recommendation, or to move it outside the list as purely informational > text? > > I think there are 2 aspects here > First is a "mostly automatic patchwork with only people who want to play > with > it obtaining accounts and doing something with them" vs one where its > recommanded to get accounts > These are 2 different philosophies ;) > > Second is, time vs time > Is the gain from manually working with patchwork saving us more time > elsewhere while > we achieve the same quality of code? > > and Third > Can we not automate whatever we would do manually ? > > And describing the actual cases with examples what is wrong and needs > manual > update would be interresting. > > Are these caseses that are clear to human but unclear to a computer ? > or we are just missing a line or 2 in the script ? > or are they maybe unclear to humans too ? which could then lead > a discussion on how to make that clear to humans first ... > > I think its needed to understand where automation fails to really discuss > this all > > thx > > Thanks for the input Michael. If you find those benefits are not clear-cut, I agree we should refrain from recommending to everyone to create a patchwork account. I'm not intimate enough with patchwork automation, so my intuition on the benefits of recommending manual patch management may be off. I've updated the patch to v2, presenting the registration of a patchwork account as an optional step that may come handy in some cases, but not generally required. If anyone can make a more informed proposal on that matter, I can incorporate it in the patch, while we're looking at it. Best regards, Manolis
diff --git a/doc/developer.texi b/doc/developer.texi index b33cab0fc7..dec27cb509 100644 --- a/doc/developer.texi +++ b/doc/developer.texi @@ -491,18 +491,25 @@ as base64-encoded attachments, so your patch is not trashed during transmission. Also ensure the correct mime type is used (text/x-diff or text/x-patch or at least text/plain) and that only one patch is inline or attached per mail. -You can check @url{https://patchwork.ffmpeg.org}, if your patch does not show up, its mime type -likely was wrong. +You can check the most recently received patches on +@url{https://patchwork.ffmpeg.org, patchwork}. +If your patch does not show up, its mime type likely was wrong. -Your patch will be reviewed on the mailing list. You will likely be asked +Your patch will be reviewed on the mailing list. Give us a few days to react. +But if some time passes without reaction, send a reminder by email. +Your patch should eventually be dealt with. However, you will likely be asked to make some changes and are expected to send in an improved version that incorporates the requests from the review. This process may go through several iterations. Once your patch is deemed good enough, some developer will pick it up and commit it to the official FFmpeg tree. -Give us a few days to react. But if some time passes without reaction, -send a reminder by email. Your patch should eventually be dealt with. - +When preparing an updated version of you patch, make sure you increment +its @emph{roll-counter}. This is achieved by adding a @code{-v <n>} argument +to @code{git format-patch}/@code{git send-email} commands. +Additionally, it is recommended to register for a +@uref{https://patchwork.ffmpeg.org, patchwork account}. +This will allow you to mark previous version of your patches as "Superseded", +and reduce the chance of someone spending time to review a stale patch. @chapter New codecs or formats checklist @@ -563,7 +570,19 @@ Did you make sure it compiles standalone, i.e. with Does @code{make fate} pass with the patch applied? @item -Was the patch generated with git format-patch or send-email? +Was the patch generated with @code{git format-patch} or @code{git send-email}? + +@item +If you are submitting a revised patch, did you increment the roll-counter +with @code{-v <n>}? + +@item +If you are submitting a revised patch, did you marked previous versions of +the patch as "Superseded" on @uref{https://patchwork.ffmpeg.org/, patchwork}? + +@item +Are you subscribed to ffmpeg-devel? +(the list is subscribers only due to spam) @item Did you sign-off your patch? (@code{git commit -s}) @@ -576,10 +595,6 @@ Did you provide a clear git commit log message? @item Is the patch against latest FFmpeg git master branch? -@item -Are you subscribed to ffmpeg-devel? -(the list is subscribers only due to spam) - @item Have you checked that the changes are minimal, so that the same cannot be achieved with a smaller patch and/or simpler final code?