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

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

[Bob Feng]

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:
-- 
2.20.1.windows.1

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

[Laszlo Ersek]

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

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

[Laszlo Ersek]

On 02/27/20 13:39, Laszlo Ersek wrote:
> 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>

Before I forget: please update the subject line of this patch to
something that actually carries information. For example:

  BaseTools: remove invalid leading space in nmake rules/directives

I'm not familiar with NMAKE, so maybe "rules" and "directives" aren't
the right terms. Still, the subject should name both NMAKE and leading
space (or whitespace at least).

Laszlo

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

[Liming Gao]

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.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>
> 
[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.gao@intel.com>
Tested-by: Liming Gao <liming.gao@intel.com>

Thanks
Liming
> 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
> 
> 
> 

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

[Bob Feng]

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

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

[Laszlo Ersek]

On 02/27/20 16:53, Feng, Bob C wrote:

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

Agreed.

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

Yes, the patch explains the issue well. If the link had been in the BZ,
I wouldn't have complained (as much).

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

It was not clear that you ever intended to add the link to the BZ.

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

Yes. There was no link from the BZ to the patch however. And it wasn't
possible to determine whether you were going to add the link later. I
tend to add the link immediately after posting, so I don't forget. My
experience tell me that most patch submitters that don't add the link at
once forget for good.

Yes, it was a generalization, sorry about that.

One thing I do have to admit (because I brought up GitHub.com before) is
that, on GitHub.com, if you submit a pull request, and at least one of
the commit messages references an Issue (like your commit message here
references TianoCore#2563), then the issue automatically gets an update.
So in that regard GitHub.com does save some manual work.

Laszlo

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

[Bob Feng]

Laszlo, I agree BZ status should be update in time. You do a very good practices about that. I need to learn from you. And thank you that you have done a lot on BZ maintenance for me and other assignees. 

I think we need have a document to record these good practices on BZ maintenance so that people can clear to know what information should be published on BZ in each development stage.

Thanks,
Bob

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Friday, February 28, 2020 1:18 AM
To: Feng, Bob C <bob.c.feng@intel.com>; devel@edk2.groups.io; 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: 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 16:53, Feng, Bob C wrote:

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

Agreed.

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

Yes, the patch explains the issue well. If the link had been in the BZ, I wouldn't have complained (as much).

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

It was not clear that you ever intended to add the link to the BZ.

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

Yes. There was no link from the BZ to the patch however. And it wasn't possible to determine whether you were going to add the link later. I tend to add the link immediately after posting, so I don't forget. My experience tell me that most patch submitters that don't add the link at once forget for good.

Yes, it was a generalization, sorry about that.

One thing I do have to admit (because I brought up GitHub.com before) is that, on GitHub.com, if you submit a pull request, and at least one of the commit messages references an Issue (like your commit message here references TianoCore#2563), then the issue automatically gets an update.
So in that regard GitHub.com does save some manual work.

Laszlo