| Message ID | 20231122140811.1021807-1-lukas.funke-oss@weidmueller.com |
|---|---|
| Headers | show |
| Series | patch: reduce changes during patch refresh | expand |
On Wed, 2023-11-22 at 15:08 +0100, Lukas Funke wrote: > From: Lukas Funke <lukas.funke@weidmueller.com> > > The patch series aims to reduce the noise in patches created by devtools. Some > diffs are just introduced due to an update in the hash or in the diffstats. > These changes are not important to a reviewer. > > Stefan Herbrechtsmeier (2): > patch: extract patches without diffstats > patch: extract patches with all-zero hash > > meta/lib/oe/patch.py | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) I'm fairly sure but haven't conclusively proven this causes these failures: https://autobuilder.yoctoproject.org/typhoon/#/builders/86/builds/6095/steps/14/logs/stdio 2023-11-23 23:31:16,216 - oe-selftest - INFO - RESULTS - devtool.DevtoolUpdateTests.test_devtool_update_recipe_local_patch_gz: FAILED (65.60s) 2023-11-23 23:31:16,216 - oe-selftest - INFO - RESULTS - devtool.DevtoolUpdateTests.test_devtool_update_recipe_long_filename: FAILED (53.35s) 2023-11-23 23:31:16,216 - oe-selftest - INFO - RESULTS - devtool.DevtoolUpdateTests.test_devtool_update_recipe_with_gitignore: FAILED (44.11s) Cheers, Richard
Hi Richard, On 27.11.2023 14:45, Richard Purdie wrote: > On Wed, 2023-11-22 at 15:08 +0100, Lukas Funke wrote: >> From: Lukas Funke <lukas.funke@weidmueller.com> >> >> The patch series aims to reduce the noise in patches created by devtools. Some >> diffs are just introduced due to an update in the hash or in the diffstats. >> These changes are not important to a reviewer. >> >> Stefan Herbrechtsmeier (2): >> patch: extract patches without diffstats >> patch: extract patches with all-zero hash >> >> meta/lib/oe/patch.py | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) > > I'm fairly sure but haven't conclusively proven this causes these > failures: > > https://autobuilder.yoctoproject.org/typhoon/#/builders/86/builds/6095/steps/14/logs/stdio > > 2023-11-23 23:31:16,216 - oe-selftest - INFO - RESULTS - devtool.DevtoolUpdateTests.test_devtool_update_recipe_local_patch_gz: FAILED (65.60s) > 2023-11-23 23:31:16,216 - oe-selftest - INFO - RESULTS - devtool.DevtoolUpdateTests.test_devtool_update_recipe_long_filename: FAILED (53.35s) > 2023-11-23 23:31:16,216 - oe-selftest - INFO - RESULTS - devtool.DevtoolUpdateTests.test_devtool_update_recipe_with_gitignore: FAILED (44.11s) Unfortunately, the patch indeed causes the tests to fail. The problamatic lines are in the 'lib\devtool\standard.py': if len(firstlineitems) > 1 and len(firstlineitems[1]) == 40: if not firstlineitems[1] in changed_revs: continue When the commit-hash becomes all zero, there is no way for devtools to detect the change. I'm a little puzzled how to handle this situation. However, I still think the all zero hash is usefull since it will reduce the noise when developers review their changed in git(lab|hub). Maybe it would be for the best to remove the check? Best regards Lukas > > Cheers, > > Richard
On Mon, 4 Dec 2023 at 11:48, Lukas Funke <lukas.funke-oss@weidmueller.com> wrote: > Unfortunately, the patch indeed causes the tests to fail. The > problamatic lines are in the 'lib\devtool\standard.py': > > if len(firstlineitems) > 1 and len(firstlineitems[1]) == 40: > if not firstlineitems[1] in changed_revs: > continue > > When the commit-hash becomes all zero, there is no way for devtools to > detect the change. I'm a little puzzled how to handle this situation. > However, I still think the all zero hash is usefull since it will reduce > the noise when developers review their changed in git(lab|hub). Maybe it > would be for the best to remove the check? It would help if you provide the context for that code snippet, so we can see why is it there, and what is it trying to do, without having to go and separately study the code ourselves. Patch review is hard, please do not make it harder. Alex
Hi Alex, On 04.12.2023 11:54, Alexander Kanavin wrote: > On Mon, 4 Dec 2023 at 11:48, Lukas Funke > <lukas.funke-oss@weidmueller.com> wrote: >> Unfortunately, the patch indeed causes the tests to fail. The >> problamatic lines are in the 'lib\devtool\standard.py': >> >> if len(firstlineitems) > 1 and len(firstlineitems[1]) == 40: >> if not firstlineitems[1] in changed_revs: >> continue >> >> When the commit-hash becomes all zero, there is no way for devtools to >> detect the change. I'm a little puzzled how to handle this situation. >> However, I still think the all zero hash is usefull since it will reduce >> the noise when developers review their changed in git(lab|hub). Maybe it >> would be for the best to remove the check? > > It would help if you provide the context for that code snippet, so we > can see why is it there, and what is it trying to do, without having > to go and separately study the code ourselves. Patch review is hard, > please do not make it harder. You're probably right. A little context is always helpful. From the top: the proposed change is setting the commit-hash in exported patches to all-zero. When devtool is updating the recipes using 'devtool update-recipe <somerecipe>' it figures out (in '_get_patchset_revs()') which commits were changed during development (in the workspace). Now devtool known which commits actually contain changes. Subsequently devtool is exporting the patches with all-zero commit hash (in '_export_patches()', which contains our all-zero commit hash change). '_export_patches()' also compares the now exported commit-hash to the expected commit-hash which is an argument to the very same function. Since it they didn't match (see the code snippet), devtool thinks there is no change. Thus, the patch is ignored. If we want to export patches with all-zero commits we have to either remove this check or have to export the original commit id in the patch. The latter would again introduce noise. Best regards Lukas > > Alex
On Mon, 4 Dec 2023 at 13:48, Lukas Funke <lukas.funke-oss@weidmueller.com> wrote: > You're probably right. A little context is always helpful. > > From the top: the proposed change is setting the commit-hash in > exported patches to all-zero. > > When devtool is updating the recipes using 'devtool update-recipe > <somerecipe>' it figures out (in '_get_patchset_revs()') which commits > were changed during development (in the workspace). > Now devtool known which commits actually contain changes. Subsequently > devtool is exporting the patches with all-zero commit hash (in > '_export_patches()', which contains our all-zero commit hash change). > '_export_patches()' also compares the now exported commit-hash to the > expected commit-hash which is an argument to the very same function. > Since it they didn't match (see the code snippet), devtool thinks there > is no change. Thus, the patch is ignored. > > If we want to export patches with all-zero commits we have to either > remove this check or have to export the original commit id in the patch. > The latter would again introduce noise. I still don't entirely understand what is happening, but the logic of deciding whether any of the commits were changed should not rely on what is written into patchfiles, and rather operate directly from the git history. That should be fixed first. Alex
From: Lukas Funke <lukas.funke@weidmueller.com> The patch series aims to reduce the noise in patches created by devtools. Some diffs are just introduced due to an update in the hash or in the diffstats. These changes are not important to a reviewer. Stefan Herbrechtsmeier (2): patch: extract patches without diffstats patch: extract patches with all-zero hash meta/lib/oe/patch.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)