Bug 55196 – Add necessary build steps to JavaScriptGlue GYP project.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dimitri Glazkov (Google)]

Add necessary build steps to JavaScriptGlue GYP project.

[Dimitri Glazkov (Google)]

Created attachment 83750 [details]
WIP: Not building yet, capturing progress so far.

[Adam Barth]

Comment on attachment 83750 [details]
WIP: Not building yet, capturing progress so far.

View in context: https://bugs.webkit.org/attachment.cgi?id=83750&action=review

> Source/JavaScriptGlue/gyp/JavaScriptGlue.gyp:20
> +        '../Info.plist',

Should this be in JavaScriptGlue.gypi?

> Source/JavaScriptGlue/gyp/JavaScriptGlue.gyp:36
> +          'action': [ 'if [ -f ../../Tools/Scripts/check-for-global-initializers ]; then\n    ../../Tools/Scripts/check-for-global-initializers || exit $?\nfi";' ],

I'm not sure whether it's better to put these inline in the gyp or to having them be external sh files.  In JavaScriptCore.gyp, I went with the external sh files.  I'm not sure this matters at this stage.  Just thinking out loud.

> Source/JavaScriptGlue/gyp/JavaScriptGlue.gyp:217
> +          'inputs': [ '$(SRCROOT)/Configurations/Version.xcconfig' ],

This won't work properly because the current plan is to suck all the xcconfig files into the gyp file.  If there's a way to grab the xcconfig files from the file system, that might be better.

> Source/JavaScriptGlue/gyp/JavaScriptGlue.gyp:218
> +          'outputs': [ '$(SRCROOT)/Info.plist' ],

Probably better to not use $ variables in inputs and outputs.  $ variables are only understood by Xcode.  If we use < variables, they'll be understood by gyp.

[Dimitri Glazkov (Google)]

Created attachment 83830 [details]
Patch

[Dimitri Glazkov (Google)]

(In reply to comment #2)
> (From update of attachment 83750 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83750&action=review
> 
> > Source/JavaScriptGlue/gyp/JavaScriptGlue.gyp:20
> > +        '../Info.plist',
> 
> Should this be in JavaScriptGlue.gypi?

Done.

> 
> > Source/JavaScriptGlue/gyp/JavaScriptGlue.gyp:36
> > +          'action': [ 'if [ -f ../../Tools/Scripts/check-for-global-initializers ]; then\n    ../../Tools/Scripts/check-for-global-initializers || exit $?\nfi";' ],
> 
> I'm not sure whether it's better to put these inline in the gyp or to having them be external sh files.  In JavaScriptCore.gyp, I went with the external sh files.  I'm not sure this matters at this stage.  Just thinking out loud.

Pulled out.

> 
> > Source/JavaScriptGlue/gyp/JavaScriptGlue.gyp:217
> > +          'inputs': [ '$(SRCROOT)/Configurations/Version.xcconfig' ],
> 
> This won't work properly because the current plan is to suck all the xcconfig files into the gyp file.  If there's a way to grab the xcconfig files from the file system, that might be better.

Turns out, it's not necessary for the script. I was just mimicking the original project file.

> 
> > Source/JavaScriptGlue/gyp/JavaScriptGlue.gyp:218
> > +          'outputs': [ '$(SRCROOT)/Info.plist' ],
> 
> Probably better to not use $ variables in inputs and outputs.  $ variables are only understood by Xcode.  If we use < variables, they'll be understood by gyp.

Done.

[Adam Barth]

Comment on attachment 83830 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83830&action=review

This is great!  Thanks.

> Source/JavaScriptGlue/ChangeLog:12
> +        * gyp/remove-headers-if-needed.sh: Added.
> +        * gyp/run-if-exists.sh: Added.
> +        * gyp/update-info-plist.sh: Added.

We'll probably want to move these up in the source tree so they can be shared by multiple projects, but we can do that once we tackle the sharing problems.

> Source/JavaScriptGlue/gyp/run-if-exists.sh:4
> +if [ -f ../../Tools/Scripts/$1 ]; then
> +    ../../Tools/Scripts/$1 || exit $?;

It would be nice if we didn't have to hard-code the "../.." here.  Maybe run-if-exists should take the fully relative path and then we can let GYP magic figure out the depth?

[Adam Barth]

Committed r79890: <http://trac.webkit.org/changeset/79890>