See comments [geo] -----Original Message----- From: [email protected] [mailto:[email protected]] On Behalf Of Mats Wichmann Sent: Tuesday, January 23, 2018 8:49 AM To: IoTivity Developer List <[email protected]> Subject: [dev] build warnings
Although I've not had as much recently as I'd like hashing things out here on the list, it still feels like the right place to ask questions. Through 2017 there was an effort to make build warnings have some meaning - many were rooted out and fixed, the flags during builds were adjusted, etc. The linux builds relatively recently were told to actually fail if there are warnings, although the default state of this setting met with a bit of trouble, and there's a patch pending for the 1.3 branch to flip it back to default to off, rather than on: https://gerrit.iotivity.org/gerrit/#/c/23963/ In any case, for as long as gcc has been around, version bumps have brought greater strictness. gcc7 is widespread now and it followed this tradition, nothing new - and as the C++ standards (in particular) evolve, it is even what you would want, to follow those standards. Ubuntu LTS 18.04 will have gcc 7.2. gcc7 won't build iotivity with these new settings, so there are now some questions about warnings. Backgrounder: 1. linux builds set the flag -Wextra in CCFLAGS, which turns on additional warnings for both C and C++. 2. linux builds set the flag -Werror in CFLAGS, which cause warnings to be treated as errors for C, but not for C++. 3. External library builds (extlibs) remove the -Werror flag, since our builds should not fail on warnings in someone else's code. If necessary we can propose fixes to upstream, but they should come in that way, not from local patching which we then have to try to maintain. 4. One external library, tinycbor, does not receive the treatment described in 3, as the SConscript there just makes sure the files are in place and then adds the files to a variable which is picked up later, so there are quite a few warnings from this library, and due to the context of the build these are fails (there's also a move to uplift the external library since the warnings are believed fixed there, but apparently there are problems at the moment) So that leaves serveral questions about warnings: A. as described in (2), -Werror applies currently only to C code. Should this flag be moved to CCFLAGS so it applies to both languages? [geo] I thought the -Werror flag in CFLAGS was also being applied to C++. I fixed warnings in both C and C++ so it should be applied to both. Since it has not been applied to both it may take a while to fix up. I will look into changing it to CCFLAGS. I just looked at the SCONS documentation CFLAGS: C only, CCFLAGS: C and C++, CXXFLAGS: C++ only, CPPFLAGS anything using C preprocessor C, C++ fortran, and assembly language source files. The intention was to enforce the iotivity coding standards which are for C and C++. I quote [geo] "Code must compile with no warnings at the standard warning level without flags forcing any warnings off. Exceptions are allowed only for warnings that occur within any external SDKs used or warnings that cannot be eliminated without using pragmatics (e.g. for nested STL templates, certain compilers will warn that names have been truncated). For these cases, #pragma directives may be used to eliminate the warning, but only for the module in which the warning occurs. [C/C++]" (https://wiki.iotivity.org/iotivity_c_coding_standards) B. how to handle the tinycbor warnings, since the discussion in IOT-2539 does not apply this this case? Since the tinycbor files are only built in to ocsrm library I chose to ignore those warnings when building the library. See https://gerrit.iotivity.org/gerrit/#/c/23965/ [still in review] I felt this made since. Its limited to a small collection of files that the extra warnings are ignored on. If the warnings are fixed on tinycbor ignore flags can be removed. C. Deprecated language features generate a warning, which now causes errors. In particular, C++ exception syntax is changing: service/notification/cpp-wrapper/consumer/inc/NSProvider.h:143:69: warning: dynamic exception specifications are deprecated in C++11 [-Wdeprecated] (there is a jira ticket on that from a while back, IOT-2782) [geo] If the code is deprecated it should be fixed to use the non-deprecated replacement. [geo] If the deprecated code is in a unit test that is specifically designed to test the deprecated code then it makes since to add the -Wno-deprecated or another appropriate build flag. (see https://gerrit.iotivity.org/gerrit/#/c/23111/12/resource/csdk/security/unittest/SConscript) for an example of turning off the deprecated warning for unit tests. [geo] Any warning can be ignored if needed but the developer that turns off a build warning should try and limit it to the smallest amount of files as possible and they should justify their reason for turning it off. D. a resource property is used as a flag, but gcc no longer likes that usage: resource/src/OCResource.cpp:227:14: warning: enum constant in boolean context [-Wint-in-bool-context] The usage that causes the complaint always looks like: if (!OC_SECURE) where OC_SECURE is an enumerator in the OCResourceProperty enumeration. E. Deprecated usage in unittests, based on our own deprecation of two functions b64Encode and b64Decode (using the new OC_DEPRECATED stuff). In fact we deprecated the whole header base64.h. So this deprecation is causing build fails... was that actually intended? Or is the fail part premature? [geo] the only thing that should continue to use the b64Encode/b64Decode inside IoTivity are the unit tests. And the deprecated warning should be ignored for that code. If it is not being ignored due to changes in the compiler we should update the scons to ignore that build failure. resource/csdk/security/unittest/base64tests.cpp:103:79: warning: 'B64Result b64Encode(const uint8_t*, size_t, char*, size_t, size_t*)' is deprecated: Please use mbedtls implementation [-Wdeprecated-declarations] Since the unit test is testing the behavior of this deprecated code, this creates an interesting dilemma: as long as the code still exists, it should have tests, no? but the test will cause build fails under the current scenario. [geo] Yes, as long as the code still exists unit tests should still be run. So meta-question: with the combination of flags, use of deprecated features causes warnings which we want; warnings cause errors which we want to get people to fix the warnings; but the combination seems to cause something we don't want. We can add a flag to have the deprecation warning go away, which will make the build fails go away, but then we don't get the warnings. I don't think there's a combination of gcc options which say "fail on warnings, except on deprecation warnings". So what is the right way forward on deprecation? [geo] Mats I hope I have answered your questions. _______________________________________________ iotivity-dev mailing list [email protected] https://lists.iotivity.org/mailman/listinfo/iotivity-dev _______________________________________________ iotivity-dev mailing list [email protected] https://lists.iotivity.org/mailman/listinfo/iotivity-dev
