Add compiler option examples

[jphickey]

Describe the contribution

Fix #24 - Add extra compile options for mission scope and arch scope.

These are separated such that the same basic structure will also apply to cross compile environments that do not/cannot use the same flags on both (host and cross) 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.

Testing performed
Build for SIMULATION=native and confirm full suite of warnings is used for both host-side tools and FSW code.
Also build for other platforms (MIPS, RTEMS) where the code is not (yet) fully warning-free. Confirmed that the warnings can be modified at the arch_build without affecting the host-side tools which are still built with full warnings.

Expected behavior changes
No impact to runtime behavior

System(s) tested on:
Ubuntu 18.04 LTS 64-bit (build host)

Contributor Info
Joseph Hickey, Vantage Systems, Inc.

Community contributors
You must attach a signed CLA (required for acceptance) or reference one already submitted

[jphickey]

Note this does NOT add a cache variable to turn off strict warnings at this time. It would be easy to add this back in, but from a framework point of view I think we should just keep it simple and always compile with the full warnings.

• For the host side tools there should be no reason to turn this off unless e.g. a new compiler version or something is causing new warnings that didn't previously exist, but then it would be preferable to fix the warnings, not disable them in the compiler.
• For the arch/FSW code there should also be no warnings at least in the CCB-managed modules. For missions deploying this software they might include 3rd party applications which do have warnings, but they can easily turn them off by editing arch_build.cmake in their own defs dir if need be. This will not affect the warnings used for tools and host side code.

[skliper]

How does the cmake/sample_defs/mission_build.cmake relate to the cmake/mission_build.cmake? I'd think overloading names could cause confusion.

Other than that this is an excellent change, and gets us exactly what we need to be compliant with this element of the current GSFC coding standard.

[jphickey]

How does the cmake/sample_defs/mission_build.cmake relate to the cmake/mission_build.cmake? I'd think overloading names could cause confusion.

Regarding the mission_build.cmake and arch_build.cmake - I don't really see it as an overload, the one inside the defs directory can be considered a local custom extension of the file in the cmake directory.

The basic function that pulled in this file from the parent script was already there, we are just now providing an example of how it works and using it for this purpose.

The other purpose of this feature (and the reason the hook had existed in the first place) was to allow missions to override the install functions in case they needed custom logic as part of their final deployment step. So for instance one can provide a custom implementation of the cfe_exec_do_install or cfs_app_do_install in their local arch_build.cmake file.

But I think it is a logical fit for the compiler options too.

[skliper]

Not my preference to have two files named the same but with different purpose (in my interpretation). Where cmake/mission_build.cmake is the "default" and cmake/sample_defs/mission_build.cmake is "overrides" or local changes or custom configuration, or whatever you want to call it. Now if, for example, a user says "I modified mission_build.cmake to work for my mission" it's hard to tell if they modified the right one without additional context.

Not a huge deal, but I'd prefer different file names (mission_build_custom.cmake or mission_build_options.cmake or whatever), which eliminates this potential confusion.

Or go the other way if you don't want to break existing dependencies on this pattern and change cmake/mission_build.cmake to mission_build_default.cmake (or some other unique name, like base, global, or whatever)?

[jphickey]

Well, my main concern with using a different name is only that this was a preexisting hook so it will break any users that were already using this for something. But, it is likely that nobody (other than myself) is using it because it wasn't obvious that this hook existed.

How about renaming to mission_build_custom.cmake and arch_build_custom.cmake for the local extension files?

[skliper]

Works for me!

[jphickey]

FYI - commit 7a2f49b adds the _custom suffix as proposed.

[skliper]

What about -Wcast-align? This should be the gold standard set, wouldn't that be useful to include? I understand it may not work everywhere today, but for those cases users can configure for the reduced set. I want to get away from only defining what passes as the "example", instead cover the set of warnings we should be passing and raise the priority of getting the warnings fixed.

[jphickey]

I would prefer to at least complete #313 before we add -Wcast-align to the standard set.

