Add test running support for tests generated as pexes.

Add test running support for tests generated as pexes. 
Hooks into CommandSelLdrTestNacl et al to make the translation part of the test.

R=robertm,jvoung,sehr,Karl
BUG= http://code.google.com/p/nativeclient/issues/detail?id=2609 
TEST=smoke_tests,large_tests

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=8014

[(google.com) Derek Schuff, 2012-03-13 20:36:22]

just the run without the rename

[robertm, 2012-03-13 20:57:50]

I wonder would it simplify things for you if we simple did not support some
tests when we run them with prebuild pexe's.

I mention the validation tests below,
but this also applies to the toolchain tests which would
run the translator in a way it will never be called
from the browser.

https://chromiumcodereview.appspot.com/9696046/diff/1/SConstruct
File SConstruct (right):

https://chromiumcodereview.appspot.com/9696046/diff/1/SConstruct#newcode1195
SConstruct:1195: if (env.Bit('pnacl_stop_with_pexe') and
env['NACL_BUILD_FAMILY'] != 'TRUSTED'):
I think it would be ok to simple skip the validator tests when we are in
pnacl_stop_with_pexe.

The validation will be tested on other bots, so we do not lose coverage.

https://chromiumcodereview.appspot.com/9696046/diff/1/site_scons/site_tools/n...
File site_scons/site_tools/naclsdk.py (right):

https://chromiumcodereview.appspot.com/9696046/diff/1/site_scons/site_tools/n...
site_scons/site_tools/naclsdk.py:217: arch_flag = ' -arch %s' % arch
having two arch flags is a little confusing.
Is there a way to have only one give that
pnacl_stop_with_pexe will be false on the bots 
which run the translator.
Otherwise, add a comment

[jvoung - send to chromium..., 2012-03-13 20:58:31]

https://chromiumcodereview.appspot.com/9696046/diff/1/SConstruct
File SConstruct (right):

https://chromiumcodereview.appspot.com/9696046/diff/1/SConstruct#newcode1195
SConstruct:1195: if (env.Bit('pnacl_stop_with_pexe') and
env['NACL_BUILD_FAMILY'] != 'TRUSTED'):
80 col (barely)

