BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1875

In order to improve the build performance, we implemented
multiple-processes AutoGen. This change will reduce 20% time
for AutoGen phase.

The design document can be got from:
https://edk2.groups.io/g/devel/files/Designs/2019/0627/Multiple-thread-AutoGen.pdf

This patch serial pass the build of Ovmf, MinKabylake, MinPurley, packages
under Edk2 repository and intel client and server platforms.

V8:
1. Fixed a regression issue that AsbuildInf files are not generated.
That was introduced by V7.

V7:
1. Fixed regression issue for build -m, build modules, build libraries.
implement this fix in patch 05/10.

2. Fixed the regression issue for duplicat items in PcdTokenNumber file.
implement this fix in patch 02/10.

V6:

This version fixed a regression issue on incremental build.
The change is in patch 04/10

In the autogen sub-process, we need to use 
PlatformInfo.Pcds rather than PlatformInfo.Platform.Pcds
because we don't want to re-evaluate the Pcds in
the autogen sub-process.

V5:
Restructure the patches of V4. Squash 11/11 patch of V4 into V5 4/10,
5/10 and 10/10.

The details of the changes are as following:

1. Update Log queue maxsize as the value of thread number * 10.
This change is implemented in patch 10/10

2. Fixed the regression issue that the PCDs from build command line
are not passed into autogen sub-process correctly.

3. Fixed the regression issue that the exception raised inside
autogen sub-process is not well handled, including code exception
and Ctrl+C keyboard interrupt.

The above 2 changes are implemented in patch 5/10

4. Fixed the regression issue that multiple builds of the same 
driver/application with FILE_GUID overrides is not handle correctly.

5. Fixed the regression issue that shared fixed PCD between lib
and module is not handle correctly, which cause the autogen.c and 
autogen.h are different from orignal.

The above 2 changes are implemented in patch 4/10

V4:
Add one more patch 11/11 to enhance this feature. 1-10 are the same as V3
1. Set Log queue maxsize as thread number * 10
2. enhance ModuleUniqueBaseName function
3. fix bugs of build option pcd in sub Process
4. enhance error handling. Handle the exception of
KeyboardInterrup and exceptions happen in subprocess.
5. fix the issue of shared fixed pcd between module and lib.
6. fix bug in the function of duplicate modules handling.
V3:
1. Fixed incremental build issue.
2. Set AutoGen worker number to be align with "-n THREADNUMBER"
3. Enable block log queue.
V2: 
1. The first version missed autogen related commit 
from e812a812c1a0800c49e11507cb46222351520cc7. V2 add those commit
back.
2. Move CreateAsBuildInf into AutoGenWorker process
3. Save GlobalVar_<platform guid>_<arch>.bin to build folder.
4. Regenerate patches based on master bb824f685d
Feng, Bob C (10):
  BaseTools: Singleton the object to handle build conf file
  BaseTools: Split WorkspaceAutoGen._InitWorker into multiple functions
  BaseTools: Add functions to get platform scope build options
  BaseTools: Decouple AutoGen Objects
  BaseTools: Enable Multiple Process AutoGen
  BaseTools: Add shared data for processes
  BaseTools: Add LogAgent to support multiple process Autogen
  BaseTools: Move BuildOption parser out of build.py
  BaseTools: Add the support for python 2
  BaseTools: Enable block queue log agent.

 BaseTools/Source/Python/AutoGen/AutoGen.py    | 4227 +----------------
 .../Source/Python/AutoGen/AutoGenWorker.py    |  259 +
 .../Source/Python/AutoGen/BuildEngine.py      |   22 +
 BaseTools/Source/Python/AutoGen/DataPipe.py   |  160 +
 BaseTools/Source/Python/AutoGen/GenC.py       |    6 +-
 .../Source/Python/AutoGen/ModuleAutoGen.py    | 1908 ++++++++
 .../Python/AutoGen/ModuleAutoGenHelper.py     |  619 +++
 .../Source/Python/AutoGen/PlatformAutoGen.py  | 1511 ++++++
 .../Source/Python/AutoGen/WorkspaceAutoGen.py |  906 ++++
 BaseTools/Source/Python/Common/EdkLogger.py   |  119 +-
 BaseTools/Source/Python/Common/Misc.py        |    1 -
 .../Python/Common/TargetTxtClassObject.py     |   28 +-
 .../Python/Common/ToolDefClassObject.py       |    6 +-
 BaseTools/Source/Python/GenFds/GenFds.py      |    4 +-
 .../Python/GenFds/GenFdsGlobalVariable.py     |   54 +-
 .../Python/PatchPcdValue/PatchPcdValue.py     |    1 -
 .../Source/Python/Workspace/DscBuildData.py   |   38 +-
 .../Source/Python/Workspace/InfBuildData.py   |   39 +
 .../Python/Workspace/WorkspaceCommon.py       |    4 +
 .../Python/Workspace/WorkspaceDatabase.py     |    3 +
 BaseTools/Source/Python/build/BuildReport.py  |    4 +-
 BaseTools/Source/Python/build/build.py        |  377 +-
 BaseTools/Source/Python/build/buildoptions.py |   92 +
 23 files changed, 5932 insertions(+), 4456 deletions(-)
 create mode 100644 BaseTools/Source/Python/AutoGen/AutoGenWorker.py
 create mode 100644 BaseTools/Source/Python/AutoGen/DataPipe.py
 create mode 100644 BaseTools/Source/Python/AutoGen/ModuleAutoGen.py
 create mode 100644 BaseTools/Source/Python/AutoGen/ModuleAutoGenHelper.py
 create mode 100644 BaseTools/Source/Python/AutoGen/PlatformAutoGen.py
 create mode 100644 BaseTools/Source/Python/AutoGen/WorkspaceAutoGen.py
 create mode 100644 BaseTools/Source/Python/build/buildoptions.py

-- 
2.20.1.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44970): https://edk2.groups.io/g/devel/message/44970
Mute This Topic: https://groups.io/mt/32779325/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

(+ Andrew, Leif, Mike; Liming)

On 08/07/19 06:25, Bob Feng wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1875
>
> In order to improve the build performance, we implemented
> multiple-processes AutoGen. This change will reduce 20% time
> for AutoGen phase.
>
> The design document can be got from:
> https://edk2.groups.io/g/devel/files/Designs/2019/0627/Multiple-thread-AutoGen.pdf
>
> This patch serial pass the build of Ovmf, MinKabylake, MinPurley,
> packages under Edk2 repository and intel client and server platforms.
>
> V8:
> 1. Fixed a regression issue that AsbuildInf files are not generated.
> That was introduced by V7.
>
> V7:
> 1. Fixed regression issue for build -m, build modules, build
> libraries. implement this fix in patch 05/10.
>
> 2. Fixed the regression issue for duplicat items in PcdTokenNumber
> file. implement this fix in patch 02/10.
>
> V6:
>
> This version fixed a regression issue on incremental build.
> The change is in patch 04/10
>
> In the autogen sub-process, we need to use
> PlatformInfo.Pcds rather than PlatformInfo.Platform.Pcds
> because we don't want to re-evaluate the Pcds in
> the autogen sub-process.
>
> V5:
> Restructure the patches of V4. Squash 11/11 patch of V4 into V5 4/10,
> 5/10 and 10/10.
>
> The details of the changes are as following:
>
> 1. Update Log queue maxsize as the value of thread number * 10.
> This change is implemented in patch 10/10
>
> 2. Fixed the regression issue that the PCDs from build command line
> are not passed into autogen sub-process correctly.
>
> 3. Fixed the regression issue that the exception raised inside
> autogen sub-process is not well handled, including code exception
> and Ctrl+C keyboard interrupt.
>
> The above 2 changes are implemented in patch 5/10
>
> 4. Fixed the regression issue that multiple builds of the same
> driver/application with FILE_GUID overrides is not handle correctly.
>
> 5. Fixed the regression issue that shared fixed PCD between lib
> and module is not handle correctly, which cause the autogen.c and
> autogen.h are different from orignal.
>
> The above 2 changes are implemented in patch 4/10
>

Thank you for the above explanation!


(1) In my clone of the QEMU repository (master @ v4.1.0-rc4), I advanced
the roms/edk2 submodule to commit 96603b4f02b9 ("BaseTools/PatchCheck:
Disable text conversion in 'git show'", 2019-08-07), and applied your
patches on top. Then I ran

$ nice make -j4 -C roms efi

in the QEMU project root.

This build forces the usage of Python2, due to
<https://bugzilla.tianocore.org/show_bug.cgi?id=1607>.

The build worked fine, Ctrl-S / Ctrl-Q worked fine, and the resultant
firmware binaries all worked fine. All good.


(2) QEMU carries a simpe UEFI application that exposes SMBIOS anchor
addresses, and ACPI RSD PTR addresses, from the UEFI guest to some QEMU
unit tests that run on the host. In the same environment as (1), I
rebuilt this application:

$ nice make -j4 -C tests/uefi-test-tools

This command uses the "-m" switch of "build", internally. It also forces
Python2, due to <https://bugzilla.tianocore.org/show_bug.cgi?id=1607>.

The build completed fine, and I tested the AARCH64 and X64 binaries
(bootable ISO images). They worked fine. Good.


(3) In my normal edk2 clone, I cleaned the tree, applied your patches
(again on top of commit 96603b4f02b9), and started a build:

$ . edksetup.sh
$ nice make -C "$EDK_TOOLS_PATH" -j $(getconf _NPROCESSORS_ONLN)
$ nice -n 19 build \
    -a IA32 \
    -p OvmfPkg/OvmfPkgIa32.dsc \
    -t GCC48 \
    -b NOOPT \
    -n 4 \
    -D SMM_REQUIRE \
    -D SECURE_BOOT_ENABLE \
    -D NETWORK_TLS_ENABLE \
    -D NETWORK_IP6_ENABLE \
    -D NETWORK_HTTP_BOOT_ENABLE \
    --report-file=.../build.ovmf.32.report \
    --log=.../build.ovmf.32.log \
    --cmd-len=65536 \
    --hash \
    --genfds-multi-thread

This command located Python3:

> WORKSPACE        = .../edk2
> EDK_TOOLS_PATH   = .../edk2/BaseTools
> CONF_PATH        = .../edk2/Conf
> PYTHON_COMMAND   = /usr/bin/python3.6
>
>
> Processing meta-data .
> Architecture(s)  = IA32
> Build target     = NOOPT
> Toolchain        = GCC48

The build launched fine.

After 10-20 seconds into the build, I interrupted it with Ctrl-C:

> build.py...
>  : error 7000: Failed to execute command
>         make tbuild [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib]
>
>
> build.py...
>  : error 7000: Failed to execute command
>         make tbuild [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1CommandsLib]
>
>
> build.py...
>  : error 7000: Failed to execute command
>         make tbuild [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslLib/OpensslLib]
>
>
> build.py...
>  : error 7000: Failed to execute command
>         make tbuild [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/MdePkg/Library/BaseLib/BaseLib]
>
> - Aborted -
> Build end time: 14:05:56, Aug.08 2019
> Build total time: 00:00:15

As next step, I repeated the same "build" command as above, in order to
continue the interrupted build. Unfortunately, this failed:

> WORKSPACE        = .../edk2
> EDK_TOOLS_PATH   = .../edk2/BaseTools
> CONF_PATH        = .../edk2/Conf
> PYTHON_COMMAND   = /usr/bin/python3.6
>
>
> Processing meta-data
> .Architecture(s)  = IA32
> Build target     = NOOPT
> Toolchain        = GCC48
>
> Active Platform          = .../edk2/OvmfPkg/OvmfPkgIa32.dsc
> ..... done!
>
> Fd File Name:OVMF (.../edk2/Build/OvmfIa32/NOOPT_GCC48/FV/OVMF.fd)
>
> Generate Region at Offset 0x0
>    Region Size = 0x40000
>    Region Name = DATA
>
> Generate Region at Offset 0x40000
>    Region Size = 0x1000
>    Region Name = None
>
> Generate Region at Offset 0x41000
>    Region Size = 0x1000
>    Region Name = DATA
>
> Generate Region at Offset 0x42000
>    Region Size = 0x42000
>    Region Name = None
>
> Generate Region at Offset 0x84000
>    Region Size = 0x348000
>    Region Name = FV
>
> Generating FVMAIN_COMPACT FV
>
> Generating PEIFV FV
> ###### ['GenFv', '-a', '.../edk2/Build/OvmfIa32/NOOPT_GCC48/FV/Ffs/PEIFV.inf', '-o', '.../edk2/Build/OvmfIa32/NOOPT_GCC48/FV/PEIFV.Fv', '-i', '.../edk2/Build/OvmfIa32/NOOPT_GCC48/FV/PEIFV.inf']
> Return Value = 2
> GenFv: ERROR 0001: Error opening file
>   .../edk2/Build/OvmfIa32/NOOPT_GCC48/FV/Ffs/52C05B14-0B98-496c-BC3B-04B50211D680PeiCore/52C05B14-0B98-496c-BC3B-04B50211D680.ffs
>
>
>
>
> build.py...
>  : error 7000: Failed to generate FV
>
>
>
> build.py...
>  : error 7000: Failed to execute command
>
>
> - Failed -
> Build end time: 14:06:25, Aug.08 2019
> Build total time: 00:00:06

To be honest, I'm not sure what to ask for, at this point.

- On one hand, this is certainly not ideal. Continuing a manually
interrupted build should preferably work -- that's a form of incremental
build. And, it did work in my v3 testing; see bullet (5) in:

  http://mid.mail-archive.com/4ea3d3fa-2210-3642-2337-db525312d312@redhat.com
  https://edk2.groups.io/g/devel/message/44246

(Is this perhaps a regression from the V6 update, which was related to
incremental builds?)

- On the other hand, this is not necessarily show-stopper, and I'm quite
out of capacity for testing further versions of this full patch set.
Perhaps you can work on this issue incrementally -- bugfixes can be
accepted during the freeze periods.

I don't feel comfortable giving Tested-by or Regression-tested-by in
this state, but I also won't block the patch set from being merged.

Note that this problem appears repeatable, and it reproduces using
Python2 as well. It should be possible for you to reproduce and to
debug.


(4) In this test, I repeated (3), but instead of interrupting the build
with Ctrl-C, I introduced a syntax error to one of the C source files
under OvmfPkg (I simply appended the constant "1" to the end of the
file).

As expected, the build failed (and correctly stopped, too):

> .../edk2/OvmfPkg/VirtioNetDxe/SnpReceive.c:186:1: error: expected identifier or '(' before numeric constant
>  1
>  ^
> make: *** [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/OvmfPkg/VirtioNetDxe/VirtioNet/OUTPUT/SnpReceive.obj] Error 1
>
>
> build.py...
>  : error 7000: Failed to execute command
>         make tbuild [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/OvmfPkg/VirtioNetDxe/VirtioNet]
>
>
> build.py...
>  : error F002: Failed to build module
>         .../edk2/OvmfPkg/VirtioNetDxe/VirtioNet.inf [IA32, GCC48, NOOPT]
>
> - Failed -
> Build end time: 14:29:18, Aug.08 2019
> Build total time: 00:00:38

I undid the syntax error, and repeated the "build" command.

The build resumed fine, and produced a functional OVMF binary. Good.


(5) I also verified that changes to C files, made after the build
completed successfully for the first time, would cause those files to be
re-built, if the "build" command was repeated. So that's OK too.


... All in all, I think the series is mature enough to merge, in order
to expose it to wider testing by the community, with the soft feature
freeze just around the corner. The main functionality seems to work,
there don't seem to be show-stoppers. IMO a BaseTools series doesn't
have to be *perfect* -- as long as it doesn't get in the way of people
doing their work, it should be possible to improve upon, incrementally.
Therefore, from my side, I'm willing to give you a (somewhat reserved)

Acked-by: Laszlo Ersek <lersek@redhat.com>

for the series.

I suggest seeking feedback from the other stewards as well.

To reiterate, the only issue I have found is that the build could not be
resumed after I interrupted it with Ctrl-C, in section (3). If there is
consensus to push the v8 series with that, I would suggest filing a
TianoCore BZ about issue (3) first, and to reference the BZ as a "known
issue" in the commit message of patch#4 or patch#5.

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45178): https://edk2.groups.io/g/devel/message/45178
Mute This Topic: https://groups.io/mt/32779325/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Laszlo,