Note that this option is basically a no-op for CI, since all architectures that are (currently) built do not have alignment requirements. So it is basically ignored. The reason for including it is only for the benefit of users on architectures that actually have alignment requirements, and for those users, enabling this option at this time will cause a build failure.

[skliper]

Understood, but the desire is the "gold standard", what should pass not what does pass everywhere. These flags and the more through CI testing will occasionally fail, that's the whole point especially on systems we don't frequently test on. The point is to set the standard, which we have failed to do as a managing organization in the past. Nobody knows the standard, so how can they even start to comply?

This is the example set, if you (or a user) has to deviate from the gold standard that should be a conscious decision. Override it in your *_defs if you want. Saying the sample_defs should never fail for anybody sounds like an impossible goal. Besides, the sample defs come preconfigured for pc-linux so for any other system you have to change the sample defs anyways... so it doesn't work out of the box to begin with for these alternate systems.

[skliper]

Basically the resistance to failure holds the community back from solving issues. If only you are applying your version of the "gold standard" when testing, we have no common ground for everyone to test against. Everyone else who may be affected by this additional flag isn't able to proactively solve the issues until we do add it, so it's waterfall delays that aren't really necessary.

[jphickey]

I simply see this as staging the changes to avoid enforcement in a configuration where the code is known to be non-compliant. This is the way we have done every other change that constituted an increase in "strictness" -- i.e. make sure the code is compliant first, then add the enforcement.

I'm not sure why this case would be handled differently than we have handled this for all the other enforcement options...?

We can just as easily add -Wcast-align to the pull request for the other changeset, rather than this one, and thereby avoid a broken config. But if there is a strong preference for adding it now and thereby (at least temporarily) breaking the build for some architectures, then that's OK too.

The only problem with allowing some CI builds to be broken out of the box is that we may miss newly-added regressions in the meantime. But the real fix won't be that far behind (I have it staged, just waiting for "master" to have the prerequisites)

[skliper]

I'm not sure why this case would be handled differently than we have handled this for all the other enforcement options...?

This is unique in that without the warning enabled, the rest of the community doesn't know there is an issue. I'm perfectly fine not including -Werror until it's fixed, is that better? It's the lack of reporting the warning I'm trying to avoid.

The only problem with allowing some CI builds to be broken out of the box is that we may miss newly-added regressions in the meantime. But the real fix won't be that far behind (I have it staged, just waiting for "master" to have the prerequisites)

Does the above suggestion help? Won't break, just will report all the warnings.

[jphickey]

I actually have stronger preference for keeping -Werror than a clean build, because without this option it is hard to notice when regressions occur on ANY architecture, whereas the cast align issue is limited to just specific architectures. So the window of opportunity for regressions is smaller by enabling cast-align warning and letting the MIPS/SPARC builds fail in the meantime.

Commit c5625fc adds this.

[skliper]

The only problem with allowing some CI builds to be broken out of the box is that we may miss newly-added regressions in the meantime. But the real fix won't be that far behind (I have it staged, just waiting for "master" to have the prerequisites)

So I'm really not following on why the systems that don't pass due to a known flag can't just remove that flag and continue to check for newly-added regressions? You know the issue is there, you can remove the flag to ensure nothing else is failing until the issue is fixed. It's this specific failure case that should be unique and require the minor extra step. The sample_defs is intended to be customized, right? I don't think maintaining that everything will always work right out of the box on every system is attainable, there's always special cases or one more system/toolchain that we haven't tested against that will end up failing. I'd like to avoid the lowest common denominator approach (we can only enforce XYZ when everybody everywhere passes). Rather enforce a high standard, and allow for customization.

[jphickey]

It can certainly be handled in CI for those known platform-specific issues with some extra logic by providing an alternate config for that platform and implementing the logic to work around the issue.

As there is (currently) no CI for these platforms anyway, it means remembering to manually tweak the config each time a MIPS/SPARC target platform is tested. Just more work.

But IMO a bigger concern is a user who happens to clone the "master" branch and follow the instructions to set up a build, and gets a compile failure.

[skliper]

CCB 20200115 - Reviewed and approved to CI