Bug 75345 – Enable the [Supplemental] IDL on CMake

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Kentaro Hara]

We have enabled the [Supplemental] IDL on Chromium, AppleWebKit, GTK and Qt, and are planning to enable it on all build systems (Meta bug 72138). In this bug, we enable it on Efl.

[Kentaro Hara]

Created attachment 120784 [details]
WIP patch to see if build passes

[Gyuyoung Kim]

Comment on attachment 120784 [details]
WIP patch to see if build passes

Attachment 120784 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11058120

[Kentaro Hara]

Created attachment 120785 [details]
WIP patch to see if build passes

[Gyuyoung Kim]

Comment on attachment 120785 [details]
WIP patch to see if build passes

Attachment 120785 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10899111

[Kentaro Hara]

Created attachment 120793 [details]
WIP patch to see if build passes

[Gyuyoung Kim]

Comment on attachment 120793 [details]
WIP patch to see if build passes

Attachment 120793 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10899167

[Kentaro Hara]

Created attachment 120794 [details]
WIP patch to see if build passes

[Gyuyoung Kim]

Comment on attachment 120794 [details]
WIP patch to see if build passes

Attachment 120794 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11056155

[Kentaro Hara]

Created attachment 120796 [details]
WIP patch to see if build passes

[Raphael Kubo da Costa (:rakuco)]

CC'ing dbates and paroga, as this is common infrastructure used by all ports which use CMake.

[Patrick R. Gansterer]

Comment on attachment 120796 [details]
WIP patch to see if build passes

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

> Source/WebCore/UseJSC.cmake:276
> +    COMMAND echo ${IDL_FILES_LIST} | tr " " \\n > ${IDL_FILES_TMP}
> +    COMMAND cat ${IDL_FILES_TMP}

the commands won't work on windows platform (e.g. ther is no cat)
Maybe this can be written with the CMake FILE() and STRING() commands
or better: add the required functionality into the perl scripts

[Kentaro Hara]

Comment on attachment 120796 [details]
WIP patch to see if build passes

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

>> Source/WebCore/UseJSC.cmake:276
>> +    COMMAND cat ${IDL_FILES_TMP}
> 
> the commands won't work on windows platform (e.g. ther is no cat)
> Maybe this can be written with the CMake FILE() and STRING() commands
> or better: add the required functionality into the perl scripts

Actually, this cat is just for debugging. I'll remove it in the following patch. Thanks!

[Kentaro Hara]

Created attachment 120798 [details]
patch for review

[Patrick R. Gansterer]

Comment on attachment 120798 [details]
patch for review

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

> Source/WebCore/ChangeLog:3
> +        Enable the [Supplemental] IDL on Efl

maybe you can change the efl to CMake?

> Source/WebCore/UseJSC.cmake:275
> +    COMMAND echo ${IDL_FILES_LIST} | tr " " \\n > ${IDL_FILES_TMP}

same problem for echo and tr, they don't exist on windows :-/

> Source/WebCore/UseV8.cmake:263
> +    COMMAND echo ${IDL_FILES_LIST} | tr " " \\n > ${IDL_FILES_TMP}

here too

[Kentaro Hara]