Thanks for looping me in.

On Thu, Aug 08, 2019 at 03:08:22PM +0200, Laszlo Ersek wrote:
> (+ Andrew, Leif, Mike; Liming)
> 
> On 08/07/19 06:25, Bob Feng wrote:
> (3) In my normal edk2 clone, I cleaned the tree, applied your patches
> (again on top of commit 96603b4f02b9), and started a build:
> 
> $ . edksetup.sh
> $ nice make -C "$EDK_TOOLS_PATH" -j $(getconf _NPROCESSORS_ONLN)
> $ nice -n 19 build \
>     -a IA32 \
>     -p OvmfPkg/OvmfPkgIa32.dsc \
>     -t GCC48 \
>     -b NOOPT \
>     -n 4 \
>     -D SMM_REQUIRE \
>     -D SECURE_BOOT_ENABLE \
>     -D NETWORK_TLS_ENABLE \
>     -D NETWORK_IP6_ENABLE \
>     -D NETWORK_HTTP_BOOT_ENABLE \
>     --report-file=.../build.ovmf.32.report \
>     --log=.../build.ovmf.32.log \
>     --cmd-len=65536 \
>     --hash \
>     --genfds-multi-thread
> 
> This command located Python3:
> 
> > WORKSPACE        = .../edk2
> > EDK_TOOLS_PATH   = .../edk2/BaseTools
> > CONF_PATH        = .../edk2/Conf
> > PYTHON_COMMAND   = /usr/bin/python3.6
> >
> >
> > Processing meta-data .
> > Architecture(s)  = IA32
> > Build target     = NOOPT
> > Toolchain        = GCC48
> 
> The build launched fine.
> 
> After 10-20 seconds into the build, I interrupted it with Ctrl-C:
> 
> > build.py...
> >  : error 7000: Failed to execute command
> >         make tbuild [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib]
> >
> >
> > build.py...
> >  : error 7000: Failed to execute command
> >         make tbuild [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1CommandsLib]
> >
> >
> > build.py...
> >  : error 7000: Failed to execute command
> >         make tbuild [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslLib/OpensslLib]
> >
> >
> > build.py...
> >  : error 7000: Failed to execute command
> >         make tbuild [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/MdePkg/Library/BaseLib/BaseLib]
> >
> > - Aborted -
> > Build end time: 14:05:56, Aug.08 2019
> > Build total time: 00:00:15
> 
> As next step, I repeated the same "build" command as above, in order to
> continue the interrupted build. Unfortunately, this failed:
> 
> > WORKSPACE        = .../edk2
> > EDK_TOOLS_PATH   = .../edk2/BaseTools
> > CONF_PATH        = .../edk2/Conf
> > PYTHON_COMMAND   = /usr/bin/python3.6
> >
> >
> > Processing meta-data
> > .Architecture(s)  = IA32
> > Build target     = NOOPT
> > Toolchain        = GCC48
> >
> > Active Platform          = .../edk2/OvmfPkg/OvmfPkgIa32.dsc
> > ..... done!
> >
> > Fd File Name:OVMF (.../edk2/Build/OvmfIa32/NOOPT_GCC48/FV/OVMF.fd)
> >
> > Generate Region at Offset 0x0
> >    Region Size = 0x40000
> >    Region Name = DATA
> >
> > Generate Region at Offset 0x40000
> >    Region Size = 0x1000
> >    Region Name = None
> >
> > Generate Region at Offset 0x41000
> >    Region Size = 0x1000
> >    Region Name = DATA
> >
> > Generate Region at Offset 0x42000
> >    Region Size = 0x42000
> >    Region Name = None
> >
> > Generate Region at Offset 0x84000
> >    Region Size = 0x348000
> >    Region Name = FV
> >
> > Generating FVMAIN_COMPACT FV
> >
> > Generating PEIFV FV
> > ###### ['GenFv', '-a', '.../edk2/Build/OvmfIa32/NOOPT_GCC48/FV/Ffs/PEIFV.inf', '-o', '.../edk2/Build/OvmfIa32/NOOPT_GCC48/FV/PEIFV.Fv', '-i', '.../edk2/Build/OvmfIa32/NOOPT_GCC48/FV/PEIFV.inf']
> > Return Value = 2
> > GenFv: ERROR 0001: Error opening file
> >   .../edk2/Build/OvmfIa32/NOOPT_GCC48/FV/Ffs/52C05B14-0B98-496c-BC3B-04B50211D680PeiCore/52C05B14-0B98-496c-BC3B-04B50211D680.ffs
> >
> >
> >
> >
> > build.py...
> >  : error 7000: Failed to generate FV
> >
> >
> >
> > build.py...
> >  : error 7000: Failed to execute command
> >
> >
> > - Failed -
> > Build end time: 14:06:25, Aug.08 2019
> > Build total time: 00:00:06
> 
> To be honest, I'm not sure what to ask for, at this point.
> 
> - On one hand, this is certainly not ideal. Continuing a manually
> interrupted build should preferably work -- that's a form of incremental
> build. And, it did work in my v3 testing; see bullet (5) in:
> 
>   http://mid.mail-archive.com/4ea3d3fa-2210-3642-2337-db525312d312@redhat.com
>   https://edk2.groups.io/g/devel/message/44246
> 
> (Is this perhaps a regression from the V6 update, which was related to
> incremental builds?)
> 
> - On the other hand, this is not necessarily show-stopper, and I'm quite
> out of capacity for testing further versions of this full patch set.
> Perhaps you can work on this issue incrementally -- bugfixes can be
> accepted during the freeze periods.

I think there are two (independent) circumstances where I would be
happy for the support to be included even given this bug:
1) The parallel autogen is only invoked (at this point in time) when
   requested by an explicit command line parameter.
or
2) The failure is detected and its cause clearly printed for the user.

From my reading of the above, neither is true.

At which point, I think we would either make one of those true, or
root cause and fix the actual error, in order to be able to accept
this into the tree. Regardless of which side of the stable tag.

I *really* don't want for us to knowingly end up with a build system
that "sometimes breaks sporadically and you need to git clean the
repository and try again".

> I don't feel comfortable giving Tested-by or Regression-tested-by in
> this state, but I also won't block the patch set from being merged.
> 
> Note that this problem appears repeatable, and it reproduces using
> Python2 as well. It should be possible for you to reproduce and to
> debug.

It being reproducible by Python 2 is actually really positive, since
it suggests Python 3 async i/o is not involved.

> (4) In this test, I repeated (3), but instead of interrupting the build
> with Ctrl-C, I introduced a syntax error to one of the C source files
> under OvmfPkg (I simply appended the constant "1" to the end of the
> file).
> 
> As expected, the build failed (and correctly stopped, too):
> 
> > .../edk2/OvmfPkg/VirtioNetDxe/SnpReceive.c:186:1: error: expected identifier or '(' before numeric constant
> >  1
> >  ^
> > make: *** [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/OvmfPkg/VirtioNetDxe/VirtioNet/OUTPUT/SnpReceive.obj] Error 1
> >
> >
> > build.py...
> >  : error 7000: Failed to execute command
> >         make tbuild [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/OvmfPkg/VirtioNetDxe/VirtioNet]
> >
> >
> > build.py...
> >  : error F002: Failed to build module
> >         .../edk2/OvmfPkg/VirtioNetDxe/VirtioNet.inf [IA32, GCC48, NOOPT]
> >
> > - Failed -
> > Build end time: 14:29:18, Aug.08 2019
> > Build total time: 00:00:38
> 
> I undid the syntax error, and repeated the "build" command.
> 
> The build resumed fine, and produced a functional OVMF binary. Good.

Not unexpected, but good to have verified.

