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

Added firmware static analysis to CI #165

Merged
merged 3 commits into from
Jan 10, 2024

Conversation

italo-sampaio
Copy link
Collaborator

@italo-sampaio italo-sampaio commented Dec 14, 2023

  • Merged old static analysis scripts into a single script
  • Added static analysis to the CI on pushes to master
  • Generated report is available for download on job summary

@italo-sampaio italo-sampaio force-pushed the enhancement/add-static-analysis branch from c9ea90d to 363064b Compare December 14, 2023 13:13
@italo-sampaio italo-sampaio marked this pull request as draft December 14, 2023 22:23
Unified the signer and ui static analysis scripts into a single script
is located under the more specific folder ledger/static-analysis. Also
modified the directory of the generated reports to be under the
ledger/static-analysis folder.
Added a new action to CI that runs static analysis on the firmware code.
A failure in static analysis won't prevent the checks from passing, this
is only informational at the moment. The generated reports are uploaded
as a github artifact and can be analyzed offline.
@italo-sampaio italo-sampaio force-pushed the enhancement/add-static-analysis branch from 363064b to 5764341 Compare January 9, 2024 17:56
@italo-sampaio
Copy link
Collaborator Author

An example run can be seen here.

@italo-sampaio italo-sampaio marked this pull request as ready for review January 9, 2024 18:13
@italo-sampaio italo-sampaio changed the title Added static analysis to firmware Added firmware static analysis to CI Jan 9, 2024
amendelzon
amendelzon previously approved these changes Jan 9, 2024
Copy link
Collaborator

@amendelzon amendelzon left a comment

Choose a reason for hiding this comment

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

LGTM!

As a question: I wonder whether it would make a difference using the arm clang toolchain we use for building firmware for the Nano S instead of the system /usr/bin/clang*.

@italo-sampaio
Copy link
Collaborator Author

LGTM!

As a question: I wonder whether it would make a difference using the arm clang toolchain we use for building firmware for the Nano S instead of the system /usr/bin/clang*.

Good question. The clang used for building the firmware is actually located at /usr/bin/clang. This is set both in the signer and ui Makefiles. The value of CLANGPATH is set globally in the ledger Dockerfile and resolves to /usr/bin/.

I'll update the script to also use the CLANGPATH env variable to avoid confusion and also make it compatible with eventual future changes.

This makes it clear that we are using the same compiler for building the firmware and for static analysis
Copy link
Collaborator

@amendelzon amendelzon left a comment

Choose a reason for hiding this comment

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

LGTM!

@amendelzon amendelzon merged commit 8a0ecdc into master Jan 10, 2024
6 checks passed
@amendelzon amendelzon deleted the enhancement/add-static-analysis branch January 10, 2024 20:09
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.

2 participants