[opensuse-packaging] Review policy
Hi, Should there be a general review policy for the obs users who have the maintainer rights for package/group. I guess there is one for the opensuse-review team maybe we should have it also for those who are accepting packages to the devel projects as well. The reason is I had a declination with a comment why I have not explained use of %ghost macro in a spec file, but a person who would have reviewed the rpm buildlogs for the package would realize the fact that rpmlint gives the following error. non-ghost-in-var-run (Badness: 1000) And the mentioning %ghost in changes file in IMHO is nonsense as the issue has nothing to the with the package but packaging itself and from the end users point of is just cosmetic. So maybe it is a good idea to have a document describing what is required/recommended in the #sr review Togan -- To unsubscribe, e-mail: opensuse-packaging+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-packaging+owner@opensuse.org
Am 20. März 2013 20:22:53 schrieb Togan Muftuoglu <toganm@opensuse.org>:
Hi,
Should there be a general review policy for the obs users who have the maintainer rights for package/group. I guess there is one for the opensuse-review team maybe we should have it also for those who are accepting packages to the devel projects as well.
In ms opinion it can't hurt if develprojects apply Factory review rules to some degree too. Since some reviewers are packagers too that aleeady happens, although it should be more forgiving than for Factory.
The reason is I had a declination with a comment why I have not explained use of %ghost macro in a spec file, but a person who would have reviewed the rpm buildlogs for the package would realize the fact that rpmlint gives the following error.
Who seriously looks at buildlogs when reviewing requests?
non-ghost-in-var-run (Badness: 1000)
And the mentioning %ghost in changes file in IMHO is nonsense as the issue has nothing to the with the package but packaging itself and from the end users point of is just cosmetic.
Depends on the exact case. In general, I'd say put it into the changes file and let the user decide. Also, reviewers look there for reasons why changes happen. Trying to read packagers minds is usually consuming too much time, so such requests may easily get a Factory decline. We have ~50-200 requests per day fighting for attention. Those with concise changes and clear documentation win. And I refuse to spend much time on the others :-)
So maybe it is a good idea to have a document describing what is required/recommended in the #sr review
I would prefer that we improve the Factory review documentation and use that as a recommendation. Less doubled work.
Togan -- To unsubscribe, e-mail: opensuse-packaging+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-packaging+owner@opensuse.org
-- To unsubscribe, e-mail: opensuse-packaging+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-packaging+owner@opensuse.org
On 03/21/2013 09:10 AM, Sascha Peilicke wrote:
In ms opinion it can't hurt if develprojects apply Factory review rules to some degree too. Since some reviewers are packagers too that aleeady happens, although it should be more forgiving than for Factory.
Agreed
The reason is I had a declination with a comment why I have not explained use of %ghost macro in a spec file, but a person who would have reviewed the rpm buildlogs for the package would realize the fact that rpmlint gives the following error.
Who seriously looks at buildlogs when reviewing requests?
I do and I would expect those who review requests do as well other wise why bother in the first place to generate a build log and rpmlint log. If the package doesnot build it is recommended to have a look at the build log. Well I would assume if it builds fine one still should look the logs and especially the rpmlint log
non-ghost-in-var-run (Badness: 1000)
And the mentioning %ghost in changes file in IMHO is nonsense as the issue has nothing to the with the package but packaging itself and from the end users point of is just cosmetic.
Depends on the exact case. In general, I'd say put it into the changes file and let the user decide. Also, reviewers look there for reasons why changes happen. Trying to read packagers minds is usually consuming too much time, so such requests may easily get a Factory decline. We have ~50-200 requests per day fighting for attention. Those with concise changes and clear documentation win. And I refuse to spend much time on the others :-)
As an end user I look the changes most of the time to see what is changed in the package, not in the packaging process. But as the review team's workload I can understand your point
I would prefer that we improve the Factory review documentation and use that as a recommendation. Less doubled work.
That is even better, can you point me the wiki page for this documentation Thanks Togan -- To unsubscribe, e-mail: opensuse-packaging+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-packaging+owner@opensuse.org
On 03/21/2013 09:50 AM, Togan Muftuoglu wrote:
On 03/21/2013 09:10 AM, Sascha Peilicke wrote:
In ms opinion it can't hurt if develprojects apply Factory review rules to some degree too. Since some reviewers are packagers too that aleeady happens, although it should be more forgiving than for Factory.
Agreed
The reason is I had a declination with a comment why I have not explained use of %ghost macro in a spec file, but a person who would have reviewed the rpm buildlogs for the package would realize the fact that rpmlint gives the following error.
Who seriously looks at buildlogs when reviewing requests?
I do and I would expect those who review requests do as well other wise why bother in the first place to generate a build log and rpmlint log. If the package doesnot build it is recommended to have a look at the build log. Well I would assume if it builds fine one still should look the logs and especially the rpmlint log
Well, we review packages building for multiple architectures and a plethora of distros. Of course I mostly care about openSUSE:Factory (i586,x86_64) but still. And yes, I look at the rpmlint log for that target too. And no, I don't look at any build-log. On a calm day, I review ~50 packages, I simply don't have the time and it makes no sense either. Packages that don't build are declined by 'factory-auto' already, packages that build are good enough. I doubt people would appreciate if we start commenting the fuzzy mess that build-logs are ;-)
non-ghost-in-var-run (Badness: 1000)
And the mentioning %ghost in changes file in IMHO is nonsense as the issue has nothing to the with the package but packaging itself and from the end users point of is just cosmetic.
Depends on the exact case. In general, I'd say put it into the changes file and let the user decide. Also, reviewers look there for reasons why changes happen. Trying to read packagers minds is usually consuming too much time, so such requests may easily get a Factory decline. We have ~50-200 requests per day fighting for attention. Those with concise changes and clear documentation win. And I refuse to spend much time on the others :-)
As an end user I look the changes most of the time to see what is changed in the package, not in the packaging process. But as the review team's workload I can understand your point.
You raised a valid point, it's always debatable what the user _really_ interests. Since only power-users seem to check RPM changelogs and everybody differs on appropriate content, I'd rather see some noise than miss some more important stuff. As an example, the whole UsrMerge topic was mostly a packaging issue. But it had quite some consequences on the user, so we documented that appropriately. Lastly, I tend to think that users wanting to now _only_ what changed in a particular upstream release will probably search the upstream homepage anyway.
I would prefer that we improve the Factory review documentation and use that as a recommendation. Less doubled work.
That is even better, can you point me the wiki page for this documentation
Thanks Togan
-- With kind regards, Sascha Peilicke SUSE Linux GmbH, Maxfeldstr. 5, D-90409 Nuernberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer HRB 16746 (AG Nürnberg) -- To unsubscribe, e-mail: opensuse-packaging+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-packaging+owner@opensuse.org
[setting Cc and Reply-To to the opensuse-buildservice list] Sascha Peilicke (speilicke@suse.com) wrote:
On 03/21/2013 09:50 AM, Togan Muftuoglu wrote:
On 03/21/2013 09:10 AM, Sascha Peilicke wrote:
The reason is I had a declination with a comment why I have not explained use of %ghost macro in a spec file, but a person who would have reviewed the rpm buildlogs for the package would realize the fact that rpmlint gives the following error.
Who seriously looks at buildlogs when reviewing requests?
I do and I would expect those who review requests do as well other wise why bother in the first place to generate a build log and rpmlint log. If the package doesnot build it is recommended to have a look at the build log. Well I would assume if it builds fine one still should look the logs and especially the rpmlint log
Well, we review packages building for multiple architectures and a plethora of distros. Of course I mostly care about openSUSE:Factory (i586,x86_64) but still. And yes, I look at the rpmlint log for that target too. And no, I don't look at any build-log. On a calm day, I review ~50 packages
8-O Wow, I had no idea any one person did that much work! AFAICS, the rpmlint results are only available in two places: - the build log - the Rpmlint Results tab on each package's Overview page Right? And neither of these are formatted in a very nice way.
I simply don't have the time and it makes no sense either. Packages that don't build are declined by 'factory-auto' already, packages that build are good enough. I doubt people would appreciate if we start commenting the fuzzy mess that build-logs are ;-)
Since I am not currently a regular reviewer, feel free to tell me if I'm talking rubbish, but it seems to me that there are some low-hanging fruit changes we could make here to make life much easier for reviewers, e.g.: - Change the Rpmlint Results tab to group warnings/errors by filter type, and just show the number of occurrences in each group. Then you can mouseover or click to see more details. - Add a way to view an rpmlint results summary via osc (is there an API call for this, or do you just have to grep the build log client-side?) - Automatically color the build log according to patterns. - Automatically split the build log into the different phases, e.g. - booting the VM - init_buildsystem etc. - %prep - %build - %install - /usr/lib/rpm/brp-* hooks - autoreqprov calculation - %clean - post-build checks (01-check-debuginfo ... 99-check-remove-rpms) - rpmlint - post-build tasks (e.g. "creating baselibs", "comparing built packages with former built") - VM power down and make each section expandable in the webUI with all sections starting collapsed and colour-coded red/green according to whether the section's contents matched any known failure patterns. (Actually I already have some code which might help implement this last idea.) Maybe an idea for the next hack week? If that makes sense to people then I can submit the idea as a proposal. -- To unsubscribe, e-mail: opensuse-packaging+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-packaging+owner@opensuse.org
On 03/21/2013 12:09 PM, Adam Spiers wrote:
[setting Cc and Reply-To to the opensuse-buildservice list]
Sascha Peilicke (speilicke@suse.com) wrote:
On 03/21/2013 09:50 AM, Togan Muftuoglu wrote:
On 03/21/2013 09:10 AM, Sascha Peilicke wrote:
The reason is I had a declination with a comment why I have not explained use of %ghost macro in a spec file, but a person who would have reviewed the rpm buildlogs for the package would realize the fact that rpmlint gives the following error.
Who seriously looks at buildlogs when reviewing requests?
I do and I would expect those who review requests do as well other wise why bother in the first place to generate a build log and rpmlint log. If the package doesnot build it is recommended to have a look at the build log. Well I would assume if it builds fine one still should look the logs and especially the rpmlint log
Well, we review packages building for multiple architectures and a plethora of distros. Of course I mostly care about openSUSE:Factory (i586,x86_64) but still. And yes, I look at the rpmlint log for that target too. And no, I don't look at any build-log. On a calm day, I review ~50 packages
8-O Wow, I had no idea any one person did that much work!
AFAICS, the rpmlint results are only available in two places:
- the build log - the Rpmlint Results tab on each package's Overview page
Right? And neither of these are formatted in a very nice way.
Somewhat true yes. The Results tab at least is colored. Back then I didn't find the screen space to put it next to something, so the grouping with "Build Results" is good enough IMO. Another good thing is that currently both have the same output (the webui simply copies it actually) to what "osc build" spits out. So it's easier to match back.
I simply don't have the time and it makes no sense either. Packages that don't build are declined by 'factory-auto' already, packages that build are good enough. I doubt people would appreciate if we start commenting the fuzzy mess that build-logs are ;-)
Since I am not currently a regular reviewer, feel free to tell me if I'm talking rubbish, but it seems to me that there are some low-hanging fruit changes we could make here to make life much easier for reviewers, e.g.:
- Change the Rpmlint Results tab to group warnings/errors by filter type, and just show the number of occurrences in each group. Then you can mouseover or click to see more details.
Could work, but in general I tried to reduce the number of clicks as much as possible since review time matters. Currently, it's one click to reach the rpmlint tab and maybe another to pick the distro you're interested in. @Henne: Maybe the distinction between distros can be released a bit, usually openSUSE:* are all the same to openSUSE:Factory. Only SLE_11_* rpmlint warnings differ, but they're maybe that outdated that we don't want to show them. Long story short, only showing Factory rpmlint issues should be enough for every request (even those not targeted at openSUSE:Factory). However, having collapsed boxes means more clicks and wouldn't like that there.
- Add a way to view an rpmlint results summary via osc (is there an API call for this, or do you just have to grep the build log client-side?)
- Automatically color the build log according to patterns.
Very useful and easily done via CodeMirror2.
- Automatically split the build log into the different phases, e.g.
- booting the VM - init_buildsystem etc. - %prep - %build - %install - /usr/lib/rpm/brp-* hooks - autoreqprov calculation - %clean - post-build checks (01-check-debuginfo ... 99-check-remove-rpms) - rpmlint - post-build tasks (e.g. "creating baselibs", "comparing built packages with former built") - VM power down
and make each section expandable in the webUI with all sections starting collapsed and colour-coded red/green according to whether the section's contents matched any known failure patterns. (Actually I already have some code which might help implement this last idea.)
I like the idea and that may reduce the scrolling needed. Collapsing sections here is that you usually search for something specific.
Maybe an idea for the next hack week? If that makes sense to people then I can submit the idea as a proposal.
Sure, please do that. I already have a number of topics for Hackweek I want to work on, but that doesn't look to difficult. @Henne: How about you? -- With kind regards, Sascha Peilicke SUSE Linux GmbH, Maxfeldstr. 5, D-90409 Nuernberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer HRB 16746 (AG Nürnberg) -- To unsubscribe, e-mail: opensuse-packaging+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-packaging+owner@opensuse.org
On 21.03.2013 12:47, Sascha Peilicke wrote:
On 03/21/2013 12:09 PM, Adam Spiers wrote:
Maybe an idea for the next hack week? If that makes sense to people then I can submit the idea as a proposal.
Sure, please do that. I already have a number of topics for Hackweek I want to work on, but that doesn't look to difficult.
@Henne: How about you?
I'm probably going to stay away from OBS during hackweek :) Henne -- Hendrik Vogelsang, Open Build Service Team SUSE LINUX GmbH, GF: Felix Imendörffer, HRB 21284 (AG Nürnberg) Maxfeldstr. 5, 90409 Nürnberg, Germany -- To unsubscribe, e-mail: opensuse-packaging+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-packaging+owner@opensuse.org
On Thu, 21 Mar 2013 12:47, Sascha Peilicke <speilicke@...> wrote:
On 03/21/2013 12:09 PM, Adam Spiers wrote:
[setting Cc and Reply-To to the opensuse-buildservice list] Sascha Peilicke (speilicke@suse.com) wrote:
On 03/21/2013 09:50 AM, Togan Muftuoglu wrote:
On 03/21/2013 09:10 AM, Sascha Peilicke wrote: [snip] Since I am not currently a regular reviewer, feel free to tell me if I'm talking rubbish, but it seems to me that there are some low-hanging fruit changes we could make here to make life much easier for reviewers, e.g.:
- Change the Rpmlint Results tab to group warnings/errors by filter type, and just show the number of occurrences in each group. Then you can mouseover or click to see more details.
Could work, but in general I tried to reduce the number of clicks as much as possible since review time matters. Currently, it's one click to reach the rpmlint tab and maybe another to pick the distro you're interested in.
How about: auto-collapse only for the non-failure sections. Pro: - Fast overview, - less scrolling to the failure-details, - no added click for failure-search Con: - added work during implementation.
@Henne: Maybe the distinction between distros can be released a bit, usually openSUSE:* are all the same to openSUSE:Factory. Only SLE_11_* rpmlint warnings differ, but they're maybe that outdated that we don't want to show them. Long story short, only showing Factory rpmlint issues should be enough for every request (even those not targeted at openSUSE:Factory).
However, having collapsed boxes means more clicks and wouldn't like that there.
- Add a way to view an rpmlint results summary via osc (is there an API call for this, or do you just have to grep the build log client-side?)
- Automatically color the build log according to patterns.
Very useful and easily done via CodeMirror2.
- Automatically split the build log into the different phases, e.g.
- booting the VM - init_buildsystem etc. - %prep - %build - %install - /usr/lib/rpm/brp-* hooks - autoreqprov calculation - %clean - post-build checks (01-check-debuginfo ... 99-check-remove-rpms) - rpmlint - post-build tasks (e.g. "creating baselibs", "comparing built packages with former built") - VM power down
and make each section expandable in the webUI with all sections starting collapsed and colour-coded red/green according to whether the section's contents matched any known failure patterns. (Actually I already have some code which might help implement this last idea.)
I like the idea and that may reduce the scrolling needed. Collapsing sections here is that you usually search for something specific.
See above: auto-collapse only for 'green' / error free sections. And, yes, re-use the same code as from above would definitely reduce the work for future maintenance. Thank you for taking this further. - Yamaban. -- To unsubscribe, e-mail: opensuse-packaging+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-packaging+owner@opensuse.org
Yamaban (foerster@lisas.de) wrote:
On Thu, 21 Mar 2013 12:47, Sascha Peilicke <speilicke@...> wrote:
On 03/21/2013 12:09 PM, Adam Spiers wrote:
[setting Cc and Reply-To to the opensuse-buildservice list] Sascha Peilicke (speilicke@suse.com) wrote:
On 03/21/2013 09:50 AM, Togan Muftuoglu wrote:
On 03/21/2013 09:10 AM, Sascha Peilicke wrote: [snip] Since I am not currently a regular reviewer, feel free to tell me if I'm talking rubbish, but it seems to me that there are some low-hanging fruit changes we could make here to make life much easier for reviewers, e.g.:
- Change the Rpmlint Results tab to group warnings/errors by filter type, and just show the number of occurrences in each group. Then you can mouseover or click to see more details.
Could work, but in general I tried to reduce the number of clicks as much as possible since review time matters. Currently, it's one click to reach the rpmlint tab and maybe another to pick the distro you're interested in.
How about: auto-collapse only for the non-failure sections. Pro: - Fast overview, - less scrolling to the failure-details, - no added click for failure-search Con: - added work during implementation.
Great idea! :-) -- To unsubscribe, e-mail: opensuse-packaging+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-packaging+owner@opensuse.org
On Wed, 20 Mar 2013, Togan Muftuoglu wrote:
Hi,
Should there be a general review policy for the obs users who have the maintainer rights for package/group. I guess there is one for the opensuse-review team maybe we should have it also for those who are accepting packages to the devel projects as well.
The reason is I had a declination with a comment why I have not explained use of %ghost macro in a spec file, but a person who would have reviewed the rpm buildlogs for the package would realize the fact that rpmlint gives the following error.
non-ghost-in-var-run (Badness: 1000)
And the mentioning %ghost in changes file in IMHO is nonsense as the issue has nothing to the with the package but packaging itself and from the end users point of is just cosmetic.
Eh? The .changes file is first and foremost about changes in _packaging_! Richard. -- Richard Biener <rguenther@suse.de> SUSE / SUSE Labs SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 GF: Jeff Hawn, Jennifer Guild, Felix Imend -- To unsubscribe, e-mail: opensuse-packaging+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-packaging+owner@opensuse.org
On 03/21/2013 09:49 AM, Richard Biener wrote:
On Wed, 20 Mar 2013, Togan Muftuoglu wrote:
The reason is I had a declination with a comment why I have not explained use of %ghost macro in a spec file, but a person who would have reviewed the rpm buildlogs for the package would realize the fact that rpmlint gives the following error.
non-ghost-in-var-run (Badness: 1000)
And the mentioning %ghost in changes file in IMHO is nonsense as the issue has nothing to the with the package but packaging itself and from the end users point of is just cosmetic.
Eh? The .changes file is first and foremost about changes in _packaging_!
Yes and no. If the changes affect the way the program behaves it should be included in the changes, but cosmetic things like fixing rpmlint errors or warnings (not mentioning the rpmlintrc file) that is cosmetic should not clutter the changes file. Though on a second thought maybe I should have mentioned it in the #sr message yet my expactation was /is one looks the rpmlint logs and understands the logic behind it. But it was pointed out not many people read the logs. Togan -- To unsubscribe, e-mail: opensuse-packaging+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-packaging+owner@opensuse.org
On Thu, 21 Mar 2013, Togan Muftuoglu wrote:
On 03/21/2013 09:49 AM, Richard Biener wrote:
On Wed, 20 Mar 2013, Togan Muftuoglu wrote:
The reason is I had a declination with a comment why I have not explained use of %ghost macro in a spec file, but a person who would have reviewed the rpm buildlogs for the package would realize the fact that rpmlint gives the following error.
non-ghost-in-var-run (Badness: 1000)
And the mentioning %ghost in changes file in IMHO is nonsense as the issue has nothing to the with the package but packaging itself and from the end users point of is just cosmetic.
Eh? The .changes file is first and foremost about changes in _packaging_!
Yes and no. If the changes affect the way the program behaves it should be included in the changes, but cosmetic things like fixing rpmlint errors or warnings (not mentioning the rpmlintrc file) that is cosmetic should not clutter the changes file.
Well, the .changes file is the _only_ place where changes in packaging can be documented. Changes in program behavior are documented in its own documentation, on web-pages in packaged README files, in the package description, etc. It's important to document packaging changes. And the .changes file is the place to do that!
Though on a second thought maybe I should have mentioned it in the #sr message yet my expactation was /is one looks the rpmlint logs and understands the logic behind it. But it was pointed out not many people read the logs.
Note that sr messages are not an appropriate way of documenting changes. Eventually the commit log history would be, but that get's seriously messed up and shortened with submits to factory (and of course with the default to just include .changes changes it's redundant as well). Richard. -- Richard Biener <rguenther@suse.de> SUSE / SUSE Labs SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 GF: Jeff Hawn, Jennifer Guild, Felix Imend -- To unsubscribe, e-mail: opensuse-packaging+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-packaging+owner@opensuse.org
On 03/21/2013 11:49 AM, Richard Biener wrote:
On Thu, 21 Mar 2013, Togan Muftuoglu wrote:
Yes and no. If the changes affect the way the program behaves it should be included in the changes, but cosmetic things like fixing rpmlint errors or warnings (not mentioning the rpmlintrc file) that is cosmetic should not clutter the changes file.
Well, the .changes file is the _only_ place where changes in packaging can be documented. Changes in program behavior are documented in its own documentation, on web-pages in packaged README files, in the package description, etc.
So in the light of this view you are saying it is OK to say for changes of the software look at the README/website as there are not vital for the package to be upgraded/updated; but how the package it build is VERY imprortant and has to be documented. This not seeing the forest for the trees, this is red tape IMO Togan PS .please keep replies to the list only. -- To unsubscribe, e-mail: opensuse-packaging+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-packaging+owner@opensuse.org
Hi, On Thu, 21 Mar 2013, Togan Muftuoglu wrote:
Well, the .changes file is the _only_ place where changes in packaging can be documented. Changes in program behavior are documented in its own documentation, on web-pages in packaged README files, in the package description, etc.
So in the light of this view you are saying it is OK to say for changes of the software look at the README/website as there are not vital for the package to be upgraded/updated; but how the package it build is VERY imprortant and has to be documented. This not seeing the forest for the trees, this is red tape IMO
Simply list both things, what users might interest as well as what packagers/reviewers might interest. No need to make a fuss about it. Ciao, Michael. -- To unsubscribe, e-mail: opensuse-packaging+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-packaging+owner@opensuse.org
participants (7)
-
Adam Spiers -
Henne Vogelsang -
Michael Matz -
Richard Biener -
Sascha Peilicke -
Togan Muftuoglu -
Yamaban