> (5) I also verified that changes to C files, made after the build
> completed successfully for the first time, would cause those files to be
> re-built, if the "build" command was repeated. So that's OK too.
> 
> ... All in all, I think the series is mature enough to merge, in order
> to expose it to wider testing by the community, with the soft feature
> freeze just around the corner. The main functionality seems to work,
> there don't seem to be show-stoppers. IMO a BaseTools series doesn't
> have to be *perfect* -- as long as it doesn't get in the way of people
> doing their work, it should be possible to improve upon, incrementally.
> Therefore, from my side, I'm willing to give you a (somewhat reserved)
> 
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> 
> for the series.
> 
> I suggest seeking feedback from the other stewards as well.
> 
> To reiterate, the only issue I have found is that the build could not be
> resumed after I interrupted it with Ctrl-C, in section (3). If there is
> consensus to push the v8 series with that, I would suggest filing a
> TianoCore BZ about issue (3) first, and to reference the BZ as a "known
> issue" in the commit message of patch#4 or patch#5.

I will throw in a transitional
Nacked-by: Leif Lindholm <leif.lindholm@linaro.org>
for now.

If it can happen from a Ctrl-C, it can happen from an OOM-event, a
lost network connection, and a bunch of other things. And we could
live with a corrupted state causing breakage on next build attempt -
but not an opaque breakage. At a minimum, it needs to be clear what
has caused the breakage.

Best Regards,

Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45186): https://edk2.groups.io/g/devel/message/45186
Mute This Topic: https://groups.io/mt/32779325/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Laszlo and Leif,

Thanks for you detailed testing and comments.

I'd like to explain the failure of the test 3#. I can reproduce the failure with your steps and I found this failure can also be reproduced without multiple process autogen patch set.  I debugged and found this failure is due to --hash build option. I double tested that if remove --hash build option, the test 3# can pass.  Would you please double verified test 3# without --hash?

I think we can enter a new BZ for the --hash bug.

Thanks,
Bob

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Leif Lindholm
Sent: Thursday, August 8, 2019 9:45 PM
To: Laszlo Ersek <lersek@redhat.com>
Cc: Feng, Bob C <bob.c.feng@intel.com>; devel@edk2.groups.io; Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
Subject: Re: [edk2-devel] [Patch 00/10 V8] Enable multiple process AutoGen

Hi Laszlo,

Thanks for looping me in.

On Thu, Aug 08, 2019 at 03:08:22PM +0200, Laszlo Ersek wrote:
> (+ Andrew, Leif, Mike; Liming)
> 
> On 08/07/19 06:25, Bob Feng wrote:
> (3) In my normal edk2 clone, I cleaned the tree, applied your patches 
> (again on top of commit 96603b4f02b9), and started a build:
> 
> $ . edksetup.sh
> $ nice make -C "$EDK_TOOLS_PATH" -j $(getconf _NPROCESSORS_ONLN) $ 
> nice -n 19 build \
>     -a IA32 \
>     -p OvmfPkg/OvmfPkgIa32.dsc \
>     -t GCC48 \
>     -b NOOPT \
>     -n 4 \
>     -D SMM_REQUIRE \
>     -D SECURE_BOOT_ENABLE \
>     -D NETWORK_TLS_ENABLE \
>     -D NETWORK_IP6_ENABLE \
>     -D NETWORK_HTTP_BOOT_ENABLE \
>     --report-file=.../build.ovmf.32.report \
>     --log=.../build.ovmf.32.log \
>     --cmd-len=65536 \
>     --hash \
>     --genfds-multi-thread
> 
> This command located Python3:
> 
> > WORKSPACE        = .../edk2
> > EDK_TOOLS_PATH   = .../edk2/BaseTools
> > CONF_PATH        = .../edk2/Conf
> > PYTHON_COMMAND   = /usr/bin/python3.6
> >
> >
> > Processing meta-data .
> > Architecture(s)  = IA32
> > Build target     = NOOPT
> > Toolchain        = GCC48
> 
> The build launched fine.
> 
> After 10-20 seconds into the build, I interrupted it with Ctrl-C:
> 
> > build.py...
> >  : error 7000: Failed to execute command
> >         make tbuild 
> > [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/ShellPkg/Library/UefiShell
> > Debug1CommandsLib/UefiShellDebug1CommandsLib]
> >
> >
> > build.py...
> >  : error 7000: Failed to execute command
> >         make tbuild 
> > [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/ShellPkg/Library/UefiShell
> > Driver1CommandsLib/UefiShellDriver1CommandsLib]
> >
> >
> > build.py...
> >  : error 7000: Failed to execute command
> >         make tbuild 
> > [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslL
> > ib/OpensslLib]
> >
> >
> > build.py...
> >  : error 7000: Failed to execute command
> >         make tbuild 
> > [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/MdePkg/Library/BaseLib/Bas
> > eLib]
> >
> > - Aborted -
> > Build end time: 14:05:56, Aug.08 2019 Build total time: 00:00:15
> 
> As next step, I repeated the same "build" command as above, in order 
> to continue the interrupted build. Unfortunately, this failed:
> 
> > WORKSPACE        = .../edk2
> > EDK_TOOLS_PATH   = .../edk2/BaseTools
> > CONF_PATH        = .../edk2/Conf
> > PYTHON_COMMAND   = /usr/bin/python3.6
> >
> >
> > Processing meta-data
> > .Architecture(s)  = IA32
> > Build target     = NOOPT
> > Toolchain        = GCC48
> >
> > Active Platform          = .../edk2/OvmfPkg/OvmfPkgIa32.dsc
> > ..... done!
> >
> > Fd File Name:OVMF (.../edk2/Build/OvmfIa32/NOOPT_GCC48/FV/OVMF.fd)
> >
> > Generate Region at Offset 0x0
> >    Region Size = 0x40000
> >    Region Name = DATA
> >
> > Generate Region at Offset 0x40000
> >    Region Size = 0x1000
> >    Region Name = None
> >
> > Generate Region at Offset 0x41000
> >    Region Size = 0x1000
> >    Region Name = DATA
> >
> > Generate Region at Offset 0x42000
> >    Region Size = 0x42000
> >    Region Name = None
> >
> > Generate Region at Offset 0x84000
> >    Region Size = 0x348000
> >    Region Name = FV
> >
> > Generating FVMAIN_COMPACT FV
> >
> > Generating PEIFV FV
> > ###### ['GenFv', '-a', 
> > '.../edk2/Build/OvmfIa32/NOOPT_GCC48/FV/Ffs/PEIFV.inf', '-o', 
> > '.../edk2/Build/OvmfIa32/NOOPT_GCC48/FV/PEIFV.Fv', '-i', 
> > '.../edk2/Build/OvmfIa32/NOOPT_GCC48/FV/PEIFV.inf']
> > Return Value = 2
> > GenFv: ERROR 0001: Error opening file
> >   
> > .../edk2/Build/OvmfIa32/NOOPT_GCC48/FV/Ffs/52C05B14-0B98-496c-BC3B-0
> > 4B50211D680PeiCore/52C05B14-0B98-496c-BC3B-04B50211D680.ffs
> >
> >
> >
> >
> > build.py...
> >  : error 7000: Failed to generate FV
> >
> >
> >
> > build.py...
> >  : error 7000: Failed to execute command
> >
> >
> > - Failed -
> > Build end time: 14:06:25, Aug.08 2019 Build total time: 00:00:06
> 
> To be honest, I'm not sure what to ask for, at this point.
> 
> - On one hand, this is certainly not ideal. Continuing a manually 
> interrupted build should preferably work -- that's a form of 
> incremental build. And, it did work in my v3 testing; see bullet (5) in:
> 
>   http://mid.mail-archive.com/4ea3d3fa-2210-3642-2337-db525312d312@redhat.com
>   https://edk2.groups.io/g/devel/message/44246
> 
> (Is this perhaps a regression from the V6 update, which was related to 
> incremental builds?)
> 
> - On the other hand, this is not necessarily show-stopper, and I'm 
> quite out of capacity for testing further versions of this full patch set.
> Perhaps you can work on this issue incrementally -- bugfixes can be 
> accepted during the freeze periods.

I think there are two (independent) circumstances where I would be happy for the support to be included even given this bug:
1) The parallel autogen is only invoked (at this point in time) when
   requested by an explicit command line parameter.
or
2) The failure is detected and its cause clearly printed for the user.

From my reading of the above, neither is true.

At which point, I think we would either make one of those true, or root cause and fix the actual error, in order to be able to accept this into the tree. Regardless of which side of the stable tag.

I *really* don't want for us to knowingly end up with a build system that "sometimes breaks sporadically and you need to git clean the repository and try again".

> I don't feel comfortable giving Tested-by or Regression-tested-by in 
> this state, but I also won't block the patch set from being merged.
> 
> Note that this problem appears repeatable, and it reproduces using
> Python2 as well. It should be possible for you to reproduce and to 
> debug.

It being reproducible by Python 2 is actually really positive, since it suggests Python 3 async i/o is not involved.

> (4) In this test, I repeated (3), but instead of interrupting the 
> build with Ctrl-C, I introduced a syntax error to one of the C source 
> files under OvmfPkg (I simply appended the constant "1" to the end of 
> the file).
> 
> As expected, the build failed (and correctly stopped, too):
> 
> > .../edk2/OvmfPkg/VirtioNetDxe/SnpReceive.c:186:1: error: expected 
> > identifier or '(' before numeric constant
> >  1
> >  ^
> > make: *** 
> > [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/OvmfPkg/VirtioNetDxe/Virti
> > oNet/OUTPUT/SnpReceive.obj] Error 1
> >
> >
> > build.py...
> >  : error 7000: Failed to execute command
> >         make tbuild 
> > [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/OvmfPkg/VirtioNetDxe/Virti
> > oNet]
> >
> >
> > build.py...
> >  : error F002: Failed to build module
> >         .../edk2/OvmfPkg/VirtioNetDxe/VirtioNet.inf [IA32, GCC48, 
> > NOOPT]
> >
> > - Failed -
> > Build end time: 14:29:18, Aug.08 2019 Build total time: 00:00:38
> 
> I undid the syntax error, and repeated the "build" command.
> 
> The build resumed fine, and produced a functional OVMF binary. Good.

Not unexpected, but good to have verified.

> (5) I also verified that changes to C files, made after the build 
> completed successfully for the first time, would cause those files to 
> be re-built, if the "build" command was repeated. So that's OK too.
> 
> ... All in all, I think the series is mature enough to merge, in order 
> to expose it to wider testing by the community, with the soft feature 
> freeze just around the corner. The main functionality seems to work, 
> there don't seem to be show-stoppers. IMO a BaseTools series doesn't 
> have to be *perfect* -- as long as it doesn't get in the way of people 
> doing their work, it should be possible to improve upon, incrementally.
> Therefore, from my side, I'm willing to give you a (somewhat reserved)
> 
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> 
> for the series.
> 
> I suggest seeking feedback from the other stewards as well.
> 
> To reiterate, the only issue I have found is that the build could not 
> be resumed after I interrupted it with Ctrl-C, in section (3). If 
> there is consensus to push the v8 series with that, I would suggest 
> filing a TianoCore BZ about issue (3) first, and to reference the BZ 
> as a "known issue" in the commit message of patch#4 or patch#5.

I will throw in a transitional
Nacked-by: Leif Lindholm <leif.lindholm@linaro.org> for now.

If it can happen from a Ctrl-C, it can happen from an OOM-event, a lost network connection, and a bunch of other things. And we could live with a corrupted state causing breakage on next build attempt - but not an opaque breakage. At a minimum, it needs to be clear what has caused the breakage.

Best Regards,

Leif




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45202): https://edk2.groups.io/g/devel/message/45202
Mute This Topic: https://groups.io/mt/32779325/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 08/08/19 17:38, Feng, Bob C wrote:
> Hi Laszlo and Leif,
> 
> Thanks for you detailed testing and comments.
> 
> I'd like to explain the failure of the test 3#. I can reproduce the failure with your steps and I found this failure can also be reproduced without multiple process autogen patch set.  I debugged and found this failure is due to --hash build option. I double tested that if remove --hash build option, the test 3# can pass.  Would you please double verified test 3# without --hash?
> 
> I think we can enter a new BZ for the --hash bug.

Confirmed -- with "--hash" removed from the build command line, the build is picked up fine after Ctrl-C. (And the firmware binary is sound.)

So, my ACK stands.

(

And now I remember that in my v3 testing, I also omitted "--hash": 

  http://mid.mail-archive.com/4ea3d3fa-2210-3642-2337-db525312d312@redhat.com
  https://edk2.groups.io/g/devel/message/44246

At the bottom I stated that I didn't test "--hash".

)

Thanks
Laszlo


> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Leif Lindholm
> Sent: Thursday, August 8, 2019 9:45 PM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Feng, Bob C <bob.c.feng@intel.com>; devel@edk2.groups.io; Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [Patch 00/10 V8] Enable multiple process AutoGen
> 
> Hi Laszlo,
> 
> Thanks for looping me in.
> 
> On Thu, Aug 08, 2019 at 03:08:22PM +0200, Laszlo Ersek wrote:
>> (+ Andrew, Leif, Mike; Liming)
>>
>> On 08/07/19 06:25, Bob Feng wrote:
>> (3) In my normal edk2 clone, I cleaned the tree, applied your patches 
>> (again on top of commit 96603b4f02b9), and started a build:
>>
>> $ . edksetup.sh
>> $ nice make -C "$EDK_TOOLS_PATH" -j $(getconf _NPROCESSORS_ONLN) $ 
>> nice -n 19 build \
>>     -a IA32 \
>>     -p OvmfPkg/OvmfPkgIa32.dsc \
>>     -t GCC48 \
>>     -b NOOPT \
>>     -n 4 \
>>     -D SMM_REQUIRE \
>>     -D SECURE_BOOT_ENABLE \
>>     -D NETWORK_TLS_ENABLE \
>>     -D NETWORK_IP6_ENABLE \
>>     -D NETWORK_HTTP_BOOT_ENABLE \
>>     --report-file=.../build.ovmf.32.report \
>>     --log=.../build.ovmf.32.log \
>>     --cmd-len=65536 \
>>     --hash \
>>     --genfds-multi-thread
>>
>> This command located Python3:
>>
>>> WORKSPACE        = .../edk2
>>> EDK_TOOLS_PATH   = .../edk2/BaseTools
>>> CONF_PATH        = .../edk2/Conf
>>> PYTHON_COMMAND   = /usr/bin/python3.6
>>>
>>>
>>> Processing meta-data .
>>> Architecture(s)  = IA32
>>> Build target     = NOOPT
>>> Toolchain        = GCC48
>>
>> The build launched fine.
>>
>> After 10-20 seconds into the build, I interrupted it with Ctrl-C:
>>
>>> build.py...
>>>  : error 7000: Failed to execute command
>>>         make tbuild 
>>> [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/ShellPkg/Library/UefiShell
>>> Debug1CommandsLib/UefiShellDebug1CommandsLib]
>>>
>>>
>>> build.py...
>>>  : error 7000: Failed to execute command
>>>         make tbuild 
>>> [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/ShellPkg/Library/UefiShell
>>> Driver1CommandsLib/UefiShellDriver1CommandsLib]
>>>
>>>
>>> build.py...
>>>  : error 7000: Failed to execute command
>>>         make tbuild 
>>> [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslL
>>> ib/OpensslLib]
>>>
>>>
>>> build.py...
>>>  : error 7000: Failed to execute command
>>>         make tbuild 
>>> [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/MdePkg/Library/BaseLib/Bas
>>> eLib]
>>>
>>> - Aborted -
>>> Build end time: 14:05:56, Aug.08 2019 Build total time: 00:00:15
>>
>> As next step, I repeated the same "build" command as above, in order 
>> to continue the interrupted build. Unfortunately, this failed:
>>
>>> WORKSPACE        = .../edk2
>>> EDK_TOOLS_PATH   = .../edk2/BaseTools
>>> CONF_PATH        = .../edk2/Conf
>>> PYTHON_COMMAND   = /usr/bin/python3.6
>>>
>>>
>>> Processing meta-data
>>> .Architecture(s)  = IA32
>>> Build target     = NOOPT
>>> Toolchain        = GCC48
>>>
>>> Active Platform          = .../edk2/OvmfPkg/OvmfPkgIa32.dsc
>>> ..... done!
>>>
>>> Fd File Name:OVMF (.../edk2/Build/OvmfIa32/NOOPT_GCC48/FV/OVMF.fd)
>>>
>>> Generate Region at Offset 0x0
>>>    Region Size = 0x40000
>>>    Region Name = DATA
>>>
>>> Generate Region at Offset 0x40000
>>>    Region Size = 0x1000
>>>    Region Name = None
>>>
>>> Generate Region at Offset 0x41000
>>>    Region Size = 0x1000
>>>    Region Name = DATA
>>>
>>> Generate Region at Offset 0x42000
>>>    Region Size = 0x42000
>>>    Region Name = None
>>>
>>> Generate Region at Offset 0x84000
>>>    Region Size = 0x348000
>>>    Region Name = FV
>>>
>>> Generating FVMAIN_COMPACT FV
>>>
>>> Generating PEIFV FV
>>> ###### ['GenFv', '-a', 
>>> '.../edk2/Build/OvmfIa32/NOOPT_GCC48/FV/Ffs/PEIFV.inf', '-o', 
>>> '.../edk2/Build/OvmfIa32/NOOPT_GCC48/FV/PEIFV.Fv', '-i', 
>>> '.../edk2/Build/OvmfIa32/NOOPT_GCC48/FV/PEIFV.inf']
>>> Return Value = 2
>>> GenFv: ERROR 0001: Error opening file
>>>   
>>> .../edk2/Build/OvmfIa32/NOOPT_GCC48/FV/Ffs/52C05B14-0B98-496c-BC3B-0
>>> 4B50211D680PeiCore/52C05B14-0B98-496c-BC3B-04B50211D680.ffs
>>>
>>>
>>>
>>>
>>> build.py...
>>>  : error 7000: Failed to generate FV
>>>
>>>
>>>
>>> build.py...
>>>  : error 7000: Failed to execute command
>>>
>>>
>>> - Failed -
>>> Build end time: 14:06:25, Aug.08 2019 Build total time: 00:00:06
>>
>> To be honest, I'm not sure what to ask for, at this point.
>>
>> - On one hand, this is certainly not ideal. Continuing a manually 
>> interrupted build should preferably work -- that's a form of 
>> incremental build. And, it did work in my v3 testing; see bullet (5) in:
>>
>>   http://mid.mail-archive.com/4ea3d3fa-2210-3642-2337-db525312d312@redhat.com
>>   https://edk2.groups.io/g/devel/message/44246
>>
>> (Is this perhaps a regression from the V6 update, which was related to 
>> incremental builds?)
>>
>> - On the other hand, this is not necessarily show-stopper, and I'm 
>> quite out of capacity for testing further versions of this full patch set.
>> Perhaps you can work on this issue incrementally -- bugfixes can be 
>> accepted during the freeze periods.
> 
> I think there are two (independent) circumstances where I would be happy for the support to be included even given this bug:
> 1) The parallel autogen is only invoked (at this point in time) when
>    requested by an explicit command line parameter.
> or
> 2) The failure is detected and its cause clearly printed for the user.
> 
> From my reading of the above, neither is true.
> 
> At which point, I think we would either make one of those true, or root cause and fix the actual error, in order to be able to accept this into the tree. Regardless of which side of the stable tag.
> 
> I *really* don't want for us to knowingly end up with a build system that "sometimes breaks sporadically and you need to git clean the repository and try again".
> 
>> I don't feel comfortable giving Tested-by or Regression-tested-by in 
>> this state, but I also won't block the patch set from being merged.
>>
>> Note that this problem appears repeatable, and it reproduces using
>> Python2 as well. It should be possible for you to reproduce and to 
>> debug.
> 
> It being reproducible by Python 2 is actually really positive, since it suggests Python 3 async i/o is not involved.
> 
>> (4) In this test, I repeated (3), but instead of interrupting the 
>> build with Ctrl-C, I introduced a syntax error to one of the C source 
>> files under OvmfPkg (I simply appended the constant "1" to the end of 
>> the file).
>>
>> As expected, the build failed (and correctly stopped, too):
>>
>>> .../edk2/OvmfPkg/VirtioNetDxe/SnpReceive.c:186:1: error: expected 
>>> identifier or '(' before numeric constant
>>>  1
>>>  ^
>>> make: *** 
>>> [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/OvmfPkg/VirtioNetDxe/Virti
>>> oNet/OUTPUT/SnpReceive.obj] Error 1
>>>
>>>
>>> build.py...
>>>  : error 7000: Failed to execute command
>>>         make tbuild 
>>> [.../edk2/Build/OvmfIa32/NOOPT_GCC48/IA32/OvmfPkg/VirtioNetDxe/Virti
>>> oNet]
>>>
>>>
>>> build.py...
>>>  : error F002: Failed to build module
>>>         .../edk2/OvmfPkg/VirtioNetDxe/VirtioNet.inf [IA32, GCC48, 
>>> NOOPT]
>>>
>>> - Failed -
>>> Build end time: 14:29:18, Aug.08 2019 Build total time: 00:00:38
>>
>> I undid the syntax error, and repeated the "build" command.
>>
>> The build resumed fine, and produced a functional OVMF binary. Good.
> 
> Not unexpected, but good to have verified.
> 
>> (5) I also verified that changes to C files, made after the build 
>> completed successfully for the first time, would cause those files to 
>> be re-built, if the "build" command was repeated. So that's OK too.
>>
>> ... All in all, I think the series is mature enough to merge, in order 
>> to expose it to wider testing by the community, with the soft feature 
>> freeze just around the corner. The main functionality seems to work, 
>> there don't seem to be show-stoppers. IMO a BaseTools series doesn't 
>> have to be *perfect* -- as long as it doesn't get in the way of people 
>> doing their work, it should be possible to improve upon, incrementally.
>> Therefore, from my side, I'm willing to give you a (somewhat reserved)
>>
>> Acked-by: Laszlo Ersek <lersek@redhat.com>
>>
>> for the series.
>>
>> I suggest seeking feedback from the other stewards as well.
>>
>> To reiterate, the only issue I have found is that the build could not 
>> be resumed after I interrupted it with Ctrl-C, in section (3). If 
>> there is consensus to push the v8 series with that, I would suggest 
>> filing a TianoCore BZ about issue (3) first, and to reference the BZ 
>> as a "known issue" in the commit message of patch#4 or patch#5.
> 
> I will throw in a transitional
> Nacked-by: Leif Lindholm <leif.lindholm@linaro.org> for now.
> 
> If it can happen from a Ctrl-C, it can happen from an OOM-event, a lost network connection, and a bunch of other things. And we could live with a corrupted state causing breakage on next build attempt - but not an opaque breakage. At a minimum, it needs to be clear what has caused the breakage.
> 
> Best Regards,
> 
> Leif
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45237): https://edk2.groups.io/g/devel/message/45237
Mute This Topic: https://groups.io/mt/32779325/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 08/09/19 00:18, Laszlo Ersek wrote:
> On 08/08/19 17:38, Feng, Bob C wrote:
>> Hi Laszlo and Leif,
>>
>> Thanks for you detailed testing and comments.
>>
>> I'd like to explain the failure of the test 3#. I can reproduce the failure with your steps and I found this failure can also be reproduced without multiple process autogen patch set.  I debugged and found this failure is due to --hash build option. I double tested that if remove --hash build option, the test 3# can pass.  Would you please double verified test 3# without --hash?
>>
>> I think we can enter a new BZ for the --hash bug.
> 
> Confirmed -- with "--hash" removed from the build command line, the build is picked up fine after Ctrl-C. (And the firmware binary is sound.)
> 
> So, my ACK stands.
> 
> (
> 
> And now I remember that in my v3 testing, I also omitted "--hash": 
> 
>   http://mid.mail-archive.com/4ea3d3fa-2210-3642-2337-db525312d312@redhat.com
>   https://edk2.groups.io/g/devel/message/44246
> 
> At the bottom I stated that I didn't test "--hash".
> 
> )

... based on the above, I can even provide:

Tested-by: Laszlo Ersek <lersek@redhat.com>

