Re: [PATCH] BaseTools: Fix build failure when specifying multiple BUILDTARGET

["Carsey, Jaben" <jaben.carsey@intel.com>]

Laszlo,

not sure which Andrew you wanted, but he didn’t get added so far as I can tell.

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Tuesday, February 05, 2019 4:05 AM
> To: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2] [PATCH] BaseTools: Fix build failure when specifying
> multiple BUILDTARGET
> 
> (+Mike, +Andrew)
> 
> On 02/05/19 12:47, Leif Lindholm wrote:
> > On Tue, Feb 05, 2019 at 10:03:21AM +0100, Laszlo Ersek wrote:
> >> On 02/05/19 02:23, Philippe Mathieu-Daudé wrote:
> >>> Since 9c2d68c0a299 the build tools default to the python version
> >>> provided by the ${PYTHON} environment variable.
> >>> However the Python3 transition is not effective before d943b0c339fe.
> >>
> >> (1) Do you mean "functional" rather than "effective"?
> >>
> >> (2) Why is this information relevant for this commit? I see that commit
> >> f8d11e5a4aaa, referenced below, falls between the above two, but I'm
> >> unsure if that has any special relevance.
> >>
> >> If the above paragraph is just background info, that's OK with me, of
> >> course.
> >>
> >>> With Python3, the dict.value() method returns an iterator.
> >>> If a dictionary is updated while an iterator on his keys is used,
> >>
> >> (3) s/his/its/
> >>
> >>> a RuntimeError is generated.
> >>> Converting the iterator to a list() forces a copy of the mutable
> >>> keys in an immutable list which can be safely iterated.
> >>>
> >>> Commit f8d11e5a4aaa converted various uses but missed one:
> >>> When specifying multiple BUILDTARGET, the first target builds
> >>> successfully, but then the PGen.BuildDatabase._CACHE_ dictionary is
> >>> updated, and the next target accessing it triggers a RuntimeError.
> >>
> >> (4) Can we clarify this please; I think it's not the "next target" that
> >> accesses the dictionary, instead the code accesses the next target in
> >> the dictionary. How about
> >>
> >>   s/the next target accessing it/accessing the next target/
> >>
> >> ?
> >>
> >>>
> >>> Convert this iterator to an immutable list, to solve this build error:
> >>>
> >>>     $ build -a IA32 -t GCC5 -b RELEASE -b NOOPT -p
> OvmfPkg/OvmfPkgIa32.dsc
> >>>     [...]
> >>>     Processing meta-data ...
> >>>     build.py...
> >>>      : error C0DE: Unknown fatal error when processing
> [OvmfPkg/OvmfPkgIa32.dsc]
> >>>
> >>>     (Please send email to edk2-devel@lists.01.org for help, attaching
> following call stack trace!)
> >>>
> >>>     (Python 3.5.3 on linux) Traceback (most recent call last):
> >>>       File
> "BaseTools/BinWrappers/PosixLike/../../Source/Python/build/build.py", line
> 2387, in Main
> >>>         MyBuild.Launch()
> >>>       File
> "BaseTools/BinWrappers/PosixLike/../../Source/Python/build/build.py", line
> 2141, in Launch
> >>>         self._MultiThreadBuildPlatform()
> >>>       File
> "BaseTools/BinWrappers/PosixLike/../../Source/Python/build/build.py", line
> 1921, in _MultiThreadBuildPlatform
> >>>         self.Progress
> >>>       File "BaseTools/Source/Python/AutoGen/AutoGen.py", line 304, in
> __init__
> >>>         self._InitWorker(Workspace, MetaFile, Target, Toolchain, Arch,
> *args, **kwargs)
> >>>       File "BaseTools/Source/Python/AutoGen/AutoGen.py", line 477, in
> _InitWorker
> >>>         for BuildData in PGen.BuildDatabase._CACHE_.values():
> >>>     RuntimeError: dictionary changed size during iteration
> >>>
> >>> Reported-by: Leif Lindholm <leif.lindholm@linaro.org>
> >>> Fixes: f8d11e5a4aaa90bf63b4789f3993dd6d16c60787
> >>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>> Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
> >>> Tested-by: Leif Lindholm <leif.lindholm@linaro.org>
> >>> ---
> >>>  BaseTools/Source/Python/AutoGen/AutoGen.py | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/BaseTools/Source/Python/AutoGen/AutoGen.py
> b/BaseTools/Source/Python/AutoGen/AutoGen.py
> >>> index a95d2c710e..12592a2a46 100644
> >>> --- a/BaseTools/Source/Python/AutoGen/AutoGen.py
> >>> +++ b/BaseTools/Source/Python/AutoGen/AutoGen.py
> >>> @@ -474,7 +474,7 @@ class WorkspaceAutoGen(AutoGen):
> >>>
> >>>              # generate the SourcePcdDict and BinaryPcdDict
> >>>              PGen = PlatformAutoGen(self, self.MetaFile, Target, Toolchain,
> Arch)
> >>> -            for BuildData in PGen.BuildDatabase._CACHE_.values():
> >>> +            for BuildData in list(PGen.BuildDatabase._CACHE_.values()):
> >>>                  if BuildData.Arch != Arch:
> >>>                      continue
> >>>                  if BuildData.MetaFile.Ext == '.inf':
> >>>
> >>
> >> LGTM :)
> >>
> >> With the commit message updated (as you prefer):
> >>
> >> Acked-by: Laszlo Ersek <lersek@redhat.com>
> >
> > I would be surprised if we hear back from the BaseTools maintainers
> > this week (Chinese New Year). For me, I have both a simple workaround
> > and this patch, so I'm OK with waiting.
> >
> > But if anyone reports issues with CI environments or similar, I would
> > say either you or I could push this patch in their absence.
> 
> Right; see also the patch I just posted:
> 
> [edk2] [PATCH] BaseTools/BuildReport: fix report for platforms/arches
> without struct PCDs
> 
> It's another build breakage, although less intrusive (it only occurs if
> you ask for a report file).
> 
> I think the last agreement on the list was that fixes for build
> breakages could be pushed after 24 hours of posting, regardless of
> Maintainer review, if the patches were "sufficiently reviewed otherwise".
> 
> I suggest we push the (upcoming v2) patch tomorrow.

Agreed.  I can push if desired, once final version is complete.

> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel