Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iOS: Treat warnings as errors #24035

Closed
wants to merge 1 commit into from
Closed

Conversation

hramos
Copy link
Contributor

@hramos hramos commented Mar 19, 2019

Summary

Related to #22609. Prevent new warnings from being introduced into the codebase by treating them as errors in CI.

Changelog

[iOS] [Changed] - Xcode warnings are treated as errors during CI tests.

Test Plan

See https://circleci.com/workflow-run/147742c3-69ee-439c-8cc4-6dd0cbc8726f for an example of a failed build, as there's a couple of warnings that exist on master as of this writing:

react-native/ReactCommon/jsi/JSCRuntime.cpp:827:65: error: unused parameter 'obj' [-Werror,-Wunused-parameter]
jsi::WeakObject JSCRuntime::createWeakObject(const jsi::Object& obj) {
                                                                ^
react-native/ReactCommon/jsi/JSCRuntime.cpp:837:62: error: unused parameter 'obj' [-Werror,-Wunused-parameter]
jsi::Value JSCRuntime::lockWeakObject(const jsi::WeakObject& obj) {
                                                             ^
2 errors generated.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels Mar 19, 2019
@hramos hramos mentioned this pull request Mar 19, 2019
9 tasks
@hramos hramos changed the title [WIP] iOS: Treat warnings as errors iOS: Treat warnings as errors Mar 19, 2019
Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this is super awesome! Land it if it passes CI.

@hramos
Copy link
Contributor Author

hramos commented Mar 19, 2019

I am not sure yet if this is the right approach. I fixed the warnings in the main project, but clang is now failing on warnings from third-party dependencies. E.g. folly:

react-native/third-party/folly-2018.10.22.00/folly/detail/Demangle.cpp:33:23: error: unused parameter 'mangled' [-Werror,-Wunused-parameter]
    char const* const mangled,
                      ^
react-native/third-party/folly-2018.10.22.00/folly/detail/Demangle.cpp:34:18: error: unused parameter 'cbref' [-Werror,-Wunused-parameter]
    void (*const cbref)(char const*, std::size_t, void*),
                 ^
react-native/third-party/folly-2018.10.22.00/folly/detail/Demangle.cpp:35:17: error: unused parameter 'opaque' [-Werror,-Wunused-parameter]
    void* const opaque) {
                ^
3 errors generated.

@cpojer
Copy link
Contributor

cpojer commented Mar 19, 2019

Aww that sucks. Maybe another approach could simply be to build with warnings and fail it if the output (stderr/stdout) text matches warnings in RN?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants