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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 2/5/19 10:03 AM, 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"?

Probably :)

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

Yes, I'll reword to explain the tree isn't bisectable there, or drop
that background info.

>> 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/
> 
> ?

Sure.

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

Thanks!

Regards,

Phil.

> 
> Thanks
> Laszlo
>