for the series.

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45240): https://edk2.groups.io/g/devel/message/45240
Mute This Topic: https://groups.io/mt/32779325/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Bob:
  I have no comments for this patch set. I am OK to add it for 201908 stable tag. 
  
  Ack-by: Liming Gao <liming.gao@intel.com>

Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
> Sent: Friday, August 9, 2019 7:29 AM
> To: Feng, Bob C <bob.c.feng@intel.com>; devel@edk2.groups.io; leif.lindholm@linaro.org
> Cc: Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [Patch 00/10 V8] Enable multiple process AutoGen
> 
> On 08/09/19 00:18, Laszlo Ersek wrote:
> > On 08/08/19 17:38, Feng, Bob C wrote:
> >> Hi Laszlo and Leif,
> >>
> >> Thanks for you detailed testing and comments.
> >>
> >> I'd like to explain the failure of the test 3#. I can reproduce the failure with your steps and I found this failure can also be reproduced
> without multiple process autogen patch set.  I debugged and found this failure is due to --hash build option. I double tested that if
> remove --hash build option, the test 3# can pass.  Would you please double verified test 3# without --hash?
> >>
> >> I think we can enter a new BZ for the --hash bug.
> >
> > Confirmed -- with "--hash" removed from the build command line, the build is picked up fine after Ctrl-C. (And the firmware binary is
> sound.)
> >
> > So, my ACK stands.
> >
> > (
> >
> > And now I remember that in my v3 testing, I also omitted "--hash":
> >
> >   http://mid.mail-archive.com/4ea3d3fa-2210-3642-2337-db525312d312@redhat.com
> >   https://edk2.groups.io/g/devel/message/44246
> >
> > At the bottom I stated that I didn't test "--hash".
> >
> > )
> 
> ... based on the above, I can even provide:
> 
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> 
> for the series.
> 
> Thanks
> Laszlo
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45249): https://edk2.groups.io/g/devel/message/45249
Mute This Topic: https://groups.io/mt/32779325/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Thanks for your response.

I'm going to push this patch set late today, if there is no more comments.

Thanks,
Bob

-----Original Message-----
From: Gao, Liming 
Sent: Friday, August 9, 2019 8:50 AM
To: devel@edk2.groups.io; lersek@redhat.com; Feng, Bob C <bob.c.feng@intel.com>; leif.lindholm@linaro.org
Cc: Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [Patch 00/10 V8] Enable multiple process AutoGen

Bob:
  I have no comments for this patch set. I am OK to add it for 201908 stable tag. 
  
  Ack-by: Liming Gao <liming.gao@intel.com>

Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of 
> Laszlo Ersek
> Sent: Friday, August 9, 2019 7:29 AM
> To: Feng, Bob C <bob.c.feng@intel.com>; devel@edk2.groups.io; 
> leif.lindholm@linaro.org
> Cc: Andrew Fish <afish@apple.com>; Kinney, Michael D 
> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [Patch 00/10 V8] Enable multiple process 
> AutoGen
> 
> On 08/09/19 00:18, Laszlo Ersek wrote:
> > On 08/08/19 17:38, Feng, Bob C wrote:
> >> Hi Laszlo and Leif,
> >>
> >> Thanks for you detailed testing and comments.
> >>
> >> I'd like to explain the failure of the test 3#. I can reproduce the 
> >> failure with your steps and I found this failure can also be 
> >> reproduced
> without multiple process autogen patch set.  I debugged and found this 
> failure is due to --hash build option. I double tested that if remove --hash build option, the test 3# can pass.  Would you please double verified test 3# without --hash?
> >>
> >> I think we can enter a new BZ for the --hash bug.
> >
> > Confirmed -- with "--hash" removed from the build command line, the 
> > build is picked up fine after Ctrl-C. (And the firmware binary is
> sound.)
> >
> > So, my ACK stands.
> >
> > (
> >
> > And now I remember that in my v3 testing, I also omitted "--hash":
> >
> >   http://mid.mail-archive.com/4ea3d3fa-2210-3642-2337-db525312d312@redhat.com
> >   https://edk2.groups.io/g/devel/message/44246
> >
> > At the bottom I stated that I didn't test "--hash".
> >
> > )
> 
> ... based on the above, I can even provide:
> 
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> 
> for the series.
> 
> Thanks
> Laszlo
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45250): https://edk2.groups.io/g/devel/message/45250
Mute This Topic: https://groups.io/mt/32779325/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 08/09/19 03:37, Feng, Bob C wrote:
> Thanks for your response.
> 
> I'm going to push this patch set late today, if there is no more comments.

Sorry, I have to disagree about that. There is a NACK standing from Leif
-- he will have to rescind that NACK explicitly, for you to push the series.

Leif: can you please confirm, in light of the new information [*] if you
are OK with this v8 patchset being merged?

[*] The new information is that "--hash" breaks "pick-up" after Ctrl-C,
independently of the present patchset. So, it's not a regression from
the new work, and if "--hash" is removed from the command line, the
"pick-up" is successful both with and without these patches.

Thanks!
Laszlo



> -----Original Message-----
> From: Gao, Liming 
> Sent: Friday, August 9, 2019 8:50 AM
> To: devel@edk2.groups.io; lersek@redhat.com; Feng, Bob C <bob.c.feng@intel.com>; leif.lindholm@linaro.org
> Cc: Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] [Patch 00/10 V8] Enable multiple process AutoGen
> 
> Bob:
>   I have no comments for this patch set. I am OK to add it for 201908 stable tag. 
>   
>   Ack-by: Liming Gao <liming.gao@intel.com>
> 
> Thanks
> Liming
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of 
>> Laszlo Ersek
>> Sent: Friday, August 9, 2019 7:29 AM
>> To: Feng, Bob C <bob.c.feng@intel.com>; devel@edk2.groups.io; 
>> leif.lindholm@linaro.org
>> Cc: Andrew Fish <afish@apple.com>; Kinney, Michael D 
>> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
>> Subject: Re: [edk2-devel] [Patch 00/10 V8] Enable multiple process 
>> AutoGen
>>
>> On 08/09/19 00:18, Laszlo Ersek wrote:
>>> On 08/08/19 17:38, Feng, Bob C wrote:
>>>> Hi Laszlo and Leif,
>>>>
>>>> Thanks for you detailed testing and comments.
>>>>
>>>> I'd like to explain the failure of the test 3#. I can reproduce the 
>>>> failure with your steps and I found this failure can also be 
>>>> reproduced
>> without multiple process autogen patch set.  I debugged and found this 
>> failure is due to --hash build option. I double tested that if remove --hash build option, the test 3# can pass.  Would you please double verified test 3# without --hash?
>>>>
>>>> I think we can enter a new BZ for the --hash bug.
>>>
>>> Confirmed -- with "--hash" removed from the build command line, the 
>>> build is picked up fine after Ctrl-C. (And the firmware binary is
>> sound.)
>>>
>>> So, my ACK stands.
>>>
>>> (
>>>
>>> And now I remember that in my v3 testing, I also omitted "--hash":
>>>
>>>   http://mid.mail-archive.com/4ea3d3fa-2210-3642-2337-db525312d312@redhat.com
>>>   https://edk2.groups.io/g/devel/message/44246
>>>
>>> At the bottom I stated that I didn't test "--hash".
>>>
>>> )
>>
>> ... based on the above, I can even provide:
>>
>> Tested-by: Laszlo Ersek <lersek@redhat.com>
>>
>> for the series.
>>
>> Thanks
>> Laszlo
>>
>> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45271): https://edk2.groups.io/g/devel/message/45271
Mute This Topic: https://groups.io/mt/32779325/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Fri, Aug 09, 2019 at 02:54:31PM +0200, Laszlo Ersek wrote:
> On 08/09/19 03:37, Feng, Bob C wrote:
> > Thanks for your response.
> > 
> > I'm going to push this patch set late today, if there is no more comments.
> 
> Sorry, I have to disagree about that. There is a NACK standing from Leif
> -- he will have to rescind that NACK explicitly, for you to push the series.
> 
> Leif: can you please confirm, in light of the new information [*] if you
> are OK with this v8 patchset being merged?
> 
> [*] The new information is that "--hash" breaks "pick-up" after Ctrl-C,
> independently of the present patchset. So, it's not a regression from
> the new work, and if "--hash" is removed from the command line, the
> "pick-up" is successful both with and without these patches.

