Bug 98715 – [EFL] Enable -Werror for the EFL port

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Laszlo Gombos]

Some other ports use -Werror as well, but the warning level between ports are not the same, so we should consider which warnings should be treated as errors (assumption is that most warnings should be treated as errors).

[Laszlo Gombos]

Created attachment 167682 [details]
proposed patch

Treat all warnings as errors, except the "unused parameter" warning. The "unused parameter" warning is not turned on for other ports as warning, so that will require some discussion with the other ports. 

Removed the ANOTHER_BRICK_IN_THE define as I do not think that is used (I found it confusing to have the define in the build logs passed on CLI to the compiler).

[Gyuyoung Kim]

Comment on attachment 167682 [details]
proposed patch

As we talked about this yesterday, I agree with this patch. IMO, it would be good if you notify this to webkit-efl mailing list.

[Chris Dumez]

Looks good. Thanks.

[Gyuyoung Kim]

Comment on attachment 167682 [details]
proposed patch

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

> ChangeLog:3
> +        [EFL] Enable -Werror for the EFL port

BTW, Is this patch adjusted into other ports which uses cmake ? I think [CMAKE] prefix is proper instead of [EFL].

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #1)
> Removed the ANOTHER_BRICK_IN_THE define as I do not think that is used (I found it confusing to have the define in the build logs passed on CLI to the compiler).

It was a nice easter egg :}

[Laszlo Gombos]

(In reply to comment #4)

> BTW, Is this patch adjusted into other ports which uses cmake ? I think [CMAKE] prefix is proper instead of [EFL].

This is a great point. I think it would be difficult to coordinate flipping the Werror flag for all cmake-based ports at the same time, so I really only meant to do it for the EFL port first.

I will upload a new patch.

[Laszlo Gombos]

Created attachment 167902 [details]
make the change for the EFL port only and do nto change the behavior for other cmake-based ports

[Gyuyoung Kim]

Comment on attachment 167902 [details]
make the change for the EFL port only and do nto change the behavior for other cmake-based ports

Looks fine.

[Raphael Kubo da Costa (:rakuco)]

Instead of making this change opt-out, I would make it opt-in: we already have the code in the WEBKIT_SET_EXTRA_COMPILER_FLAGS() to make it accept additional options. You could just accept an extra option there which would flip the -Werror switch.

[Laszlo Gombos]

Created attachment 172619 [details]
Make it opt in instead of opt-out (see comment #9)

I also had to exclude more warnings to be not fire errors to make sure it builds.

[EFL EWS Bot]

Comment on attachment 172619 [details]
Make it opt in instead of opt-out (see comment #9)

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

[Laszlo Gombos]

Created attachment 172650 [details]
adding -Wno-error=unused-function to list of flags

one more exclusion to make sure it builds.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 172650 [details]
adding -Wno-error=unused-function to list of flags

My suggestion still holds: instead of using a global variable like that, accept another option in the WEBKIT_SET_EXTRA_COMPILER_FLAGS macro and parse it just like we already do with IGNORECXX_WARNINGS.

[Thiago Marcos P. Santos]

Comment on attachment 172650 [details]
adding -Wno-error=unused-function to list of flags

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

> Source/cmake/WebKitHelpers.cmake:-40
> -        SET(OLD_COMPILE_FLAGS "-DANOTHER_BRICK_IN_THE -Wall -Wextra -Wcast-align -Wformat-security -Wmissing-format-attribute -Wpointer-arith -Wundef -Wwrite-strings ${OLD_COMPILE_FLAGS}")

-DANOTHER_BRICK_IN_THE -Wall is a long standing joke on the CMake buildsystem, I think it should stay.

> Source/cmake/WebKitHelpers.cmake:45
> +        SET(OLD_COMPILE_FLAGS "-Werror -Wno-error=unused-parameter -Wno-error=switch -Wno-error=sign-compare -Wno-error=unused-function ${OLD_COMPILE_FLAGS}")

I would write this as SET(OLD_COMPILE_FLAGS ${OLD_COMPILE_FLAGS} ...) to make it explicit that you are appending parameters.

[Chris Dumez]

Comment on attachment 172650 [details]
adding -Wno-error=unused-function to list of flags

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

>> Source/cmake/WebKitHelpers.cmake:-40
>> -        SET(OLD_COMPILE_FLAGS "-DANOTHER_BRICK_IN_THE -Wall -Wextra -Wcast-align -Wformat-security -Wmissing-format-attribute -Wpointer-arith -Wundef -Wwrite-strings ${OLD_COMPILE_FLAGS}")
> 
> -DANOTHER_BRICK_IN_THE -Wall is a long standing joke on the CMake buildsystem, I think it should stay.

I don't see the point in keeping this "joke". It makes the line uselessly longer. If anything it does not look serious.

[Laszlo Gombos]

Comment on attachment 172650 [details]
adding -Wno-error=unused-function to list of flags

updated patch coming..

[Laszlo Gombos]

Created attachment 172814 [details]
take rakuco's feedback

[EFL EWS Bot]

Comment on attachment 172814 [details]
take rakuco's feedback

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

[Laszlo Gombos]

Created attachment 172849 [details]
one more try

Add back -Wno-error=switch for now and do not (yet) add Werror for the WebKit2 sources to fix the build. After this patch landed I will file separate bugs to resolve these issues.

[Raphael Kubo da Costa (:rakuco)]

LGTM if the EWS does not complain.

[EFL EWS Bot]

Comment on attachment 172849 [details]
one more try

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

[Laszlo Gombos]

Created attachment 172892 [details]
one more try

hmm.. I have not had this build failure locally. Excluding webkit1 sources from Werror as well for now..

[Raphael Kubo da Costa (:rakuco)]

Doesn't it sense to fix the WebKit bits before landing this one?

[Laszlo Gombos]

(In reply to comment #23)
> Doesn't it sense to fix the WebKit bits before landing this one?

I prefer to land the baseline first and start fixing the remaining issues. We normally take the same approach with Layouttests as well. 

When I first introduce this patch about a month ago I only had to exclude unused-parameter. I month later it seems I had to exclude unused-parameter, sign-compare, switch and WebKit1 WebKit2. I think the longer we chase the trunk the harder it is to do it in one patch.

[WebKit Review Bot]

Comment on attachment 172892 [details]
one more try

Clearing flags on attachment: 172892

Committed r134057: <http://trac.webkit.org/changeset/134057>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.