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

Add coverity support to workflow #26

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yasslbk
Copy link

@yasslbk yasslbk commented Feb 27, 2025

This PR adds Coverity support to the build workflow.
We need to add the needed secrets at a repository level.
We can restrict Coverity to publish only results of main branch.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this deserves a separate workflow.

Also do we even want to automatically run Coverity on every PR/push? (I don't know the deal that we have or how much it costs)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will let you choose on this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this into a separate workflow then? This will make it easy to tweak the run condition without affecting the regular build.

Also we will be able to use jobs.<job_id>.if instead of jobs.<job_id>.steps[*].if.

COVERITY_SCAN_TOKEN: ${{ secrets.COVERITY_SCAN_TOKEN }}
run: |
# Download toolbox
COVERITY_URL="https://scan.coverity.com/download/cxx/linux64"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use an environment variable?

Copy link
Collaborator

@jerome-pouiller jerome-pouiller Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, you can probably replace all these lines by something like:

curl https://scan.coverity.com/download/cxx/linux64 \
   --form token=${{ secrets.COVERITY_SCAN_TOKEN }} \
   --form project=wisun-br-linux | tar -xC /opt
ln -sfn /opt/coverity*/bin/coverity /bin

tar -xf coverity.tar.gz -C coverity --strip-components=1
rm -fr coverity.tar.gz
# For the current shell session
export PATH=$(pwd)/coverity/bin:$PATH
Copy link
Collaborator

@MathisMARION MathisMARION Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer either:

  • Install coverity in a standard directory (ie. /usr/bin, /usr/local/bin or alike).
  • Use an absolute path when calling Coverity tools.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this can work also.

export PATH=$(pwd)/coverity/bin:$PATH
coverity --version
# For the upcoming shell sessions
echo "$(pwd)/coverity/bin" >> $GITHUB_PATH
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure coverity exists then simplify the next call of cov-build.

Comment on lines +82 to +89
env:
COVERITY_SCAN_TOKEN: ${{ secrets.COVERITY_SCAN_TOKEN }}
COVERITY_SCAN_EMAIL: ${{ secrets.COVERITY_SCAN_EMAIL }}
COVERITY_SCAN_VERSION: ${{ github.sha }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to use env vars?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I now understand the need for secrets, but is it necessary for the SHA?

Copy link
Author

@yasslbk yasslbk Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessary, there is plenty of other ways to do it, the aim here was reusability by other teams working on open sources projects.
But let me point smt: It's common practice to use env variables in CI/CD tools.
Environment variables are ephemeral and exist only during pipeline execution. No persistence.
Most importantly, defining parameters as environment variables enhances reusability and flexibility, allowing different users or projects to run workflows with custom values without modifying the core script or commands or stages.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want something generic and reusable, the good way is to write an action. Maybe in the future we could have a Silabs action for Coverity if we have common steps.

btw, there seems to be an official Coverity GitHub action, I don't know how usable it is:
https://documentation.blackduck.com/bundle/bridge/page/documentation/c_github-coverity.html

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer relying on simple commands directly over using GitHub Actions.

GA abstract too much, making it feel like "magic" or "black box".
Reproducing workflows locally isn’t straightforward for developers—unless you use nektos/act, which isn’t easy to setup nor to use.

For me, it's the same logic as with Jenkins plugin: the less you rely on it, the happier you are.

Copy link
Collaborator

@MathisMARION MathisMARION Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, it is nice to view the Workflow as a list of commands that one can execute locally by copy-pasting.

I don't know much about Coverity, but it looks quite similar to SonarQube in being "magic / black box".
Sure you download tools, but you have no idea what they do besides produce data in a proprietary format that you then upload to a web platform for processing. Maybe I am wrong, as I have not looked in the details. Anyway, I'm diverging.

run: |
# Download toolbox
COVERITY_URL="https://scan.coverity.com/download/cxx/linux64"
wget -q $COVERITY_URL --post-data "token=${COVERITY_SCAN_TOKEN}&project=wisun-br-linux" -O coverity.tar.gz --no-check-certificate
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why no-check-certificate?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locally, I couldn't check the certificate under the Silabs network, and this issue would also apply to self-hosted runners. We can assume that we won't use self-hosted runners for public repositories, and therefore, for Coverity, we don't need that flag.
However, I wanted to note the first point somewhere.

tar -xf coverity.tar.gz -C coverity --strip-components=1
rm -fr coverity.tar.gz
# For the current shell session
export PATH=$(pwd)/coverity/bin:$PATH
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ln -sfn $(pwd)/coverity/bin/coverity /usr/bin, so you can drop the pain with $PATH

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean ln -sfn $(pwd)/coverity/bin/ /usr/bin ?
Because, the goal is to facilitate the call to toolbox (cov-build, cov-analyze ...) not only coverity.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, ln -sfn $(pwd)/coverity/bin/cov* /usr/bin

@yasslbk yasslbk force-pushed the wip/yla/ci branch 4 times, most recently from 05d1b98 to 76daf49 Compare February 27, 2025 15:33
* For main branch, download toolbox, build wsbrd using coverity then publish results.
* Make sure cov commands are stored in /opt/coverity and accessible by creating a symlink to /usr/local/bin.
@yasslbk
Copy link
Author

yasslbk commented Feb 27, 2025

V2:

  • Run Coverity only for main branch.
  • Coverity's download, build and publish are done a the end. (we create another job called coverage in the same file: link).
  • Coverity is stored in a folder named independently of its version, this would reduce maintenance.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this into a separate workflow then? This will make it easy to tweak the run condition without affecting the regular build.

Also we will be able to use jobs.<job_id>.if instead of jobs.<job_id>.steps[*].if.

COVERITY_SCAN_EMAIL: ${{ secrets.COVERITY_SCAN_EMAIL }}
COVERITY_SCAN_VERSION: ${{ github.sha }}
run: |
tar czf cov-int.tgz cov-int
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "int" mean here?

--form email="$COVERITY_SCAN_EMAIL" \
--form [email protected] \
--form version="$COVERITY_SCAN_VERSION" \
--form description="Coverity scan results for commit $COVERITY_SCAN_VERSION" \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this description necessary? It currently does not provide much information since the version is already provided. I could not even find where this description can be read on the frontend 😅

--form token="$COVERITY_SCAN_TOKEN" \
--form email="$COVERITY_SCAN_EMAIL" \
--form [email protected] \
--form version="$COVERITY_SCAN_VERSION" \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use something more human friendly than a SHA? The output of wsbrd --version uses git describe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants