Re: [edk2-devel] [Patch 1/1][edk2-stable202002]BaseTools: Fixed a regression issue in Makefile for incremental build

["Bob Feng" <bob.c.feng@intel.com>]

Hi Laszlo,

Thanks for your comments. I add comments inline.

Thanks,
Bob

-----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.feng@intel.com>; Andrew Fish <afish@apple.com>; Leif Lindholm <leif@nuviainc.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
Cc: devel@edk2.groups.io; Pierre Gondois <pierre.gondois@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 <afish@apple.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Pierre Gondois <pierre.gondois@arm.com>
> Signed-off-by: Bob Feng <bob.c.feng@intel.com>
> Reviewed-by: Liming Gao <liming.gao@intel.com>
> Tested-by: Liming Gao <liming.gao@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 <lersek@redhat.com>

(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 forgets 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. 

- 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.
[Bob] I agree the BZ status should be update in time. I don't think BZ status update is the reviewer's/maintainer's responsibility, the BZ owner should 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 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?
[Bob] I don't agree this comments. 
I added the bug reproduce steps in BZ description. I think it's enough when I submit a new BZ.  I'll append the root cause and solution ( would be just patch review link) in its comments when I update the BZ status later. 
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. The 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 commit 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 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!
[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 <liming.gao@intel.com>
Tested-by: Liming Gao <liming.gao@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