(In reply to comment #14)
> (From update of attachment 120798 [details])

Thanks, Patrick!

> > Source/WebCore/UseJSC.cmake:275
> > +    COMMAND echo ${IDL_FILES_LIST} | tr " " \\n > ${IDL_FILES_TMP}
> 
> same problem for echo and tr, they don't exist on windows :-/
> 
> > Source/WebCore/UseV8.cmake:263
> > +    COMMAND echo ${IDL_FILES_LIST} | tr " " \\n > ${IDL_FILES_TMP}
> 
> here too

OK, I'll fix it in the next patch.

By the way, maybe should we need to make a change on PlatformBlackBerry.cmake at the same time? In other words, if we make a change on CMakefile.list, the change will affect UseJSC.cmake, UseV8.cmake and PlatformBlackBerry.cmake, right?

But I'm not sure how I can confirm that my change on PlatformBlackBerry.cmake would be correct or not (since I do not have a BlackBerry build environment around me)...

[Kentaro Hara]

Created attachment 120800 [details]
Patch

[Patrick R. Gansterer]

Comment on attachment 120800 [details]
Patch

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

> Source/WebCore/UseJSC.cmake:270
> +FILE(WRITE ${IDL_FILES_TMP} ${IDL_FILES_LIST})

does not seem like a problem, but only FYI: This way IDL_FILES_TMP will be written at _every_ CMake run, so nothing should depend on it to avoid unnecessary rebuilds.

> Source/WebCore/UseJSC.cmake:275
> +    MAIN_DEPENDENCY ${SCRIPTS_RESOLVE_SUPPLEMENTAL} ${WebCore_IDL_FILES}
> +    DEPENDS ${WEBCORE_DIR}/bindings/scripts/resolve-supplemental.pl ${SCRIPTS_RESOLVE_SUPPLEMENTAL} ${WebCore_IDL_FILES}

sorry, missed in the last patch:
MAIN_DEPENDENCY should contain only _ONE_ file. The "main dependency" will be used in IDE to provide a nicer interface. You can then right click on a IDL file and generate the corresponding h and cpp files.
The MAIN_DEPENDENCY should be used only once in the project.
All additional dependencies should be added to DEPENDS. The MAIN_DEPENDENCY does not need to be added to DEPENDS.

> Source/WebCore/UseJSC.cmake:283
> +        MAIN_DEPENDENCY ${_file} ${SUPPLEMENTAL_DEPENDENCY_FILE}

too

> Source/WebCore/UseV8.cmake:263
> +    MAIN_DEPENDENCY ${SCRIPTS_RESOLVE_SUPPLEMENTAL} ${WebCore_IDL_FILES}
> +    DEPENDS ${WEBCORE_DIR}/bindings/scripts/resolve-supplemental.pl ${SCRIPTS_RESOLVE_SUPPLEMENTAL} ${WebCore_IDL_FILES}

too

> Source/WebCore/UseV8.cmake:271
> +        MAIN_DEPENDENCY ${_file} ${SUPPLEMENTAL_DEPENDENCY_FILE}

too

[Kentaro Hara]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 120798 [details] [details])
> 
> By the way, maybe should we need to make a change on PlatformBlackBerry.cmake at the same time? In other words, if we make a change on CMakefile.list, the change will affect UseJSC.cmake, UseV8.cmake and PlatformBlackBerry.cmake, right?

Correction: The change on CMakeLists.txt in this patch does not affect the current behavior of PlatformBlackBerry.cmake. Although we need to make a change to PlatformBlackBerry.cmake in the near future but do not have to do it in this patch.

[Kentaro Hara]

(In reply to comment #17)
> (From update of attachment 120800 [details])
> > Source/WebCore/UseJSC.cmake:270
> > +FILE(WRITE ${IDL_FILES_TMP} ${IDL_FILES_LIST})
> 
> does not seem like a problem, but only FYI: This way IDL_FILES_TMP will be written at _every_ CMake run, so nothing should depend on it to avoid unnecessary rebuilds.

Yeah... To make it better, is there any way to write "FILE(WRITE ${IDL_FILES_TMP} ${IDL_FILES_LIST})" inside ADD_CUSTOM_COMMAND(...), like this?

ADD_CUSTOM_COMMAND(
    OUTPUT ${IDL_FILES_TMP}
    COMMAND FILE(WRITE ${IDL_FILES_TMP} ${IDL_FILES_LIST})   # Syntax Error!
)

ADD_CUSTOM_COMMAND(
    OUTPUT ${SUPPLEMENTAL_DEPENDENCY_FILE}
    DEPENDS ${IDL_FILES_TMP}
    COMMAND perl resolve-supplemental.pl ...
)

[Kentaro Hara]

Created attachment 120801 [details]
Patch

[Patrick R. Gansterer]

(In reply to comment #19)
> (In reply to comment #17)
> > (From update of attachment 120800 [details] [details])
> > > Source/WebCore/UseJSC.cmake:270
> > > +FILE(WRITE ${IDL_FILES_TMP} ${IDL_FILES_LIST})
> > 
> > does not seem like a problem, but only FYI: This way IDL_FILES_TMP will be written at _every_ CMake run, so nothing should depend on it to avoid unnecessary rebuilds.
> 
> Yeah... To make it better, is there any way to write "FILE(WRITE ${IDL_FILES_TMP} ${IDL_FILES_LIST})" inside ADD_CUSTOM_COMMAND(...), like this?
> 
> ADD_CUSTOM_COMMAND(
>     OUTPUT ${IDL_FILES_TMP}
>     COMMAND FILE(WRITE ${IDL_FILES_TMP} ${IDL_FILES_LIST})   # Syntax Error!
> )
> 
> ADD_CUSTOM_COMMAND(
>     OUTPUT ${SUPPLEMENTAL_DEPENDENCY_FILE}
>     DEPENDS ${IDL_FILES_TMP}
>     COMMAND perl resolve-supplemental.pl ...
> )

Not really, you can write a CMake "script", but I don't think that we want this. IMHO it's not bad style to write this file as long as we know about possible problems.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 120801 [details]
Patch

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

> Source/WebCore/UseJSC.cmake:269
> +SET(FIRST_IDL_FILE_FLAG 1)
> +FOREACH (_idl ${WebCore_IDL_FILES})
> +    IF (${FIRST_IDL_FILE_FLAG})
> +        SET(FIRST_IDL_FILE_FLAG 0)
> +        SET(IDL_FILES_LIST "${WEBCORE_DIR}/${_idl}\n")
> +    ELSE ()
> +        SET(IDL_FILES_LIST "${IDL_FILES_LIST}${WEBCORE_DIR}/${_idl}\n")
> +    ENDIF ()
> +ENDFOREACH ()

Isn't

  FOREACH (_idl ${WebCore_IDL_Files})
    SET(IDL_FILES_LIST "${IDL_FILES_LIST}${WEBCORE_DIR}/${_idl}\n")
  ENDFOREACH ()

enough?

[Kentaro Hara]

Created attachment 120802 [details]
Patch

[Kentaro Hara]

(In reply to comment #22)
> (From update of attachment 120801 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120801&action=review
> 
> > Source/WebCore/UseJSC.cmake:269
> > +SET(FIRST_IDL_FILE_FLAG 1)
> > +FOREACH (_idl ${WebCore_IDL_FILES})
> > +    IF (${FIRST_IDL_FILE_FLAG})
> > +        SET(FIRST_IDL_FILE_FLAG 0)
> > +        SET(IDL_FILES_LIST "${WEBCORE_DIR}/${_idl}\n")
> > +    ELSE ()
> > +        SET(IDL_FILES_LIST "${IDL_FILES_LIST}${WEBCORE_DIR}/${_idl}\n")
> > +    ENDIF ()
> > +ENDFOREACH ()
> 
> Isn't
> 
>   FOREACH (_idl ${WebCore_IDL_Files})
>     SET(IDL_FILES_LIST "${IDL_FILES_LIST}${WEBCORE_DIR}/${_idl}\n")
>   ENDFOREACH ()
> 
> enough?

Absolutely. Fixed it.

[Raphael Kubo da Costa (:rakuco)]

Informal r+ from the EFL side.

[Daniel Bates]

CC'ing Antonio Gomes and Rob Buis so that they can follow this change with respect to PlatformBlackBerry.cmake.

[Daniel Bates]

Comment on attachment 120802 [details]
Patch

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

This looks good to me.

> Source/WebCore/ChangeLog:35
> +        * UseJSC.cmake: Described the above build.

Nit: This remark doesn't read well (*). I would either remove this remark or revise it. One suggestion is: "Modified to reflect the new build flow as described above."

(*) The remark doesn't read well because the word "describe" neither clarifies whether this patch modifies the file UseJSC.cmake nor what the modification is. (Although the presence of this line in the commit message indicates that either UseJSC.cmake was modified or newly added.) Moreover, the past tense usage of the word "describe" gives the impression that UseJSC.cmake either already implements or has a comment that describes the supplemental build flow ("new build flow"). Together these issues make it difficult to understand this remark.

> Source/WebCore/ChangeLog:36
> +        * UseV8.cmake: Exactly the same change as UseJSC.cmake, except for "JS" or "V8".

Nit: Since the purpose of this remark is to indicate that a similar change to the build flow was made to file UseV8.cmake, I would just write "Ditto."

> Source/WebCore/CMakeLists.txt:373
>      webaudio/DelayNode.idl
> +    webaudio/DOMWindowWebAudio.idl

Nit: These lines should be swapped such that this section of the list of IDL files is in sorted order according to the UNIX sort command.

[Kentaro Hara]

Created attachment 120821 [details]
patch for commit

[Kentaro Hara]

Created attachment 120822 [details]
patch for commit

[Kentaro Hara]

(In reply to comment #27)
> (From update of attachment 120802 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120802&action=review
> 
> This looks good to me.
> 
> > Source/WebCore/ChangeLog:35
> > +        * UseJSC.cmake: Described the above build.
> 
> Nit: This remark doesn't read well (*). I would either remove this remark or revise it. One suggestion is: "Modified to reflect the new build flow as described above."
> 
> (*) The remark doesn't read well because the word "describe" neither clarifies whether this patch modifies the file UseJSC.cmake nor what the modification is. (Although the presence of this line in the commit message indicates that either UseJSC.cmake was modified or newly added.) Moreover, the past tense usage of the word "describe" gives the impression that UseJSC.cmake either already implements or has a comment that describes the supplemental build flow ("new build flow"). Together these issues make it difficult to understand this remark.
> 
> > Source/WebCore/ChangeLog:36
> > +        * UseV8.cmake: Exactly the same change as UseJSC.cmake, except for "JS" or "V8".
> 
> Nit: Since the purpose of this remark is to indicate that a similar change to the build flow was made to file UseV8.cmake, I would just write "Ditto."
> 
> > Source/WebCore/CMakeLists.txt:373
> >      webaudio/DelayNode.idl
> > +    webaudio/DOMWindowWebAudio.idl
> 
> Nit: These lines should be swapped such that this section of the list of IDL files is in sorted order according to the UNIX sort command.

dbates: Fixed them and committed. Thanks for the review.

[WebKit Review Bot]

Comment on attachment 120822 [details]
patch for commit

Clearing flags on attachment: 120822

Committed r103854: <http://trac.webkit.org/changeset/103854>

[Ming Xie]

As Antonio suggested, I tried this patch with our BlackBerry port which also uses CMake. Unfortunately, our local repository hasn't synced up with upstream to have all the required IDLs, and our upstream BlackBerry build port is not yet functional. Therefore, we couldn't really try out this patch with our port at this moment, and we will re-try this later.

[Adam Barth]

When is later?

[Antonio Gomes]

(In reply to comment #33)
> When is later?

We are upstreaming, so lets not get this bug blocked on us.

It was very nice of kentaro to try to build his patch for BlackBerry port. We have a bug for tracking BlackBerry specific bits of his work, so if there is nothing else left here, it is good to go.. :)

[Kentaro Hara]

(In reply to comment #34)
> (In reply to comment #33)
> > When is later?
> 
> We are upstreaming, so lets not get this bug blocked on us.
> 
> It was very nice of kentaro to try to build his patch for BlackBerry port. We have a bug for tracking BlackBerry specific bits of his work, so if there is nothing else left here, it is good to go.. :)

ming, tonikitoo: Thank you very much for the quick support. Do you mean that for the time being it is OK to commit the bug 75413 without confirming that the BlackBerry build passes? (The change of the bug 75413 is almost the same as the change that I've done for UseJSC.cmake and UseV8.cmake.)

In fact, the bug 75413 is the last patch for enabling the [Supplemental] IDL on all build systems. In order to use the [Supplemental] IDL in WebKit, we need to enable it on *all* build systems. (If we start to use the [Supplemental] IDL in WebKit without committing the bug 75413, I am afraid that the BlackBerry build will be broken more because it cannot interpret the [Supplemental] IDL.)

[Adam Barth]

I think he's saying to go ahead and use [Supplemental] without committing bug 75413.  They're in the process of bringing up their port.  When they're ready, they'll take a look at bug 75413 and provide feedback / tweak it as necessary.

[Kentaro Hara]

(In reply to comment #36)
> I think he's saying to go ahead and use [Supplemental] without committing bug 75413.  They're in the process of bringing up their port.  When they're ready, they'll take a look at bug 75413 and provide feedback / tweak it as necessary.

Ah, I got it. Thanks.

[Martin Robinson]

I believe that cmake has had support for the supplemental IDL for a while now.