Ah, it was not clear to me that the bug was reproducible with --hash
without this set. (I read it as the bug was only reproducible with
this set if using --hash, which is not the same thing.)

In that case, I rescind my NACK.

Thank you for the clarification, Laszlo.

I would say this makes a fix for the --hash issue a hard requirement
for the stable tag.

Best Regards,

Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45272): https://edk2.groups.io/g/devel/message/45272
Mute This Topic: https://groups.io/mt/32779325/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Leif:

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Leif Lindholm
> Sent: Friday, August 9, 2019 9:29 PM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Andrew Fish <afish@apple.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] [Patch 00/10 V8] Enable multiple process AutoGen
> 
> On Fri, Aug 09, 2019 at 02:54:31PM +0200, Laszlo Ersek wrote:
> > On 08/09/19 03:37, Feng, Bob C wrote:
> > > Thanks for your response.
> > >
> > > I'm going to push this patch set late today, if there is no more comments.
> >
> > Sorry, I have to disagree about that. There is a NACK standing from Leif
> > -- he will have to rescind that NACK explicitly, for you to push the series.
> >
> > Leif: can you please confirm, in light of the new information [*] if you
> > are OK with this v8 patchset being merged?
> >
> > [*] The new information is that "--hash" breaks "pick-up" after Ctrl-C,
> > independently of the present patchset. So, it's not a regression from
> > the new work, and if "--hash" is removed from the command line, the
> > "pick-up" is successful both with and without these patches.
> 
> Ah, it was not clear to me that the bug was reproducible with --hash
> without this set. (I read it as the bug was only reproducible with
> this set if using --hash, which is not the same thing.)
> 
> In that case, I rescind my NACK.
> 
> Thank you for the clarification, Laszlo.
> 
> I would say this makes a fix for the --hash issue a hard requirement
> for the stable tag.
> 

If --hash issue is the regression issue caused by this patch set, I agree it must be fixed
together with this patch set. If --hash issue is the existing issue, and it also exists in 201905 
stable tag, it may be another issue. We can handle it as the separate BZ. 

Thanks
Liming

> Best Regards,
> 
> Leif
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45281): https://edk2.groups.io/g/devel/message/45281
Mute This Topic: https://groups.io/mt/32779325/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Fri, Aug 09, 2019 at 02:54:28PM +0000, Gao, Liming wrote:
> > Thank you for the clarification, Laszlo.
> > 
> > I would say this makes a fix for the --hash issue a hard requirement
> > for the stable tag.
> 
> If --hash issue is the regression issue caused by this patch set, I agree it must be fixed
> together with this patch set. If --hash issue is the existing issue, and it also exists in 201905 
> stable tag, it may be another issue. We can handle it as the separate BZ. 

Yes, it has no relevance for this patch set.

Best Regards,

Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45286): https://edk2.groups.io/g/devel/message/45286
Mute This Topic: https://groups.io/mt/32779325/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Thanks Leif, I'll push the patches.

I file a BZ https://bugzilla.tianocore.org/show_bug.cgi?id=2072 for the --hash issue.

Thanks,
Bob

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Leif Lindholm
Sent: Friday, August 9, 2019 9:29 PM
To: Laszlo Ersek <lersek@redhat.com>
Cc: Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [Patch 00/10 V8] Enable multiple process AutoGen

On Fri, Aug 09, 2019 at 02:54:31PM +0200, Laszlo Ersek wrote:
> On 08/09/19 03:37, Feng, Bob C wrote:
> > Thanks for your response.
> > 
> > I'm going to push this patch set late today, if there is no more comments.
> 
> Sorry, I have to disagree about that. There is a NACK standing from 
> Leif
> -- he will have to rescind that NACK explicitly, for you to push the series.
> 
> Leif: can you please confirm, in light of the new information [*] if 
> you are OK with this v8 patchset being merged?
> 
> [*] The new information is that "--hash" breaks "pick-up" after 
> Ctrl-C, independently of the present patchset. So, it's not a 
> regression from the new work, and if "--hash" is removed from the 
> command line, the "pick-up" is successful both with and without these patches.

Ah, it was not clear to me that the bug was reproducible with --hash without this set. (I read it as the bug was only reproducible with this set if using --hash, which is not the same thing.)

In that case, I rescind my NACK.

Thank you for the clarification, Laszlo.

I would say this makes a fix for the --hash issue a hard requirement for the stable tag.

Best Regards,

Leif




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45282): https://edk2.groups.io/g/devel/message/45282
Mute This Topic: https://groups.io/mt/32779325/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Sure. Thanks Laszlo for your clarification. 

-Bob

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Friday, August 9, 2019 8:55 PM
To: Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; leif.lindholm@linaro.org
Cc: Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [Patch 00/10 V8] Enable multiple process AutoGen

On 08/09/19 03:37, Feng, Bob C wrote:
> Thanks for your response.
> 
> I'm going to push this patch set late today, if there is no more comments.

Sorry, I have to disagree about that. There is a NACK standing from Leif
-- he will have to rescind that NACK explicitly, for you to push the series.

Leif: can you please confirm, in light of the new information [*] if you are OK with this v8 patchset being merged?

[*] The new information is that "--hash" breaks "pick-up" after Ctrl-C, independently of the present patchset. So, it's not a regression from the new work, and if "--hash" is removed from the command line, the "pick-up" is successful both with and without these patches.

Thanks!
Laszlo



> -----Original Message-----
> From: Gao, Liming
> Sent: Friday, August 9, 2019 8:50 AM
> To: devel@edk2.groups.io; lersek@redhat.com; Feng, Bob C 
> <bob.c.feng@intel.com>; leif.lindholm@linaro.org
> Cc: Andrew Fish <afish@apple.com>; Kinney, Michael D 
> <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] [Patch 00/10 V8] Enable multiple process 
> AutoGen
> 
> Bob:
>   I have no comments for this patch set. I am OK to add it for 201908 stable tag. 
>   
>   Ack-by: Liming Gao <liming.gao@intel.com>
> 
> Thanks
> Liming
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of 
>> Laszlo Ersek
>> Sent: Friday, August 9, 2019 7:29 AM
>> To: Feng, Bob C <bob.c.feng@intel.com>; devel@edk2.groups.io; 
>> leif.lindholm@linaro.org
>> Cc: Andrew Fish <afish@apple.com>; Kinney, Michael D 
>> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
>> Subject: Re: [edk2-devel] [Patch 00/10 V8] Enable multiple process 
>> AutoGen
>>
>> On 08/09/19 00:18, Laszlo Ersek wrote:
>>> On 08/08/19 17:38, Feng, Bob C wrote:
>>>> Hi Laszlo and Leif,
>>>>
>>>> Thanks for you detailed testing and comments.
>>>>
>>>> I'd like to explain the failure of the test 3#. I can reproduce the 
>>>> failure with your steps and I found this failure can also be 
>>>> reproduced
>> without multiple process autogen patch set.  I debugged and found 
>> this failure is due to --hash build option. I double tested that if remove --hash build option, the test 3# can pass.  Would you please double verified test 3# without --hash?
>>>>
>>>> I think we can enter a new BZ for the --hash bug.
>>>
>>> Confirmed -- with "--hash" removed from the build command line, the 
>>> build is picked up fine after Ctrl-C. (And the firmware binary is
>> sound.)
>>>
>>> So, my ACK stands.
>>>
>>> (
>>>
>>> And now I remember that in my v3 testing, I also omitted "--hash":
>>>
>>>   http://mid.mail-archive.com/4ea3d3fa-2210-3642-2337-db525312d312@redhat.com
>>>   https://edk2.groups.io/g/devel/message/44246
>>>
>>> At the bottom I stated that I didn't test "--hash".
>>>
>>> )
>>
>> ... based on the above, I can even provide:
>>
>> Tested-by: Laszlo Ersek <lersek@redhat.com>
>>
>> for the series.
>>
>> Thanks
>> Laszlo
>>
>> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45280): https://edk2.groups.io/g/devel/message/45280
Mute This Topic: https://groups.io/mt/32779325/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-