Bug 90258 – [CMAKE] Add GENERATE_BINDINGS macro to share the codes which use generate-bindings.pl.

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

[Ryuan Choi]

As following a comment of Bug 87659,
add GENERATE_BINDINGS macro to share the codes which use generate-bindings.pl.

[Ryuan Choi]

Created attachment 150114 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 150114 [details]
Patch

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

Thanks for working on this. git grep tells me that UseV8.cmake and PlatformBlackBerry.cmake also call generate-bindings.pl, it'd be good if you could generalize the macro to be able to use it in those files as well.

> Source/WebCore/UseJSC.cmake:319
> +    SET(WEBCORE_IDL_FILES_LIST ${WEBCORE_IDL_FILES_LIST} ${WEBCORE_DIR}/${_idl})

Is this change related?

> Source/WebCore/UseJSC.cmake:324
> +    SET(WebCoreTestSupport_IDL_FILES_LIST ${WebCoreTestSupport_IDL_FILES_LIST} ${WEBCORE_DIR}/${_idl})

Ditto.

> Source/cmake/WebKitMacros.cmake:27
> +MACRO (GENERATE_BINDINGS _output_sources _input_files _idl_includes _destination _generator)

It'd be good if you could add some documentation here. See, for example, http://trac.webkit.org/browser/trunk/Source/cmake/FindGStreamer.cmake#L54

> Source/cmake/WebKitMacros.cmake:34
> +    FOREACH (f ${ARGN})
> +        IF (NOT _supplemental_dependency)
> +            SET(_supplemental_dependency --supplementalDependencyFile ${f})
> +        ENDIF ()
> +    ENDFOREACH ()

So there can be at most one supplemental dependency?

[Ryuan Choi]

(In reply to comment #2)
> (From update of attachment 150114 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150114&action=review
> 
> Thanks for working on this. git grep tells me that UseV8.cmake and PlatformBlackBerry.cmake also call generate-bindings.pl, it'd be good if you could generalize the macro to be able to use it in those files as well.
> 
OK, I will.

> > Source/WebCore/UseJSC.cmake:319
> > +    SET(WEBCORE_IDL_FILES_LIST ${WEBCORE_IDL_FILES_LIST} ${WEBCORE_DIR}/${_idl})
> 
> Is this change related?
> 
> > Source/WebCore/UseJSC.cmake:324
> > +    SET(WebCoreTestSupport_IDL_FILES_LIST ${WebCoreTestSupport_IDL_FILES_LIST} ${WEBCORE_DIR}/${_idl})
> 
> Ditto.
> 
It's because GENERATE_BINDINGS macro should not limit the IDL path to WEBCORE_DIR.

As alternatives, I think that each item in IDL lists should be absolute path.
I can create new bug for it because this changes are huge.

> > Source/cmake/WebKitMacros.cmake:27
> > +MACRO (GENERATE_BINDINGS _output_sources _input_files _idl_includes _destination _generator)
> 
> It'd be good if you could add some documentation here. See, for example, http://trac.webkit.org/browser/trunk/Source/cmake/FindGStreamer.cmake#L54

Thank you, I will try.

> 
> > Source/cmake/WebKitMacros.cmake:34
> > +    FOREACH (f ${ARGN})
> > +        IF (NOT _supplemental_dependency)
> > +            SET(_supplemental_dependency --supplementalDependencyFile ${f})
> > +        ENDIF ()
> > +    ENDFOREACH ()
> 
> So there can be at most one supplemental dependency?

I don't know supplemental dependency enough.
But If I am right, there is one supplemental dependency file.
I want that this is an optional argument.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 150114 [details]
Patch

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

>>> Source/WebCore/UseJSC.cmake:319
>>> +    SET(WEBCORE_IDL_FILES_LIST ${WEBCORE_IDL_FILES_LIST} ${WEBCORE_DIR}/${_idl})
>> 
>> Is this change related?
> 
> It's because GENERATE_BINDINGS macro should not limit the IDL path to WEBCORE_DIR.
> 
> As alternatives, I think that each item in IDL lists should be absolute path.
> I can create new bug for it because this changes are huge.

Well, it's easier to adjust the macro. If necessary, pass the base directory the script should be called from.

>>> Source/cmake/WebKitMacros.cmake:34
>>> +    ENDFOREACH ()
>> 
>> So there can be at most one supplemental dependency?
> 
> I don't know supplemental dependency enough.
> But If I am right, there is one supplemental dependency file.
> I want that this is an optional argument.

If there can be only one (please check the supplemental page in Trac), you can simply get the first argument instead of needlessly looping.

[Ryuan Choi]

Created attachment 150698 [details]
Patch

[Dominik Röttsches (drott)]

I gave this a try, works fine locally.

[Ryuan Choi]

(In reply to comment #4)
> (From update of attachment 150114 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150114&action=review
> 
> >>> Source/WebCore/UseJSC.cmake:319
> >>> +    SET(WEBCORE_IDL_FILES_LIST ${WEBCORE_IDL_FILES_LIST} ${WEBCORE_DIR}/${_idl})
> >> 
> >> Is this change related?
> > 
> > It's because GENERATE_BINDINGS macro should not limit the IDL path to WEBCORE_DIR.
> > 
> > As alternatives, I think that each item in IDL lists should be absolute path.
> > I can create new bug for it because this changes are huge.
> 
> Well, it's easier to adjust the macro. If necessary, pass the base directory the script should be called from.

OK, I tried like you mentioned.

> 
> >>> Source/cmake/WebKitMacros.cmake:34
> >>> +    ENDFOREACH ()
> >> 
> >> So there can be at most one supplemental dependency?
> > 
> > I don't know supplemental dependency enough.
> > But If I am right, there is one supplemental dependency file.
> > I want that this is an optional argument.
> 
> If there can be only one (please check the supplemental page in Trac), you can simply get the first argument instead of needlessly looping.

I checked scripts and supplemental file can be only one.
I changed little bit.

[Rob Buis]

Comment on attachment 150698 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 150698 [details]
Patch

Clearing flags on attachment: 150698

Committed r121857: <http://trac.webkit.org/changeset/121857>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dominik Röttsches (drott)]

Could it be that this causes the generated sources now to be always rebuilt?

[Ryuan Choi]

(In reply to comment #11)
> Could it be that this causes the generated sources now to be always rebuilt?

Oops I found typo. MAIN_DEPEND'D'ENCY.
I will fix it.