[opensuse-factory] Renumbering patches and the factory bot
My current request for hamster-time-tracker https://build.opensuse.org/request/show/798689 (needed to make the hamster GNOME extension work with GNOME 3.36) is repeatedly rejected by the factory bot with messages like this: "A patch (0002-Fix-disable-callback-gnome-shell-3.30- compatibility.patch) is being deleted without this removal being mentioned in the changelog." If you look at the changelog, you can see that the patches have been *renumbered* to avoid ambiguity, and that I've documented this in the changelog in detail. Can someone please override the bot? Or if not, can I get some guidance how to document this such that the bot accepts it? Martin -- Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 SUSE Software Solutions Germany GmbH HRB 36809, AG Nürnberg GF: Felix Imendörffer -- To unsubscribe, e-mail: opensuse-factory+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-factory+owner@opensuse.org
Hi, On Wed, 2020-04-29 at 13:31 +0000, Martin Wilck wrote:
My current request for hamster-time-tracker https://build.opensuse.org/request/show/798689 (needed to make the hamster GNOME extension work with GNOME 3.36) is repeatedly rejected by the factory bot with messages like this:
"A patch (0002-Fix-disable-callback-gnome-shell-3.30- compatibility.patch) is being deleted without this removal being mentioned in the changelog."
If you look at the changelog, you can see that the patches have been *renumbered* to avoid ambiguity, and that I've documented this in the changelog in detail.
The bhot is right - the policy says old and new name are mentioned. This is not given in your case (the number you refer to is not the PatchXYZ> number from the spec file, but is part of the patch file name) Cheers, Dominique
On Wednesday 2020-04-29 15:31, Martin Wilck wrote:
"A patch (0002-Fix-disable-callback-gnome-shell-3.30- compatibility.patch) is being deleted without this removal being mentioned in the changelog."
If you look at the changelog, you can see that the patches have been *renumbered*
That is still a different filename. And it is also the reason using numbers in filenames for ordering reasons are frowned upon. The only true order is determined by the %patch statements (including those possibly implicitly generated by %autopatch). -- To unsubscribe, e-mail: opensuse-factory+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-factory+owner@opensuse.org
On Wed, 2020-04-29 at 17:05 +0200, Jan Engelhardt wrote:
On Wednesday 2020-04-29 15:31, Martin Wilck wrote:
"A patch (0002-Fix-disable-callback-gnome-shell-3.30- compatibility.patch) is being deleted without this removal being mentioned in the changelog."
If you look at the changelog, you can see that the patches have been *renumbered*
That is still a different filename.
The way I did it was: * renumber patch: 0102-Fix-disable-callback-gnome-shell-3.30- compatibility.patch (was 0002) This was an abbreviation of "(was: 0002-Fix-disable-callback-gnome- shell-3.30-compatibility.patch)". Obvious for human readers, I thought. Not for the bot, of course. It's fixed now (I hope).
And it is also the reason using numbers in filenames for ordering reasons are frowned upon. The only true order is determined by the %patch statements (including those possibly implicitly generated by %autopatch).
It's the filename format used by "git format-patch", and ensures correct ordering of the generated patch files. I find it helpful if the %patch number and the number in the patch file name agree. That makes it easy to verify that patches are listed in the right order in the spec file. Martin -- Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 SUSE Software Solutions Germany GmbH HRB 36809, AG Nürnberg GF: Felix Imendörffer -- To unsubscribe, e-mail: opensuse-factory+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-factory+owner@opensuse.org
On Wednesday 2020-04-29 17:30, Martin Wilck wrote:
And it is also the reason using numbers in filenames for ordering reasons are frowned upon. The only true order is determined by the %patch statements (including those possibly implicitly generated by %autopatch).
It's the filename format used by "git format-patch", and ensures correct ordering of the generated patch files.
Indeed it is -- for git, and any "filesystem" tasks, which is to say, globbing. But that goes out the window the moment one adds PatchN: lines to a .spec file, unfortunate as it may be. Maybe you can open a feature request at the rpm bug tracker to the effect that, in the future, we could drop all the PatchN: lines and have them implicitly generated from a [0-9]*.patch wildcard. That would align nicely with %autopatch and decriminalize {renames for the sole purpose of reordering}. -- To unsubscribe, e-mail: opensuse-factory+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-factory+owner@opensuse.org
On Wed, 2020-04-29 at 18:56 +0200, Jan Engelhardt wrote:
On Wednesday 2020-04-29 17:30, Martin Wilck wrote:
And it is also the reason using numbers in filenames for ordering reasons are frowned upon. The only true order is determined by the %patch statements (including those possibly implicitly generated by %autopatch).
It's the filename format used by "git format-patch", and ensures correct ordering of the generated patch files.
Indeed it is -- for git, and any "filesystem" tasks, which is to say, globbing. But that goes out the window the moment one adds PatchN: lines to a .spec file, unfortunate as it may be.
Maybe you can open a feature request at the rpm bug tracker to the effect that, in the future, we could drop all the PatchN: lines and have them implicitly generated from a [0-9]*.patch wildcard. That would align nicely with %autopatch and decriminalize {renames for the sole purpose of reordering}.
Name: Dummy Version: 1.0 Release: 0 Source0: Source.tar.xz %patchlist 0001-Patch-Foo.patch 0002-Patch-Bar.patch 0003-Patch-doSomething.patch %prep %autosetup -p1 %build … Something like that you mean? That's supported since RPM 4.15 (they translate to patch0, patch1 and patch2 here.. so not based on the number in the patch file name, but based on the list it is. Also, I've seen I don't know how many packages where there were multiple 0001-FOO.patch patches; the Number means nothing. People check out master, write the patch, then do format-patch; git always starts numbering at 0001 in this case. Cheers, Dominique
On Wednesday 2020-04-29 19:14, Dominique Leuenberger / DimStar wrote:
Name: Dummy Version: 1.0 Release: 0 Source0: Source.tar.xz %patchlist 0001-Patch-Foo.patch 0002-Patch-Bar.patch 0003-Patch-doSomething.patch
%prep %autosetup -p1
%build …
Something like that you mean?
I envisioned getting completely rid of patchlists (both Patch15 and %patchlist): ... Source0: s.tar.xz Patch*: [0-9]*.patch # or even "Patch*: *.diff" -- whatever it is you want for your package %prep %autosetup -p1 -- To unsubscribe, e-mail: opensuse-factory+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-factory+owner@opensuse.org
Am 29.04.20 um 20:09 schrieb Jan Engelhardt:
... Source0: s.tar.xz Patch*: [0-9]*.patch # or even "Patch*: *.diff" -- whatever it is you want for your package
The kernel packages use patches.tar.xz and get away with it. Every package with more than a dozen patches is most likely easier to handle with that approach :) Greetings, Stephan -- Lighten up, just enjoy life, smile more, laugh more, and don't get so worked up about things. Kenneth Branagh -- To unsubscribe, e-mail: opensuse-factory+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-factory+owner@opensuse.org
Stephan Kulow <coolo@suse.de> writes:
Am 29.04.20 um 20:09 schrieb Jan Engelhardt:
... Source0: s.tar.xz Patch*: [0-9]*.patch # or even "Patch*: *.diff" -- whatever it is you want for your package
The kernel packages use patches.tar.xz and get away with it. Every package with more than a dozen patches is most likely easier to handle with that approach :)
Or the same "trick" that dracut uses: just put your patches as git commits into a fork on github (and rebase them on every new upstream release) and clone the repo via a _service file. Cheers, Dan -- Dan Čermák <dcermak@suse.com> Software Engineer Development tools SUSE Software Solutions Germany GmbH Maxfeldstr. 5 90409 Nuremberg Germany (HRB 36809, AG Nürnberg) Managing Director: Felix Imendörffer
On 4/30/20 7:05 AM, Stephan Kulow wrote:
Am 29.04.20 um 20:09 schrieb Jan Engelhardt:
Source0: s.tar.xz Patch*: [0-9]*.patch # or even "Patch*: *.diff" -- whatever it is you want for your package
The kernel packages use patches.tar.xz and get away with it. Every package with more than a dozen patches is most likely easier to handle with that approach :)
Isn't it much harder to review changes with that approach? Ciao, Michael. -- To unsubscribe, e-mail: opensuse-factory+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-factory+owner@opensuse.org
On Thu, 30 Apr 2020 10:02:20 +0200 Michael Ströder <michael@stroeder.com> wrote:
On 4/30/20 7:05 AM, Stephan Kulow wrote:
Am 29.04.20 um 20:09 schrieb Jan Engelhardt:
Source0: s.tar.xz Patch*: [0-9]*.patch # or even "Patch*: *.diff" -- whatever it is you want for your package
The kernel packages use patches.tar.xz and get away with it. Every package with more than a dozen patches is most likely easier to handle with that approach :)
Isn't it much harder to review changes with that approach?
Not really. First, these tarballs are typically generated from git which is easy to review in git. Second, OBS web UI can show diffs of files inside a tarball. Thanks Michal -- To unsubscribe, e-mail: opensuse-factory+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-factory+owner@opensuse.org
On Thu, Apr 30, 2020 at 10:02:20AM +0200, Michael Ströder wrote:
On 4/30/20 7:05 AM, Stephan Kulow wrote:
Am 29.04.20 um 20:09 schrieb Jan Engelhardt:
Source0: s.tar.xz Patch*: [0-9]*.patch # or even "Patch*: *.diff" -- whatever it is you want for your package
The kernel packages use patches.tar.xz and get away with it. Every package with more than a dozen patches is most likely easier to handle with that approach :)
Isn't it much harder to review changes with that approach?
Quite the contrary, I would say; git and tools around it are way better suited for tracking and reviewing changes than OBS and its tools. Michal -- To unsubscribe, e-mail: opensuse-factory+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-factory+owner@opensuse.org
On 4/30/20 10:54 AM, Michal Kubecek wrote:
On Thu, Apr 30, 2020 at 10:02:20AM +0200, Michael Ströder wrote:
On 4/30/20 7:05 AM, Stephan Kulow wrote:
Am 29.04.20 um 20:09 schrieb Jan Engelhardt:
Source0: s.tar.xz Patch*: [0-9]*.patch # or even "Patch*: *.diff" -- whatever it is you want for your package
The kernel packages use patches.tar.xz and get away with it. Every package with more than a dozen patches is most likely easier to handle with that approach :)
Isn't it much harder to review changes with that approach?
Quite the contrary, I would say; git and tools around it are way better suited for tracking and reviewing changes than OBS and its tools.
AFAIK for most packages there is no standard git integration in OBS. So I guess the kernel team uses extra resources not available for the simple OBS work-flow. That's ok for the kernel team. But for simple stuff? I saw fairly simple packages with 5-6 files stuffed into a tar.gz. This always causes an extra burden on maintaining them without adding real benefit. BTW: The most annoying issue is finding out whether patches are still valid/required. Often there are no references to why they were added. Ciao, Michael. -- To unsubscribe, e-mail: opensuse-factory+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-factory+owner@opensuse.org
On 4/30/20 10:54 AM, Michal Kubecek wrote:
On Thu, Apr 30, 2020 at 10:02:20AM +0200, Michael Ströder wrote:
On 4/30/20 7:05 AM, Stephan Kulow wrote:
Am 29.04.20 um 20:09 schrieb Jan Engelhardt:
Source0: s.tar.xz Patch*: [0-9]*.patch # or even "Patch*: *.diff" -- whatever it is you want for your package
The kernel packages use patches.tar.xz and get away with it. Every package with more than a dozen patches is most likely easier to handle with that approach :)
Isn't it much harder to review changes with that approach?
Quite the contrary, I would say; git and tools around it are way better suited for tracking and reviewing changes than OBS and its tools.
AFAIK for most packages there is no standard git integration in OBS. So I guess the kernel team uses extra resources not available for the simple OBS work-flow. That's ok for the kernel team. But for simple stuff? For simple stuff you probably want loose patches. There are packages
On Thu, 30 Apr 2020 13:20:44 +0200 Michael Ströder <michael@stroeder.com> wrote: like qemu or u-boot that use scripts to generate both the patches and the changelog entries. You can copy these scripts in your package to get the same.
I saw fairly simple packages with 5-6 files stuffed into a tar.gz. This always causes an extra burden on maintaining them without adding real benefit.
Depends on how they get there. If you can re-create the archive in a reproducible way then it's just packaging. At least it's obvious how to get the patch files. Imagine it was stuffed in an obscpio archive. And yes, some packagers may choose to use tar archives just to avoid the requirement to list the patches in changelog.
BTW: The most annoying issue is finding out whether patches are still valid/required. Often there are no references to why they were added.
That is completely unrelated to how they are packaged, however. The very reason why you are supposed to list patches in changelog is that it is difficult to tell from OBS alone when and why the patch was added. If you have patches in a tar archive and no way to re-create the archive reproducibly it should be a requirement to include some metadata in the patches I suppose. If the archive is created from git or similar VCS it should be possible to tell from the history there. Thanks Michal -- To unsubscribe, e-mail: opensuse-factory+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-factory+owner@opensuse.org
On Thu, 2020-04-30 at 10:02 +0200, Michael Ströder wrote:
On 4/30/20 7:05 AM, Stephan Kulow wrote:
The kernel packages use patches.tar.xz and get away with it. Every package with more than a dozen patches is most likely easier to handle with that approach :)
Isn't it much harder to review changes with that approach?
Are you saying that factory reviewers would look at every single patch and try to figure out if it's correct? In my experience, factory review is focused on formal aspects of rpm building, the spec file and the changelog. Which is fine on that level. Review of individual patches for given software packages are the package maintainer's task. Package maintainers are likely to work with git or some other VC system on their package anyway, and if they are, they are served better by the VC than by the patch listing in the spec file. Regards, Martin -- Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 SUSE Software Solutions Germany GmbH HRB 36809, AG Nürnberg GF: Felix Imendörffer -- To unsubscribe, e-mail: opensuse-factory+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-factory+owner@opensuse.org
On Thu, 2020-04-30 at 14:00 +0000, Martin Wilck wrote:
Are you saying that factory reviewers would look at every single patch and try to figure out if it's correct? In my experience, factory review is focused on formal aspects of rpm building, the spec file and the changelog. Which is fine on that level.
It's mostly on formal level, yet we also look at patches and every now and then even spot something obviously wrong. It's just a pair of eyes more looking at it.
Review of individual patches for given software packages are the package maintainer's task. Package maintainers are likely to work with git or some other VC system on their package anyway, and if they are, they are served better by the VC than by the patch listing in the spec file.
Considering that the review team every now and then still declines patches that are obviously wrong: what does that tell us about the maintainers? They're human. They can make errors. Luckily, this is not the common case. But having the patches visible does help. Cheers, Dominique
On Thu, 2020-04-30 at 16:11 +0200, Dominique Leuenberger / DimStar wrote:
Review of individual patches for given software packages are the package maintainer's task. Package maintainers are likely to work with git or some other VC system on their package anyway, and if they are, they are served better by the VC than by the patch listing in the spec file.
Considering that the review team every now and then still declines patches that are obviously wrong: what does that tell us about the maintainers? They're human. They can make errors. Luckily, this is not the common case. But having the patches visible does help.
I appreciate that the review team does this, but I'd never have expected that, and I believe it can by no means be a requirement for the distro-level review (in the sense that a reviewer could be reprimanded for having approved a package with a patch that others found "obviously wrong" later). I guess it all depends on the number and complexity of the patches. The kernel is one obvious, extreme example. Other packages, where we basically take upstream releases with just a few trivial patches, would be the opposite extreme. Regards, Martin -- Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 SUSE Software Solutions Germany GmbH HRB 36809, AG Nürnberg GF: Felix Imendörffer -- To unsubscribe, e-mail: opensuse-factory+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-factory+owner@opensuse.org
On 4/30/20 11:30 PM, Martin Wilck wrote:
On Thu, 2020-04-30 at 10:02 +0200, Michael Ströder wrote:
On 4/30/20 7:05 AM, Stephan Kulow wrote:
The kernel packages use patches.tar.xz and get away with it. Every package with more than a dozen patches is most likely easier to handle with that approach :)
Isn't it much harder to review changes with that approach?
Are you saying that factory reviewers would look at every single patch and try to figure out if it's correct? In my experience, factory review is focused on formal aspects of rpm building, the spec file and the changelog. Which is fine on that level.
I will atleast check the patch likely does what the changelog description and or patchlog description say they should, people make mistakes sometimes the wrong patch gets copied over and ends up as the wrong name and gets uploaded as the wrong thing. If a package only has one maintainer and they do the update and review occasionally they might miss it. This is why having the patches in obs makes life easier for our reviews. -- Simon Lees (Simotek) http://simotek.net Emergency Update Team keybase.io/simotek SUSE Linux Adelaide Australia, UTC+10:30 GPG Fingerprint: 5B87 DB9D 88DC F606 E489 CEC5 0922 C246 02F0 014B
Am Mittwoch, 29. April 2020, 17:30:28 CEST schrieb Martin Wilck:
The way I did it was:
* renumber patch: 0102-Fix-disable-callback-gnome-shell-3.30- compatibility.patch (was 0002)
This was an abbreviation of "(was: 0002-Fix-disable-callback-gnome- shell-3.30-compatibility.patch)". Obvious for human readers, I thought. Not for the bot, of course. It's fixed now (I hope).
Guess, you found the reason why some people do not respond well to the bot.. OTOH, imagine how hard an implementation could get to derive the real filenames from you've written. Even more so doing this in a non not contradictory fashion. Cheers, Pete -- To unsubscribe, e-mail: opensuse-factory+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-factory+owner@opensuse.org
On Wed, 2020-04-29 at 20:02 +0200, Hans-Peter Jansen wrote:
Guess, you found the reason why some people do not respond well to the bot..
OTOH, imagine how hard an implementation could get to derive the real filenames from you've written. Even more so doing this in a non not contradictory fashion.
GSoC project proposal: AI for the factory bot ... ? Martin -- Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 SUSE Software Solutions Germany GmbH HRB 36809, AG Nürnberg GF: Felix Imendörffer -- To unsubscribe, e-mail: opensuse-factory+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-factory+owner@opensuse.org
Am 29.04.20 um 17:30 schrieb Martin Wilck:
On Wed, 2020-04-29 at 17:05 +0200, Jan Engelhardt wrote:
On Wednesday 2020-04-29 15:31, Martin Wilck wrote:
"A patch (0002-Fix-disable-callback-gnome-shell-3.30- compatibility.patch) is being deleted without this removal being mentioned in the changelog."
If you look at the changelog, you can see that the patches have been *renumbered*
That is still a different filename.
The way I did it was:
* renumber patch: 0102-Fix-disable-callback-gnome-shell-3.30- compatibility.patch (was 0002)
Don't argue with that stupid bot. My advice: just use patch1.patch patch2.patch patch3.patch as name and never worry about renaming patches again ;-) -- Stefan Seyfried "For a successful technology, reality must take precedence over public relations, for nature cannot be fooled." -- Richard Feynman -- To unsubscribe, e-mail: opensuse-factory+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-factory+owner@opensuse.org
On Thu, 2020-04-30 at 05:20 +0200, Stefan Seyfried wrote:
Am 29.04.20 um 17:30 schrieb Martin Wilck:
On Wed, 2020-04-29 at 17:05 +0200, Jan Engelhardt wrote:
On Wednesday 2020-04-29 15:31, Martin Wilck wrote:
"A patch (0002-Fix-disable-callback-gnome-shell-3.30- compatibility.patch) is being deleted without this removal being mentioned in the changelog."
If you look at the changelog, you can see that the patches have been *renumbered*
That is still a different filename.
The way I did it was:
* renumber patch: 0102-Fix-disable-callback-gnome-shell-3.30- compatibility.patch (was 0002)
Don't argue with that stupid bot.
I'm still awaiting your rewrite of the bot Cheers, Dominique
Am 30.04.20 um 09:51 schrieb Dominique Leuenberger / DimStar:
On Thu, 2020-04-30 at 05:20 +0200, Stefan Seyfried wrote:>> Don't argue with that stupid bot.
I'm still awaiting your rewrite of the bot
You are right. s/bot/rule/. Getting rid of the rule would also obviate the need to fix the bot ;-) -- Stefan Seyfried "For a successful technology, reality must take precedence over public relations, for nature cannot be fooled." -- Richard Feynman -- To unsubscribe, e-mail: opensuse-factory+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-factory+owner@opensuse.org
Hello, Am Donnerstag, 30. April 2020, 05:20:38 CEST schrieb Stefan Seyfried:
Am 29.04.20 um 17:30 schrieb Martin Wilck:
On Wed, 2020-04-29 at 17:05 +0200, Jan Engelhardt wrote:
On Wednesday 2020-04-29 15:31, Martin Wilck wrote:
"A patch (0002-Fix-disable-callback-gnome-shell-3.30- compatibility.patch) is being deleted without this removal being mentioned in the changelog."
If you look at the changelog, you can see that the patches have been *renumbered*
That is still a different filename.
The way I did it was: * renumber patch: 0102-Fix-disable-callback-gnome-shell-3.30- compatibility.patch (was 0002)
Don't argue with that stupid bot.
Maybe it's time to make the bot slightly less stupid? Currently it works in a "reject early, reject often" way, which makes it terribly annoying - and one of the results is that a number of packages now have a patches.tar.gz which fools the bot, but makes contributions harder (unpack the tarball, change a patch, re-pack the tarball). The bot would be less annoying if it would reject only once (when creating a SR) - without rejecting reopened SRs (reopening basically translates to "I know what I'm doing.") And/or don't reject a SR if it contains a keyword, maybe something like !damnbot ;-) (No, I don't volunteer to adjust the bot ;-) Regards, Christian Boltz -- <dragotin> where is that lazy chicken btw? <suseROCKs> Ouch! <dragotin> oh - code of conduct violated? <dragotin> "It is not allowed to call henne a lazy chicken!" <suseROCKs> revised last week: It is required to call him that [from #opensuse-project] -- To unsubscribe, e-mail: opensuse-factory+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-factory+owner@opensuse.org
participants (12)
-
Christian Boltz -
Dan Čermák -
Dominique Leuenberger / DimStar -
Hans-Peter Jansen -
Jan Engelhardt -
Martin Wilck -
Michael Ströder -
Michal Kubecek -
Michal Suchánek -
Simon Lees -
Stefan Seyfried -
Stephan Kulow