https://chromiumcodereview.appspot.com/9696046/diff/1/SConstruct#newcode1967
SConstruct:1967: translate_command = ' '.join([env['TRANSLATE']] + trans_flags +
[pexe_name,
Would it work if you let scons do the env interpolation on the command string?
E.g., something like:

'${TRANSLATE} ${TRANSLATEFLAGS} %s -o %s -Wl,-L ${LIB_DIR}' % (pexe_name,
nexe_name)

?  Might be more readable.

https://chromiumcodereview.appspot.com/9696046/diff/1/SConstruct#newcode1996
SConstruct:1996: if (env.Bit('pnacl_stop_with_pexe') and
env['NACL_BUILD_FAMILY'] != 'TRUSTED'):
80 col (barely)

https://chromiumcodereview.appspot.com/9696046/diff/1/site_scons/site_tools/n...
File site_scons/site_tools/naclsdk.py (right):

https://chromiumcodereview.appspot.com/9696046/diff/1/site_scons/site_tools/n...
site_scons/site_tools/naclsdk.py:326: TRANSLATE=pnacl_translate + arch_flag +
pnacl_translate_flags,
Actually, it looks like you have now baked in translate flags so you don't need
to reference env['TRANSLATEFLAGS'] anymore in the SConstruct file patch?

Err... n/m that is added by the test nacl.scons files.

[(google.com) Derek Schuff, 2012-03-13 21:23:03]

PTAL

https://chromiumcodereview.appspot.com/9696046/diff/1/SConstruct
File SConstruct (right):

https://chromiumcodereview.appspot.com/9696046/diff/1/SConstruct#newcode1195
SConstruct:1195: if (env.Bit('pnacl_stop_with_pexe') and
env['NACL_BUILD_FAMILY'] != 'TRUSTED'):
hm. since the only place this is used is the whole archive test, and presumably
just to make sure the whole archive is valid, i think i'll just skip this run in
that mode. 
On 2012/03/13 20:57:51, robertm wrote:
> I think it would be ok to simple skip the validator tests when we are in
> pnacl_stop_with_pexe.
> 
> The validation will be tested on other bots, so we do not lose coverage.

https://chromiumcodereview.appspot.com/9696046/diff/1/SConstruct#newcode1967
SConstruct:1967: translate_command = ' '.join([env['TRANSLATE']] + trans_flags +
[pexe_name,
yeah, that works.
On 2012/03/13 20:58:31, jvoung wrote:
> Would it work if you let scons do the env interpolation on the command string?
> E.g., something like:
> 
> '${TRANSLATE} ${TRANSLATEFLAGS} %s -o %s -Wl,-L ${LIB_DIR}' % (pexe_name,
> nexe_name)
> 
> ?  Might be more readable.

https://chromiumcodereview.appspot.com/9696046/diff/1/SConstruct#newcode1996
SConstruct:1996: if (env.Bit('pnacl_stop_with_pexe') and
env['NACL_BUILD_FAMILY'] != 'TRUSTED'):
punting this because the name change fixes it :P
On 2012/03/13 20:58:31, jvoung wrote:
> 80 col (barely)

https://chromiumcodereview.appspot.com/9696046/diff/1/site_scons/site_tools/n...
File site_scons/site_tools/naclsdk.py (right):

https://chromiumcodereview.appspot.com/9696046/diff/1/site_scons/site_tools/n...
site_scons/site_tools/naclsdk.py:217: arch_flag = ' -arch %s' % arch
Actually I think the right thing to do is have sandboxed translation only use
the pexe mode. but that's a future CL.
I can revisit this flag thing in that CL as well.

On 2012/03/13 20:57:51, robertm wrote:
> having two arch flags is a little confusing.
> Is there a way to have only one give that
> pnacl_stop_with_pexe will be false on the bots 
> which run the translator.
> Otherwise, add a comment

https://chromiumcodereview.appspot.com/9696046/diff/1/site_scons/site_tools/n...
site_scons/site_tools/naclsdk.py:326: TRANSLATE=pnacl_translate + arch_flag +
pnacl_translate_flags,
yes, it's just those tests that need to add extra translate flags.
On 2012/03/13 20:58:31, jvoung wrote:
> Actually, it looks like you have now baked in translate flags so you don't
need
> to reference env['TRANSLATEFLAGS'] anymore in the SConstruct file patch?
> 
> Err... n/m that is added by the test nacl.scons files.

[jvoung - send to chromium..., 2012-03-13 21:41:23]

lgtm

https://chromiumcodereview.appspot.com/9696046/diff/1/SConstruct
File SConstruct (right):

https://chromiumcodereview.appspot.com/9696046/diff/1/SConstruct#newcode1996
SConstruct:1996: if (env.Bit('pnacl_stop_with_pexe') and
env['NACL_BUILD_FAMILY'] != 'TRUSTED'):
On 2012/03/13 21:23:03, Derek Schuff wrote:
> punting this because the name change fixes it :P
> On 2012/03/13 20:58:31, jvoung wrote:
> > 80 col (barely)
> 

=) sounds good.

https://chromiumcodereview.appspot.com/9696046/diff/1/site_scons/site_tools/n...
File site_scons/site_tools/naclsdk.py (right):

https://chromiumcodereview.appspot.com/9696046/diff/1/site_scons/site_tools/n...
site_scons/site_tools/naclsdk.py:326: TRANSLATE=pnacl_translate + arch_flag +
pnacl_translate_flags,
On 2012/03/13 21:23:03, Derek Schuff wrote:
> yes, it's just those tests that need to add extra translate flags.
> On 2012/03/13 20:58:31, jvoung wrote:
> > Actually, it looks like you have now baked in translate flags so you don't
> need
> > to reference env['TRANSLATEFLAGS'] anymore in the SConstruct file patch?
> > 
> > Err... n/m that is added by the test nacl.scons files.
> 


Can you initialize it to an empty string '' and add a comment that tests may add
to this?

[robertm, 2012-03-13 21:53:15]

https://chromiumcodereview.appspot.com/9696046/diff/7004/SConstruct
File SConstruct (right):

https://chromiumcodereview.appspot.com/9696046/diff/7004/SConstruct#newcode1195
SConstruct:1195: if env.Bit('pnacl_stop_with_pexe'):
I am not a big fan of the "late kill",
it would be more  intuitive to move the test up into the 
corresponding scons file if it is only a few lines of change there.

[(google.com) Derek Schuff, 2012-03-13 21:59:27]

https://chromiumcodereview.appspot.com/9696046/diff/7004/SConstruct
File SConstruct (right):

https://chromiumcodereview.appspot.com/9696046/diff/7004/SConstruct#newcode1195
SConstruct:1195: if env.Bit('pnacl_stop_with_pexe'):
Maybe... until someone else tries to add another validator test and then it
breaks or requires them to write more boilerplate in their scons file?

On 2012/03/13 21:53:15, robertm wrote:
> I am not a big fan of the "late kill",
> it would be more  intuitive to move the test up into the 
> corresponding scons file if it is only a few lines of change there.

[Mark Seaborn, 2012-03-13 22:16:55]

Drive-by review.  Here's a change to fix the problems:
https://chromiumcodereview.appspot.com/9695064/

https://chromiumcodereview.appspot.com/9696046/diff/7004/SConstruct
File SConstruct (right):

https://chromiumcodereview.appspot.com/9696046/diff/7004/SConstruct#newcode1956
SConstruct:1956: # Translate the given pexe. Return the name of the translated
nexe and
Sentence not completed.

https://chromiumcodereview.appspot.com/9696046/diff/7004/SConstruct#newcode1960
SConstruct:1960: trans_flags = []
trans_flag is not used.

https://chromiumcodereview.appspot.com/9696046/diff/7004/SConstruct#newcode1965
SConstruct:1965: return nexe_name, pexe_node
In the call sites, nexe_name is not used.

https://chromiumcodereview.appspot.com/9696046/diff/7004/site_scons/site_tool...
File site_scons/site_tools/naclsdk.py (right):

https://chromiumcodereview.appspot.com/9696046/diff/7004/site_scons/site_tool...
site_scons/site_tools/naclsdk.py:222: 
Style nit: Shouldn't have >1 blank line inside a function

https://chromiumcodereview.appspot.com/9696046/diff/7004/tests/minnacl/nacl.s...
File tests/minnacl/nacl.scons (right):

https://chromiumcodereview.appspot.com/9696046/diff/7004/tests/minnacl/nacl.s...
tests/minnacl/nacl.scons:55: extra_deps = []
What's with this extra_deps when SCons will track dependencies automatically
when you use target objects?