Bug 232936 – [GTK] When building introspection files, add CMAKE_C_FLAGS to the compiler flags.

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alexander Kanavin]

When building introspection files, add CMAKE_C_FLAGS to the compiler flags.

[Alexander Kanavin]

Created attachment 443793 [details]
Patch

[Michael Catanzaro]

Comment on attachment 443793 [details]
Patch

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

Hi, thanks for your patch. Hm, it sure seems like the right thing to do, so I suppose we should try it. That said, this seems very familiar. Pretty sure we've discussed this before at some point, likely years ago. I wonder if this will surface some unexpected problem. Oh well, only one way to find out!

r- is only because you've failed the style bot due to the "No new tests (OOPS!)."

> Source/JavaScriptCore/ChangeLog:3
> +        When building introspection files, add CMAKE_C_FLAGS to the compiler flags.

[GTK] When building introspection files, add CMAKE_C_FLAGS to the compiler flags.

> Source/WebKit/ChangeLog:3
> +        When building introspection files, add CMAKE_C_FLAGS to the compiler flags.

[GTK] When building introspection files, add CMAKE_C_FLAGS to the compiler flags.

> Source/WebKit/ChangeLog:8
> +        No new tests (OOPS!).

You can simply remove this line in order to pass the style bot. Your change doesn't require adding new tests.

> Source/JavaScriptCore/PlatformGTK.cmake:74
> +        COMMAND CC=${CMAKE_C_COMPILER} CFLAGS=-Wno-deprecated-declarations\ ${CMAKE_C_FLAGS} LDFLAGS=

Can you write it without a backslash as CFLAGS="-Wno-deprecated-declarations ${CMAKE_C_FLAGS}"?

[Michael Catanzaro]

P.S. If you set the WebKitGTK component when reporting the bug, we'll see it faster.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #2)
> Hi, thanks for your patch. Hm, it sure seems like the right thing to do, so
> I suppose we should try it. That said, this seems very familiar. Pretty sure
> we've discussed this before at some point, likely years ago. I wonder if
> this will surface some unexpected problem. Oh well, only one way to find out!

Actually it looks like gobject-introspection is supposed to handle this itself. Let me check with them.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #4)
> Actually it looks like gobject-introspection is supposed to handle this
> itself. Let me check with them.

Sorry, I misunderstood the patch. This looks fine to me.

[Adrian Perez]

Comment on attachment 443793 [details]
Patch

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

>> Source/JavaScriptCore/PlatformGTK.cmake:74
>> +        COMMAND CC=${CMAKE_C_COMPILER} CFLAGS=-Wno-deprecated-declarations\ ${CMAKE_C_FLAGS} LDFLAGS=
> 
> Can you write it without a backslash as CFLAGS="-Wno-deprecated-declarations ${CMAKE_C_FLAGS}"?

Should work without the backslash, yup. Here it is harmless because escaping a space
character in the shell results in a space character ("\ " → " ") but it would be
nicer to remove it, for clarity.

> Source/WebKit/PlatformGTK.cmake:677
> +        COMMAND CC=${CMAKE_C_COMPILER} CFLAGS=-Wno-deprecated-declarations\ ${CMAKE_C_FLAGS} LDFLAGS=

Same issue here with the backslash.

> Source/WebKit/PlatformGTK.cmake:723
> +        COMMAND CC=${CMAKE_C_COMPILER} CFLAGS=-Wno-deprecated-declarations\ ${CMAKE_C_FLAGS}

...and here.

[Alexander Kanavin]

Created attachment 452217 [details]
Patch

[Adrian Perez]

(In reply to Alexander Kanavin from comment #7)
> Created attachment 452217 [details]
> Patch

I think the patch needs a rebase, could you update it and resubmit it?

[Alexander Kanavin]

Created attachment 452846 [details]
Patch

[EWS]

Committed r290312 (247635@main): <https://commits.webkit.org/247635@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452846 [details].