[NaCl SDK] Fix create_nmf handling of missing libraries.

[NaCl SDK] Fix create_nmf handling of missing libraries.

Previously a library that was needed on two architectures
but only found on one would be silently ignored.

BUG=
R=binji@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202688

[binji, 2013-05-28 22:52:41]

lgtm

https://codereview.chromium.org/15891011/diff/2001/native_client_sdk/src/tool...
File native_client_sdk/src/tools/create_nmf.py (right):

https://codereview.chromium.org/15891011/diff/2001/native_client_sdk/src/tool...
native_client_sdk/src/tools/create_nmf.py:283: found_basenames = {}
= set(), you never use the value

https://codereview.chromium.org/15891011/diff/2001/native_client_sdk/src/tool...
native_client_sdk/src/tools/create_nmf.py:308: if not arch or arch == file_arch:
might be nicer to use
not(arch and arch != file_arch) to make the connection between this and the
above check clearer.

https://codereview.chromium.org/15891011/diff/2001/native_client_sdk/src/tool...
native_client_sdk/src/tools/create_nmf.py:308: if not arch or arch == file_arch:
now that I look at it closer, where is file_arch coming from here? It looks like
it is only set by line 296 above.

https://codereview.chromium.org/15891011/diff/2001/native_client_sdk/src/tool...
native_client_sdk/src/tools/create_nmf.py:315: raise Error('Library not found
[%s]: %s' % (arch, filename))
seems like it would be useful to know which object needed this missing library.

[Sam Clegg, 2013-05-28 23:04:39]

https://codereview.chromium.org/15891011/diff/2001/native_client_sdk/src/tool...
File native_client_sdk/src/tools/create_nmf.py (right):

https://codereview.chromium.org/15891011/diff/2001/native_client_sdk/src/tool...
native_client_sdk/src/tools/create_nmf.py:283: found_basenames = {}
On 2013/05/28 22:52:41, binji wrote:
> = set(), you never use the value

Its used below, unless I'm misunderstanding.

https://codereview.chromium.org/15891011/diff/2001/native_client_sdk/src/tool...
native_client_sdk/src/tools/create_nmf.py:308: if not arch or arch == file_arch:
On 2013/05/28 22:52:41, binji wrote:
> might be nicer to use
> not(arch and arch != file_arch) to make the connection between this and the
> above check clearer.

Just removed this condition as the continue above means it is alwasy true.

https://codereview.chromium.org/15891011/diff/2001/native_client_sdk/src/tool...
native_client_sdk/src/tools/create_nmf.py:315: raise Error('Library not found
[%s]: %s' % (arch, filename))
On 2013/05/28 22:52:41, binji wrote:
> seems like it would be useful to know which object needed this missing
library.

Maybe.   That would require another re-factor since that information is not
available at this level.

[Sam Clegg, 2013-05-28 23:29:43]

Committed patchset #4 manually as r202688 (presubmit successful).

[M-A Ruel, 2013-11-26 18:19:41]

On 2013/05/28 23:29:43, Sam Clegg wrote:
> Committed patchset #4 manually as r202688 (presubmit successful).

I got the following failure on a local Try Server setup building on linux:

FAILED: cd ../../chrome/test/data/nacl; python
../../../../native_client_sdk/src/tools/create_nmf.py
../../../../out/Release/nacl_test_data/glibc/simple_glibc_x86_64.nexe
../../../../out/Release/nacl_test_data/glibc/simple_glibc_x86_32.nexe
"--objdump=../../../../native_client/toolchain/linux_x86/bin/x86_64-nacl-objdump"
"--output=../../../../out/Release/nacl_test_data/glibc/simple.nmf"
"--path-prefix=simple_libs"
"--stage-dependencies=../../../../out/Release/nacl_test_data/glibc"
"--library-path=../../../../native_client/toolchain/linux_x86/x86_64-nacl/lib"
"--library-path=../../../../out/Release/gen/tc_glibc/lib64"
"--library-path=../../../../native_client/toolchain/linux_x86/x86_64-nacl/lib32"
"--library-path=../../../../out/Release/gen/tc_glibc/lib32"
Traceback (most recent call last):
  File "../../../../native_client_sdk/src/tools/create_nmf.py", line 766, in
<module>
    rtn = main(sys.argv[1:])
  File "../../../../native_client_sdk/src/tools/create_nmf.py", line 750, in
main
    nmf.GetManifest()
  File "../../../../native_client_sdk/src/tools/create_nmf.py", line 518, in
GetManifest
    self._GenerateManifest()
  File "../../../../native_client_sdk/src/tools/create_nmf.py", line 470, in
_GenerateManifest
    needed = self.GetNeeded()
  File "../../../../native_client_sdk/src/tools/create_nmf.py", line 368, in
GetNeeded
    all_files, unexamined = self.GleanFromObjdump(self.main_files, None)
  File "../../../../native_client_sdk/src/tools/create_nmf.py", line 323, in
GleanFromObjdump
    match = '/'.join([file_arch, matched.group(1)])
UnboundLocalError: local variable 'file_arch' referenced before assignment

It looks like matched may first be None and then match on "matched =
NeededMatcher.match(line)", causing file_arch to not be yet defined or use the
value of the previous iteration.

Not sure why I got it here, maybe there's a race condition in the order of build
steps in the .gyp files?

[binji1, 2013-11-26 18:30:31]

I spoke to Sam, and neither of us have seen this before. We'll take a look.


On Tue, Nov 26, 2013 at 10:19 AM, <maruel@chromium.org> wrote:

> On 2013/05/28 23:29:43, Sam Clegg wrote:
>
>> Committed patchset #4 manually as r202688 (presubmit successful).
>>
>
> I got the following failure on a local Try Server setup building on linux:
>
> FAILED: cd ../../chrome/test/data/nacl; python
> ../../../../native_client_sdk/src/tools/create_nmf.py
> ../../../../out/Release/nacl_test_data/glibc/simple_glibc_x86_64.nexe
> ../../../../out/Release/nacl_test_data/glibc/simple_glibc_x86_32.nexe
> "--objdump=../../../../native_client/toolchain/linux_x86/
> bin/x86_64-nacl-objdump"
> "--output=../../../../out/Release/nacl_test_data/glibc/simple.nmf"
> "--path-prefix=simple_libs"
> "--stage-dependencies=../../../../out/Release/nacl_test_data/glibc"
> "--library-path=../../../../native_client/toolchain/linux_
> x86/x86_64-nacl/lib"
> "--library-path=../../../../out/Release/gen/tc_glibc/lib64"
> "--library-path=../../../../native_client/toolchain/linux_
> x86/x86_64-nacl/lib32"
> "--library-path=../../../../out/Release/gen/tc_glibc/lib32"
> Traceback (most recent call last):
>   File "../../../../native_client_sdk/src/tools/create_nmf.py", line 766,
> in
> <module>
>     rtn = main(sys.argv[1:])
>   File "../../../../native_client_sdk/src/tools/create_nmf.py", line 750,
> in
> main
>     nmf.GetManifest()
>   File "../../../../native_client_sdk/src/tools/create_nmf.py", line 518,
> in
> GetManifest
>     self._GenerateManifest()
>   File "../../../../native_client_sdk/src/tools/create_nmf.py", line 470,
> in
> _GenerateManifest
>     needed = self.GetNeeded()
>   File "../../../../native_client_sdk/src/tools/create_nmf.py", line 368,
> in
> GetNeeded
>     all_files, unexamined = self.GleanFromObjdump(self.main_files, None)
>   File "../../../../native_client_sdk/src/tools/create_nmf.py", line 323,
> in
> GleanFromObjdump
>     match = '/'.join([file_arch, matched.group(1)])
> UnboundLocalError: local variable 'file_arch' referenced before assignment
>
> It looks like matched may first be None and then match on "matched =
> NeededMatcher.match(line)", causing file_arch to not be yet defined or use
> the
> value of the previous iteration.
>
> Not sure why I got it here, maybe there's a race condition in the order of
> build
> steps in the .gyp files?
>
> https://codereview.chromium.org/15891011/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.