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

[Laszlo Ersek <lersek@redhat.com>]

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

Thanks
Laszlo