link with validator_ragel only in standalone binaries

Link with validator_ragel only in standalone binaries

That should be enough for the testing phase we are at now. Yet another difference is a bit ugly (esp in GYP), I am sure it will go away in months, not years.

This change unblocks NaCl DEPS roll in Chromium caused by commit on revision 8372. I performed the $TEST manually on linux and it passed.

BUG=http://code.google.com/p/nativeclient/issues/detail?id=2597
TEST=pull into chrome build, compare chrome binary size, should *not* change dramatically

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

[Mark Seaborn, 2012-04-25 16:48:43]

Can you set BUG=, please?

https://chromiumcodereview.appspot.com/10174016/diff/7/src/trusted/service_ru...
File src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp (right):

https://chromiumcodereview.appspot.com/10174016/diff/7/src/trusted/service_ru...
src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp:26: # addition to
the main validator for testing. For chrome we avoid this
Nit: Capitalise 'Chrome'

https://chromiumcodereview.appspot.com/10174016/diff/7/src/trusted/service_ru...
src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp:41:
['nacl_standalone==0 and target_arch=="ia32"', {
Can you simplify this to (in pseudocode):

if arch == 'ia32':
  dependencies += ['ncvalidate_x86_32']
if arch == 'ia32' and nacl_standalone==1:
  dependencies += ['dfa_validate_x86_32']

i.e. No need to list ncvalidate_x86_32 twice.

Same for x86-64.

https://chromiumcodereview.appspot.com/10174016/diff/7/src/trusted/service_ru...
src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp:70: 'target_name':
'service_runtime_x86_common64',
You shouldn't duplicating the whole 'service_runtime_x86_common64' target.  Can
you put the conditional inside the target instead, please?

https://chromiumcodereview.appspot.com/10174016/diff/7/src/trusted/service_ru...
File src/trusted/service_runtime/sel_validate_image.c (right):

https://chromiumcodereview.appspot.com/10174016/diff/7/src/trusted/service_ru...
src/trusted/service_runtime/sel_validate_image.c:37: #if defined(__arm__) ||
!defined (NACL_STANDALONE)
No space after 'defined'.  Can you add a comment here too to say that we are
avoiding linking two validators into Chromium for size reasons?  This is the
part that really determines whether the validator will get included, more than
the Gyp file.

[pasko-google - do not use, 2012-04-25 17:03:21]

https://chromiumcodereview.appspot.com/10174016/diff/7/src/trusted/service_ru...
File src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp (right):

https://chromiumcodereview.appspot.com/10174016/diff/7/src/trusted/service_ru...
src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp:41:
['nacl_standalone==0 and target_arch=="ia32"', {
On 2012/04/25 16:48:43, Mark Seaborn wrote:
> Can you simplify this to (in pseudocode):
> 
> if arch == 'ia32':
>   dependencies += ['ncvalidate_x86_32']
> if arch == 'ia32' and nacl_standalone==1:
>   dependencies += ['dfa_validate_x86_32']
> 
> i.e. No need to list ncvalidate_x86_32 twice.
> 
> Same for x86-64.

I am not sure that conditions inside conditions are intended to work. Are there
examples of this in Chrome?

Do you mean creating new variables of list type? Are they supported in GYP?

I am really a noob in GYP. Would be happy to learn, but without docs near my
sight you'd need to provide some more information.

[pasko-google - do not use, 2012-04-25 17:19:01]

On 2012/04/25 16:48:43, Mark Seaborn wrote:
> Can you set BUG=, please?

I have set it to "none", is there a better bug for this?
Or should I write in English that blah deps roll is blocked? That thing is
written above, I can repeat, if you want. The intention is not clear to me
though.

[Mark Seaborn, 2012-04-25 17:23:57]

On 25 April 2012 10:19, <pasko@google.com> wrote:

> On 2012/04/25 16:48:43, Mark Seaborn wrote:
>
>> Can you set BUG=, please?
>>
>
> I have set it to "none", is there a better bug for this?
> Or should I write in English that blah deps roll is blocked? That thing is
> written above, I can repeat, if you want. The intention is not clear to me
> though.
>

A tracking bug for the Ragel/DFA validator would do.  You're addressing a
problem introduced by https://chromiumcodereview.appspot.com/10070010, so
you can use the same bug number, i.e.
http://code.google.com/p/nativeclient/issues/detail?id=2597.

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[pasko-google - do not use, 2012-04-25 17:32:03]

https://chromiumcodereview.appspot.com/10174016/diff/7/src/trusted/service_ru...
File src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp (right):

https://chromiumcodereview.appspot.com/10174016/diff/7/src/trusted/service_ru...
src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp:26: # addition to
the main validator for testing. For chrome we avoid this
On 2012/04/25 16:48:43, Mark Seaborn wrote:
> Nit: Capitalise 'Chrome'

Done.

https://chromiumcodereview.appspot.com/10174016/diff/7/src/trusted/service_ru...
File src/trusted/service_runtime/sel_validate_image.c (right):

https://chromiumcodereview.appspot.com/10174016/diff/7/src/trusted/service_ru...
src/trusted/service_runtime/sel_validate_image.c:37: #if defined(__arm__) ||
!defined (NACL_STANDALONE)
On 2012/04/25 16:48:43, Mark Seaborn wrote:
> No space after 'defined'.  Can you add a comment here too to say that we are
> avoiding linking two validators into Chromium for size reasons?  This is the
> part that really determines whether the validator will get included, more than
> the Gyp file.

Done.

[Mark Seaborn, 2012-04-25 17:42:02]

On 25 April 2012 10:03, <pasko@google.com> wrote:

> https://chromiumcodereview.**appspot.com/10174016/diff/7/**
>
src/trusted/service_runtime/**arch/x86/service_runtime_x86.**gyp<https://chromiumcodereview.appspot.com/10174016/diff/7/src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp>
> File src/trusted/service_runtime/**arch/x86/service_runtime_x86.**gyp
> (right):
>
> https://chromiumcodereview.**appspot.com/10174016/diff/7/**
>
src/trusted/service_runtime/**arch/x86/service_runtime_x86.**gyp#newcode41<https://chromiumcodereview.appspot.com/10174016/diff/7/src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp#newcode41>
> src/trusted/service_runtime/**arch/x86/service_runtime_x86.**gyp:41:
> ['nacl_standalone==0 and target_arch=="ia32"', {
> On 2012/04/25 16:48:43, Mark Seaborn wrote:
>
>> Can you simplify this to (in pseudocode):
>>
>
>  if arch == 'ia32':
>>   dependencies += ['ncvalidate_x86_32']
>> if arch == 'ia32' and nacl_standalone==1:
>>   dependencies += ['dfa_validate_x86_32']
>>
>
>  i.e. No need to list ncvalidate_x86_32 twice.
>>
>
>  Same for x86-64.
>>
>
> I am not sure that conditions inside conditions are intended to work.
> Are there examples of this in Chrome?
>

The part of my comments you're quoting does not ask for conditions in
conditions, but my later comment did.  But yes, conditions inside
conditions work.  There are examples in chrome/chrome_browser.gypi.

Do you mean creating new variables of list type? Are they supported in
> GYP?
>

No, the above was pseudocode because I was being lazy in not writing out
the full Gyp.

What I meant above is that you should write:

        ['target_arch=="ia32"', {
          'dependencies': [

'<(DEPTH)/native_client/src/trusted/validator/x86/32/validator_x86_32.gyp:ncvalidate_x86_32',
          ],
        }],
        ['target_arch=="ia32" and nacl_standalone==1', {
          'dependencies': [

'<(DEPTH)/native_client/src/trusted/validator_ragel/dfa_validator_x86_32.gyp:dfa_validate_x86_32',
          ],
        }],

For the later part of the file I meant that you should write:

  'conditions': [
    ['OS=="win"', {
      'targets': [
        {
          'target_name': 'service_runtime_x86_common64',
          'type': 'static_library',
          'variables': {
            'target_base': 'srt_x86_cmn',
            'win_target': 'x64',
          },
          'dependencies': [

'<(DEPTH)/native_client/src/trusted/validator/x86/64/validator_x86_64.gyp:ncvalidate_x86_64',
          ],
          'conditions': [
            ['nacl_standalone==1', {
               'dependencies': [

'<(DEPTH)/native_client/src/trusted/validator_ragel/dfa_validator_x86_32.gyp:dfa_validate_x86_32',
               ],
            }],
          },
        },
      ],
    }],
  ],

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[pasko-google - do not use, 2012-04-26 12:22:50]

On 2012/04/25 17:42:02, Mark Seaborn wrote:
> On 25 April 2012 10:03, <mailto:pasko@google.com> wrote:
> 
> > https://chromiumcodereview.**appspot.com/10174016/diff/7/**
> >
>
src/trusted/service_runtime/**arch/x86/service_runtime_x86.**gyp<https://chromiumcodereview.appspot.com/10174016/diff/7/src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp>
> > File src/trusted/service_runtime/**arch/x86/service_runtime_x86.**gyp
> > (right):
> >
> > https://chromiumcodereview.**appspot.com/10174016/diff/7/**
> >
>
src/trusted/service_runtime/**arch/x86/service_runtime_x86.**gyp#newcode41<https://chromiumcodereview.appspot.com/10174016/diff/7/src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp#newcode41>
> > src/trusted/service_runtime/**arch/x86/service_runtime_x86.**gyp:41:
> > ['nacl_standalone==0 and target_arch=="ia32"', {
> > On 2012/04/25 16:48:43, Mark Seaborn wrote:
> >
> >> Can you simplify this to (in pseudocode):
> >>
> >
> >  if arch == 'ia32':
> >>   dependencies += ['ncvalidate_x86_32']
> >> if arch == 'ia32' and nacl_standalone==1:
> >>   dependencies += ['dfa_validate_x86_32']
> >>
> >
> >  i.e. No need to list ncvalidate_x86_32 twice.
> >>
> >
> >  Same for x86-64.
> >>
> >
> > I am not sure that conditions inside conditions are intended to work.
> > Are there examples of this in Chrome?
> >
> 
> The part of my comments you're quoting does not ask for conditions in
> conditions, but my later comment did.  But yes, conditions inside
> conditions work.  There are examples in chrome/chrome_browser.gypi.
> 
> Do you mean creating new variables of list type? Are they supported in
> > GYP?
> >
> 
> No, the above was pseudocode because I was being lazy in not writing out
> the full Gyp.
> 
> What I meant above is that you should write:
> 
>         ['target_arch=="ia32"', {
>           'dependencies': [
> 
>
'<(DEPTH)/native_client/src/trusted/validator/x86/32/validator_x86_32.gyp:ncvalidate_x86_32',
>           ],
>         }],
>         ['target_arch=="ia32" and nacl_standalone==1', {
>           'dependencies': [
> 
>
'<(DEPTH)/native_client/src/trusted/validator_ragel/dfa_validator_x86_32.gyp:dfa_validate_x86_32',
>           ],
>         }],
> 
> For the later part of the file I meant that you should write:
> 
>   'conditions': [
>     ['OS=="win"', {
>       'targets': [
>         {
>           'target_name': 'service_runtime_x86_common64',
>           'type': 'static_library',
>           'variables': {
>             'target_base': 'srt_x86_cmn',
>             'win_target': 'x64',
>           },
>           'dependencies': [
> 
>
'<(DEPTH)/native_client/src/trusted/validator/x86/64/validator_x86_64.gyp:ncvalidate_x86_64',
>           ],
>           'conditions': [
>             ['nacl_standalone==1', {
>                'dependencies': [
> 
>
'<(DEPTH)/native_client/src/trusted/validator_ragel/dfa_validator_x86_32.gyp:dfa_validate_x86_32',
>                ],
>             }],
>           },
>         },
>       ],
>     }],
>   ],

Done. Thanks.

I did not know two things about GYP files:
1. 'dependencies' are added each time, not replaced
2. 'conditions' in 'conditions' are possible

now the changes are more compact which makes me happier

[pasko-google - do not use, 2012-04-26 12:24:57]

PTAL

[Mark Seaborn, 2012-04-26 15:10:54]

LGTM

https://chromiumcodereview.appspot.com/10174016/diff/18001/src/trusted/servic...
File src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp (right):

https://chromiumcodereview.appspot.com/10174016/diff/18001/src/trusted/servic...
src/trusted/service_runtime/arch/x86/service_runtime_x86.gyp:81: ],
Indent -2