| Message ID | 20230223062754.3162249-1-chee.yang.lee@intel.com |
|---|---|
| State | Accepted, archived |
| Commit | 237c1b66e5014123c1e5c3e78f9ab0357bcd62dc |
| Headers | show |
| Series | checklayer: check for patch file upstream status | expand |
I don't recall the upstream-status being added as a yocto compatibility requirement. Can someone point me to a discussion that I missed ? As everyone can recall, I wasn't on board with this being a default QA check and error, and I'm also not on board with it being a yocto compliance error. I realize it isn't being made an error in this patch, but I wanted to raise my concerns now, before someone throws that switch. Bruce On Thu, Feb 23, 2023 at 1:28 AM Lee Chee Yang <chee.yang.lee@intel.com> wrote: > > From: Chee Yang Lee <chee.yang.lee@intel.com> > > yocto-check-layer to check all .patch file in layer for > Upstream-status and list down all .patch file without Upstream-Status. > Since upstream-status is additional Yocto Compatible requirement, > set this test as expected failure for now so it wont fail final > result. > > [YOCTO #14642] > > Signed-off-by: Chee Yang Lee <chee.yang.lee@intel.com> > --- > scripts/lib/checklayer/cases/common.py | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/scripts/lib/checklayer/cases/common.py b/scripts/lib/checklayer/cases/common.py > index 722d3cf638..8fb37e175f 100644 > --- a/scripts/lib/checklayer/cases/common.py > +++ b/scripts/lib/checklayer/cases/common.py > @@ -72,6 +72,21 @@ class CommonCheckLayer(OECheckLayerTestCase): > self.tc.layer['name']) > self.fail('\n'.join(msg)) > > + @unittest.expectedFailure > + def test_patches_upstream_status(self): > + patches = [] > + for dirpath, dirs, files in os.walk(self.tc.layer['path']): > + for filename in files: > + if filename.endswith(".patch"): > + data = "" > + ppath = os.path.join(dirpath, filename) > + with open(ppath, 'r', encoding='utf-8', errors='ignore') as f: > + data = f.read() > + if not re.search(r'^Upstream-Status', data, flags=re.I + re.M): > + patches.append(ppath) > + self.assertEqual(len(patches), 0 , \ > + msg="Layer contains patches without upstream status:\n%s" % '\n'.join([str(patch) for patch in patches])) > + > def test_signatures(self): > if self.tc.layer['type'] == LayerType.SOFTWARE and \ > not self.tc.test_software_layer_signatures: > -- > 2.37.3 > > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#177599): https://lists.openembedded.org/g/openembedded-core/message/177599 > Mute This Topic: https://lists.openembedded.org/mt/97178225/1050810 > Group Owner: openembedded-core+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [bruce.ashfield@gmail.com] > -=-=-=-=-=-=-=-=-=-=-=- >
On Thu, 2023-02-23 at 08:33 -0500, Bruce Ashfield wrote: > I don't recall the upstream-status being added as a yocto > compatibility requirement. Can someone point me to a discussion that I > missed ? It isn't a requirement at this time and there isn't any recent discussion you've missed. The topic has come up now and again at various points, including with the YP TSC and I suspect that was why we have https://bugzilla.yoctoproject.org/show_bug.cgi?id=14642 from back in 2021. I think the view of the YP TSC was that we probably should warn about things like this from yocto-check-layer perspective as it is a best practise we want to raise the profile of. > As everyone can recall, I wasn't on board with this being a default QA > check and error, and I'm also not on board with it being a yocto > compliance error. > > I realize it isn't being made an error in this patch, but I wanted to > raise my concerns now, before someone throws that switch. Your concerns are definitely known! As I said at the time in the discussion, the TSC does intend to add some extra checks around QA issues to YP Compatible but no decision on which ones has been made yet, I think that action rests with me to make a proposal. My view is this one can remain a warning, I can't speak for the rest of the TSC. I do think having yocto-check-layer show more warnings about best practises if the direction things will move though even if it doesn't change the resulting compatibility status. There is an intent to improve "quality" over time so I can't promise this will never become an error but I don't have any plans to push this beyond a warning. As above, I can't speak for all TSC members though. Cheers, Richard
On Thu, Feb 23, 2023 at 8:48 AM Richard Purdie <richard.purdie@linuxfoundation.org> wrote: > > On Thu, 2023-02-23 at 08:33 -0500, Bruce Ashfield wrote: > > I don't recall the upstream-status being added as a yocto > > compatibility requirement. Can someone point me to a discussion that I > > missed ? > > It isn't a requirement at this time and there isn't any recent > discussion you've missed. > > The topic has come up now and again at various points, including with > the YP TSC and I suspect that was why we have > https://bugzilla.yoctoproject.org/show_bug.cgi?id=14642 from back in > 2021. I think the view of the YP TSC was that we probably should warn > about things like this from yocto-check-layer perspective as it is a > best practise we want to raise the profile of. > > > As everyone can recall, I wasn't on board with this being a default QA > > check and error, and I'm also not on board with it being a yocto > > compliance error. > > > > I realize it isn't being made an error in this patch, but I wanted to > > raise my concerns now, before someone throws that switch. > > Your concerns are definitely known! As I said at the time in the > discussion, the TSC does intend to add some extra checks around QA > issues to YP Compatible but no decision on which ones has been made > yet, I think that action rests with me to make a proposal. My view is > this one can remain a warning, I can't speak for the rest of the TSC. I > do think having yocto-check-layer show more warnings about best > practises if the direction things will move though even if it doesn't > change the resulting compatibility status. > > There is an intent to improve "quality" over time so I can't promise > this will never become an error but I don't have any plans to push this > beyond a warning. As above, I can't speak for all TSC members though. > Sounds good. Summary: Everything is under control, and I didn't manage to zone out and miss something while battling with the latest round of golang craziness. I've already gone through my layers and updated the formatting and put inappropriate on most of the patches (I may have missed some), so my argument isn't really an argument at this point. With the QA check being around now, and anyone being able to turn it on, patches without a status should be fairly rare. If it did become a compatibility requirement, I'd of course make the changes and stay compatible (maybe grumbling a bit at the same time ;)) Cheers, Bruce > Cheers, > > Richard > > > >
> -----Original Message----- > From: Richard Purdie <richard.purdie@linuxfoundation.org> > Sent: Thursday, February 23, 2023 9:49 PM > To: Bruce Ashfield <bruce.ashfield@gmail.com>; Lee, Chee Yang > <chee.yang.lee@intel.com> > Cc: openembedded-core@lists.openembedded.org > Subject: Re: [OE-core] [PATCH] checklayer: check for patch file upstream > status > > On Thu, 2023-02-23 at 08:33 -0500, Bruce Ashfield wrote: > > I don't recall the upstream-status being added as a yocto > > compatibility requirement. Can someone point me to a discussion that I > > missed ? > > It isn't a requirement at this time and there isn't any recent discussion you've > missed. Sorry that I misunderstand the discussion in the bug ticket. Would update the commit message to avoid confusion. > > The topic has come up now and again at various points, including with the YP > TSC and I suspect that was why we have > https://bugzilla.yoctoproject.org/show_bug.cgi?id=14642 from back in 2021. I > think the view of the YP TSC was that we probably should warn about things > like this from yocto-check-layer perspective as it is a best practise we want to > raise the profile of. > > > As everyone can recall, I wasn't on board with this being a default QA > > check and error, and I'm also not on board with it being a yocto > > compliance error. > > > > I realize it isn't being made an error in this patch, but I wanted to > > raise my concerns now, before someone throws that switch. > > Your concerns are definitely known! As I said at the time in the discussion, > the TSC does intend to add some extra checks around QA issues to YP > Compatible but no decision on which ones has been made yet, I think that > action rests with me to make a proposal. My view is this one can remain a > warning, I can't speak for the rest of the TSC. I do think having yocto-check- > layer show more warnings about best practises if the direction things will > move though even if it doesn't change the resulting compatibility status. > > There is an intent to improve "quality" over time so I can't promise this will > never become an error but I don't have any plans to push this beyond a > warning. As above, I can't speak for all TSC members though. > > Cheers, > > Richard > > >
diff --git a/scripts/lib/checklayer/cases/common.py b/scripts/lib/checklayer/cases/common.py index 722d3cf638..8fb37e175f 100644 --- a/scripts/lib/checklayer/cases/common.py +++ b/scripts/lib/checklayer/cases/common.py @@ -72,6 +72,21 @@ class CommonCheckLayer(OECheckLayerTestCase): self.tc.layer['name']) self.fail('\n'.join(msg)) + @unittest.expectedFailure + def test_patches_upstream_status(self): + patches = [] + for dirpath, dirs, files in os.walk(self.tc.layer['path']): + for filename in files: + if filename.endswith(".patch"): + data = "" + ppath = os.path.join(dirpath, filename) + with open(ppath, 'r', encoding='utf-8', errors='ignore') as f: + data = f.read() + if not re.search(r'^Upstream-Status', data, flags=re.I + re.M): + patches.append(ppath) + self.assertEqual(len(patches), 0 , \ + msg="Layer contains patches without upstream status:\n%s" % '\n'.join([str(patch) for patch in patches])) + def test_signatures(self): if self.tc.layer['type'] == LayerType.SOFTWARE and \ not self.tc.test_software_layer_signatures: