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

["Laszlo Ersek" <lersek@redhat.com>]

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!

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

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!

(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