-
Notifications
You must be signed in to change notification settings - Fork 202
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
Sample makefile should use strict compiler flags and treat warnings as errors #24
Comments
Just looked at OSAL building verbose, and it didn't include -Wall by default (doesn't look like elf2cfetbl has it either). Is the line setting it in osal/CMakeLists.txt working as intended? Should this be a top level setting (along with the rest of the useful flags) instead of buried in each element's recipe? |
The problem was, historically, not every tool built successfully with Now that we have cleaned up warnings in most tools, this could possibly be made better. |
A cleanup would be good. My take (from your email suggestion) is at minimum "-Wall –std=c99 –pedantic –Wstrict-prototypes –Wwrite-strings" everywhere by default (or at least all flight code), all the time set from a top level (hopefully something in sample_defs or from the default top Makefile). I'd prefer if -Werror was optional such that we can script a CI build of the community set of apps with and without it included. I want to avoid having to edit files for the various CI tests and I suspect not all apps will be warning free. |
Addendum to previous comment - other threads have mentioned -Wall and -Werror does break some elements, which bolsters the use case for -Werror being easily configurable such that we can apply this change easily without breaking everything (report all warnings, make -Werror optional). |
My idea is to add these flags to the This way, if you do set that variable in your prep step, your setting will override the value in the makefile. This way its easy to take it out in case of building some legacy code where -Werror doesn't work, or in a CI build script that includes legacy code. |
Note it doesn't work as I'd expect now
reports
Yet a
snippit for an OSAL element shows:
Note the missing -Wall. My hunch is the flags are getting set in osal/CMakeLists.txt and reported:
and then exported to parent scope, but then in setting up the UNIT TEST SUPPORT LIBRARIES, the flags are getting reset to not include Wall. I think there is more cleanup required beyond just the proposed modification. |
Although that's OSAL, so likely a topic of a different issue. |
I've come to the conclusion that this would probably work a lot better if OSAL stopped setting CMAKE_C_FLAGS directly. Years ago there was some desire to make things work with very old versions of CMake that were shipped with RHEL 5, which I think was version 2.6.4. But we no longer test with anything nearly that old. In newer versions CMake has added features to help manage the use of different compiler flags in different targets/directories and thereby avoid directly setting the CMAKE_C_FLAGS variable. Unfortunately RHEL7, which is still in active use, provides only CMake v2.8.12. But even this version has most of the target-oriented commands that would help, as it is a lot newer than the RHEL5 version (target_compile_definitions, target_compile_options, etc). |
Sounds great to me. Could you prioritize this update? It's currently complicating improvements in work for our CI. |
Adds STRICT_NO_WARNINGS and OMIT_DEPRECATED prep options for CI and as example build
Adds STRICT_NO_WARNINGS and OMIT_DEPRECATED prep options for CI and as example build
Add extra compile options for mission scope and arch scope. These are separated to support cross compile environments that do not/cannot use the same flags on both builds. For "mission" build the targets are never cross compiled, only built for the native host machine. It should be safe to assume a compiler in the GCC family and the strict warnings should _always_ be applicable here. For "arch" build the options are compiler vendor dependent. The file as-supplied can only be used if all the target cross compilers are in the same family and support the same warning options. However, this file can be modified without affecting the options used for the host side tools.
Add a _custom suffix to differentiate the customization file from the base file in the cmake directory.
Include in the basic warning set. Note that at this time the CFE does not build cleanly on all architectures with this warning enabled, pending resolution of issue nasa#313.
Fix #24, Add compiler option examples
Describe the bug
The default makefiles / CMake scripts do not enforce any strict compiler warning flags and do not treat warnings as errors. This means issues like #22 can more easily slip past.
To Reproduce
Building the
rc-6.7.0
branch as a "release" (-O3) using the default settings/sample config/makefile wrapper by preparing as:make SIMULATION=native BUILDTYPE=release prep
Then run make and you do get some warnings, at least when using gcc 7.x and above:
However, the build continues and completes the process with no error results.
Expected behavior
The build should stop, because warnings are problems that need to be resolved. If the build does not stop then it is very easy to not notice these issues.
System observed on:
Ubuntu 18.04 (64-bit), kernel 5.0.0-23-generic, gcc 7.4.0
Reporter Info
Joseph Hickey, Vantage Systems, Inc.
The text was updated successfully, but these errors were encountered: