Laszlo: Thanks for your quick response. I add my comments. Thanks Liming > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek > Sent: Thursday, February 27, 2020 8:39 PM > To: Feng, Bob C <bob.c.f...@intel.com>; Andrew Fish <af...@apple.com>; Leif > Lindholm <l...@nuviainc.com>; Kinney, Michael D > <michael.d.kin...@intel.com>; Gao, Liming <liming....@intel.com> > Cc: devel@edk2.groups.io; Pierre Gondois <pierre.gond...@arm.com> > 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=2563 > > > > This patch is to fix a increametal build regression bug > > which happen when using nmake. That's introduced by 818283de3f6d. > > > > If there is white space before !INCLUDE instruction, nmake will not > > process it. Source code's dependent header files are listed in > > ${deps_file} file, if it's not included successfully, nmake will > > not detect the change of those header file. > > > > 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 this > > fix. > > There is no impact on the clean build. > > > > Cc: Andrew Fish <af...@apple.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Cc: Leif Lindholm <l...@nuviainc.com> > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > > Cc: Pierre Gondois <pierre.gond...@arm.com> > > Signed-off-by: Bob Feng <bob.c.f...@intel.com> > > Reviewed-by: Liming Gao <liming....@intel.com> > > Tested-by: Liming Gao <liming....@intel.com> > > --- > > .../Source/Python/AutoGen/IncludesAutoGen.py | 16 ++++++++-------- > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/BaseTools/Source/Python/AutoGen/IncludesAutoGen.py > > 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 = self.module_autogen.BuildOption.get('MAKE', > > {}).get('PATH') > > if not MakePath: > > EdkLogger.error("build", PARAMETER_MISSING, Message="No Make > > path available.") > > elif "nmake" in MakePath: > > _INCLUDE_DEPS_TEMPLATE = 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 = TemplateString(''' > > - ${BEGIN} > > - -include ${deps_file} > > - ${END} > > +${BEGIN} > > +-include ${deps_file} > > +${END} > > ''') > > > > try: > > deps_include_str = _INCLUDE_DEPS_TEMPLATE.Replace(deps_file) > > except Exception as e: > > > > (1) I agree this should go into edk2-stable202002. > > Acked-by: Laszlo Ersek <ler...@redhat.com> > [Liming] I see this fix is to restore the original behavior before the commit 818283de3f6d for !INCLUDE style in Makefile. So, I think the fix risk is low. Because this regression causes the basic incremental build not work with VS nmake tool, I also prefer to add it into this stable tag.
> (2) Andrew, Leif, Mike: should we extend the hard freeze by a few days? > [Liming] I will summary all pending patch lists and status into patch list mail and collect the feedback. > (3) Bob, I'm sad that proper Bugzilla practices are not being followed. > > - Commit 818283de3f6d never referenced TianoCore#2481. If the submitter > forgets about adding such a reference, then it's the reviewer's / > maintainer's responsibility to point it out! > > - In the same vein, when commit 818283de3f6d was pushed, TianoCore#2481 > was 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. [Liming] Thank your help. I agree that Maintainer and reviewer should reminder the submitter to follow BZ process. > > 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 numbers in commit messages. > > - TianoCore#2563 (which tracks the regression) identifies *neither* the > BZ for which the regression was introduced (2481), *nor* the faulty > commit (818283de3f6d). You realize it's *completely useless* to file BZs > with such negligence, 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 you only trying to fill a BZ quota? > > - TianoCore#2563 did not receive the Regression keyword. > > - TianoCore#2563 was not moved to IN_PROGRESS status when the present > patch 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! > [Liming] Thanks for your information. Now, we have no detail wiki page on how to use report issue and update status. I would like to update wiki page https://github.com/tianocore/tianocore.github.io/wiki/Reporting-Issues. > (4) Liming, can you please post the following two tags in response to > the patch, on the list: > [Liming] Sure. With the commit message is update, I give Reviewed-by: Liming Gao <liming....@intel.com> Tested-by: Liming Gao <liming....@intel.com> Thanks Liming > Reviewed-by: Liming Gao <liming....@intel.com> > Tested-by: Liming Gao <liming....@intel.com> > > There is no sign on the list that you have ever reviewed and/or tested > this 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 still need to state that on the list yourself, after > the patch has been posted. > > I don't have the slightest hope that GitHub.com PRs would fix any of > this. No tooling can fix lack of caring. > > Laszlo > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#55002): https://edk2.groups.io/g/devel/message/55002 Mute This Topic: https://groups.io/mt/71583896/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
