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

Have CI compile match user compile #1532

Open
timcanham opened this issue Jun 27, 2022 · 5 comments
Open

Have CI compile match user compile #1532

timcanham opened this issue Jun 27, 2022 · 5 comments
Labels
Documentation Documentation, Tutorials and References updates enhancement Low Priority Low prioirty items given the current project priorities.

Comments

@timcanham
Copy link
Collaborator

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
Copy link
Contributor

Joshua-Anderson commented Jun 27, 2022

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:

  • It builds the root CMake project which turns on a lot of extra compilation flags, like -Wall and -Wextra.
  • On the "code quality" CI job, cmake compiles with the latest supported version of GCC (as the newest compiler is likely to generate warnings that the default GCC doesn't have) and runs clang-tidy, a static analyzer, with some specific checks enabled.

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:

  • As a fprime framework developer, compile your changes using the root cmake project with the extra compilation flags before submission.
  • Optionally, run a clang-tidy build before submission, or let CI run clang-tidy and accept you may have to update your PR after submission with static analysis warnings.

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
Copy link
Collaborator Author

timcanham commented Jun 28, 2022

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
Copy link
Contributor

Joshua-Anderson commented Jun 28, 2022

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 LeStarch self-assigned this Sep 30, 2022
@LeStarch LeStarch added the Documentation Documentation, Tutorials and References updates label Sep 30, 2022
@LeStarch
Copy link
Collaborator

Solution: document the steps here formally

@LeStarch LeStarch removed their assignment Aug 24, 2023
@LeStarch LeStarch added the Low Priority Low prioirty items given the current project priorities. label Mar 27, 2024
@LeStarch LeStarch assigned LeStarch and unassigned LeStarch Mar 28, 2024
@LeStarch LeStarch added ROSES Work funded by the ROSES proposal - see Discussions #3041 ROSES Candidate labels Nov 25, 2024
@matt392code
Copy link
Contributor

matt392code commented Dec 27, 2024

  1. fprime-ci-tools.sh.txt
  2. clang-tidy-config.txt
  3. docicheck-script.sh.txt

Targeted solution that addresses the actual F Prime CI setup.
Here's how to use it:

  1. Add the doCiChecks.sh script to your F Prime repository root. Make it executable:
chmod +x doCiChecks.sh
  1. Add the clang-tidy configuration files to your repository:
  • .clang-tidy for general checks
  • release.clang-tidy for flight software specific checks

Key features:

  1. Standard Compilation:

    • Uses fprime-util check from the root to match CI build flags
    • Includes -Wall and -Wextra flags
  2. Clang-Tidy Integration:

    • Runs both general and flight software specific checks
    • Separate configurations for unit tests and flight software
    • Flight software checks focus on recursion detection
  3. Convenience:

    • Single script to run all CI checks locally
    • Clear output indicating progress
    • Checks for required tools before starting

To use this before submitting a PR:

./doCiChecks.sh

The script will run all checks sequentially and report any issues. This allows you to catch potential CI failures before pushing your changes.

Note that as mentioned by Joshua-Anderson:

  • The extra compilation flags (like -Wall -Werror) are intentionally only enabled for CI
  • Clang-tidy is separate due to its build impact
  • Flight software specific checks mainly focus on recursion

@thomas-bc thomas-bc removed the ROSES Work funded by the ROSES proposal - see Discussions #3041 label Feb 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Documentation, Tutorials and References updates enhancement Low Priority Low prioirty items given the current project priorities.
Projects
None yet
Development

No branches or pull requests

5 participants