From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by mx.groups.io with SMTP id smtpd.web09.6037.1582818803414892103 for ; Thu, 27 Feb 2020 07:53:23 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.88, mailfrom: bob.c.feng@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Feb 2020 07:53:23 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,492,1574150400"; d="scan'208";a="437072310" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga005.fm.intel.com with ESMTP; 27 Feb 2020 07:53:22 -0800 Received: from shsmsx606.ccr.corp.intel.com (10.109.6.216) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 27 Feb 2020 07:53:22 -0800 Received: from shsmsx601.ccr.corp.intel.com (10.109.6.141) by SHSMSX606.ccr.corp.intel.com (10.109.6.216) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Thu, 27 Feb 2020 23:53:20 +0800 Received: from shsmsx601.ccr.corp.intel.com ([10.109.6.141]) by SHSMSX601.ccr.corp.intel.com ([10.109.6.141]) with mapi id 15.01.1713.004; Thu, 27 Feb 2020 23:53:20 +0800 From: "Bob Feng" To: "devel@edk2.groups.io" , "lersek@redhat.com" , Andrew Fish , Leif Lindholm , "Kinney, Michael D" , "Gao, Liming" CC: Pierre Gondois Subject: Re: [edk2-devel] [Patch 1/1][edk2-stable202002]BaseTools: Fixed a regression issue in Makefile for incremental build Thread-Topic: [edk2-devel] [Patch 1/1][edk2-stable202002]BaseTools: Fixed a regression issue in Makefile for incremental build Thread-Index: AQHV7Wrj9VQ9lhL7IkClUa3Lv95Gw6gvGo5A Date: Thu, 27 Feb 2020 15:53:20 +0000 Message-ID: <32761feabc3847c6847b3ff908ce9014@intel.com> References: <20200227094705.25404-1-bob.c.feng@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiY2Q5OTViNzktNTVkYS00ZDBmLThkNTktODQxYTA3ODZiYzA3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiNmcrbXBBMjdQc2hIWU5zSG1TQ3lpd1RsT09aeCtVMHZPQ0VhXC93M2pPZzFhSVhvNUVwWG5jRmZ0dlltZjFEdUoifQ== dlp-version: 11.2.0.6 dlp-product: dlpe-windows x-ctpclassification: CTP_NT dlp-reaction: no-action x-originating-ip: [10.239.127.36] MIME-Version: 1.0 Return-Path: bob.c.feng@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Laszlo, Thanks for your comments. I add comments inline. Thanks, Bob -----Original Message----- From: devel@edk2.groups.io On Behalf Of Laszlo Erse= k Sent: Thursday, February 27, 2020 8:39 PM To: Feng, Bob C ; Andrew Fish ; Lei= f Lindholm ; Kinney, Michael D ; Gao, Liming Cc: devel@edk2.groups.io; Pierre Gondois Subject: Re: [edk2-devel] [Patch 1/1][edk2-stable202002]BaseTools: Fixed a= regression issue in Makefile for incremental build On 02/27/20 10:47, Bob Feng wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2563 >=20 > This patch is to fix a increametal build regression bug which happen=20 > when using nmake. That's introduced by 818283de3f6d. >=20 > If there is white space before !INCLUDE instruction, nmake will not=20 > process it. Source code's dependent header files are listed in=20 > ${deps_file} file, if it's not included successfully, nmake will not=20 > detect the change of those header file. >=20 > This patch has been verified in Windows with VS2015 and Linux with GCC5. > The header file add/modify/delete can trig the incremental build with th= is fix. > There is no impact on the clean build. >=20 > Cc: Andrew Fish > Cc: Laszlo Ersek > Cc: Leif Lindholm > Cc: Michael D Kinney > Cc: Pierre Gondois > Signed-off-by: Bob Feng > Reviewed-by: Liming Gao > Tested-by: Liming Gao > --- > .../Source/Python/AutoGen/IncludesAutoGen.py | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) >=20 > diff --git a/BaseTools/Source/Python/AutoGen/IncludesAutoGen.py=20 > b/BaseTools/Source/Python/AutoGen/IncludesAutoGen.py > index 0a6314266f45..720d93395aaf 100644 > --- a/BaseTools/Source/Python/AutoGen/IncludesAutoGen.py > +++ b/BaseTools/Source/Python/AutoGen/IncludesAutoGen.py > @@ -50,21 +50,21 @@ class IncludesAutoGen(): > MakePath =3D self.module_autogen.BuildOption.get('MAKE', {}).ge= t('PATH') > if not MakePath: > EdkLogger.error("build", PARAMETER_MISSING, Message=3D"No M= ake path available.") > elif "nmake" in MakePath: > _INCLUDE_DEPS_TEMPLATE =3D TemplateString(''' > - ${BEGIN} > - !IF EXIST(${deps_file}) > - !INCLUDE ${deps_file} > - !ENDIF > - ${END} > +${BEGIN} > +!IF EXIST(${deps_file}) > +!INCLUDE ${deps_file} > +!ENDIF > +${END} > ''') > else: > _INCLUDE_DEPS_TEMPLATE =3D TemplateString(''' > - ${BEGIN} > - -include ${deps_file} > - ${END} > +${BEGIN} > +-include ${deps_file} > +${END} > ''') > > try: > deps_include_str =3D _INCLUDE_DEPS_TEMPLATE.Replace(deps_fi= le) > except Exception as e: >=20 (1) I agree this should go into edk2-stable202002. Acked-by: Laszlo Ersek (2) Andrew, Leif, Mike: should we extend the hard freeze by a few days? (3) Bob, I'm sad that proper Bugzilla practices are not being followed. - Commit 818283de3f6d never referenced TianoCore#2481. If the submitter fo= rgets about adding such a reference, then it's the reviewer's / maintainer'= s responsibility to point it out! [Bob] I agree reviewer/maintainer should remind the submitter to add BZ.= =20 - In the same vein, when commit 818283de3f6d was pushed, TianoCore#2481 wa= s not closed. I've had to associate the BZ with the commit now (first: I had to figure out the right BZ), and then close the BZ. [Bob] I agree the BZ status should be update in time. I don't think BZ sta= tus update is the reviewer's/maintainer's responsibility, the BZ owner shou= ld be responsible for it. NOTE: GitHub.com Pull Requests would not help *at all* in the face of such= sloppiness; even on GitHub.com, people have to at least *name* issue numbe= rs in commit messages. - TianoCore#2563 (which tracks the regression) identifies *neither* the BZ= for which the regression was introduced (2481), *nor* the faulty commit (8= 18283de3f6d). You realize it's *completely useless* to file BZs with such n= egligence, right? It has no more information than "stuff broke, we need to = fix it" -- but ain't that the general state of things, at all times? Are yo= u only trying to fill a BZ quota? [Bob] I don't agree this comments.=20 I added the bug reproduce steps in BZ description. I think it's enough whe= n I submit a new BZ. I'll append the root cause and solution ( would be ju= st patch review link) in its comments when I update the BZ status later.=20 We found this critical bug in this afternoon (PRC time) and root cause and= created patch very quickly. I don't think that I did not update the BZ in = time is process violation. I think the necessary information was provided when the patch send out. Th= e bug description and reproduce steps are in BZ, root cause is in the patch= commit message, the solution is the patch itself, test result is in the co= mmit message. - TianoCore#2563 did not receive the Regression keyword. [Bob] Sorry, I missed this. - TianoCore#2563 was not moved to IN_PROGRESS status when the present patc= h got posted; nor did it receive a pointer to the patch in the list archive= . (Not even the *subject* was listed in the BZ.) There are no words. I've fixed up these workflow warts for you now, but come on now! [Bob] Thanks for doing this for me. (4) Liming, can you please post the following two tags in response to the = patch, on the list: Reviewed-by: Liming Gao Tested-by: Liming Gao There is no sign on the list that you have ever reviewed and/or tested thi= s patch, so Bob's including those tags in the commit message right off the = bat is unjustified. It's OK if you tested it off-list beforehand, but you s= till need to state that on the list yourself, after the patch has been post= ed. I don't have the slightest hope that GitHub.com PRs would fix any of this.= No tooling can fix lack of caring. Laszlo