nasa / fprime

F´ - A flight software and embedded systems framework
https://fprime.jpl.nasa.gov
Apache License 2.0
10.05k stars 1.3k forks source link

Have CI compile match user compile #1532

Open timcanham opened 2 years ago

timcanham commented 2 years ago
F´ Version 3.0
Affected Component

Feature Description

Have the CI compile flags match the user compile flags.

Rationale

CI builds, especially unit tests, often fail because the compile arguments are stricter than the ones that the user can use. This can cause multiple branch pushes to a PR just to get CI to pass. The compiler arguments should match or alternatively the user could be provided a script or cmake command to make sure CI won't fail before pushing to CI.

Joshua-Anderson commented 2 years ago

Apologies for the annoyance of the extra CI compilation steps - I know just how annoying the iteration process can be when you get CI failures.

CI does compile fprime a little differently from most users:

I'm going to postulate that users don't want these compilation flags enabled for their projects. -Wall -Werror in particular can be problematic, because with every compiler update new warnings are added. It's extremely annoying when you update your compiler and suddenly become unable to compile your dependencies because of "harmless" new compiler warnings. To my understanding, it's best practice to only error on compiler warnings within your project.

Because it's a static analyzer, clang-tidy has a fairly significant build impact, so it's usually something you only want to run right before you submit a PR, not on every compilation.

What I would propose:

I'm not sure where the best place to document this would be, but I agree this should be documented to help fprime contributors catch warnings prior to opening a PR.

Does this seem reasonable to you Tim?

timcanham commented 2 years ago

So to understand the current capability, I can get the current CI build (minus clang-tidy) by doing fprime-util check from the root of the fprime repo? How does one run clang-tidy manually? I rather do that than have CI bounce a couple of times. It might be nice to have a doCiChecks.sh script that we could start and walk away for a while just before doing a push to a PR branch.

Joshua-Anderson commented 2 years ago

So to understand the current capability, I can get the current CI build (minus clang-tidy) by doing fprime-util check from the root of the fprime repo?

Yes, you get everything minus clang-tidy by building the root project.

Running clang-tidy just needs a slightly different generation option fprime-util generate --ut -DCMAKE_CXX_CLANG_TIDY=clang-tidy. The one thing to keep in mind is there are actually two different sets of clang tidy configs. One config applies to all fprime C++ files, the other config checks flight software specific rules that we don't follow for unit tests.

So technically you'd need to do two builds:

fprime-util generate --ut -DCMAKE_CXX_CLANG_TIDY=clang-tidy
fprime-util build --all --ut
fprime-util generate -DCMAKE_CXX_CLANG_TIDY=clang-tidy;--config-file=$PWD/release.clang-tidy
fprime-util build --all -j4

However, currently all the flight software specific clang tidy run checks for is recursion, so practically speaking it can be pretty safely skipped and the general clang tidy check would be sufficient.

LeStarch commented 2 years ago

Solution: document the steps here formally