SCA: Add Eclair support #68324

simhein · 2024-01-31T08:38:22Z

The PR add support for the SCA tool ECLAIR from Bugseng.

The rule set which is added with the commits was adapted to the zephyr projects coding guidelines.

Note: The integration was a collaboration with Bugseng itself therefor Simone Ballarin autohred some commits

Hint: All ecl files are files which are used by the eclair tool to set up rules and other options.

tejlmand

Thanks for this.

The ecl configuration files have not been reviewed as i'm not familiar with the tool itself, not its configuration file / its settings.

Thus only the integration to the build system has been reviewed.

From what I can understand here: https://www.bugseng.com/filebrowser/download/379
then Eclair also supports Windows, therefore I would like to see a more generic (cross-platform) integration.

tejlmand · 2024-02-05T12:40:33Z

cmake/sca/eclair/sca.cmake

+                  -b ${BOARD} \
+                  --build-dir ${ECLAIR_BUILD_DIR} \
+                  ${APPLICATION_SOURCE_DIR} \
+                  | tee ${ECLAIR_OUTPUT_DIR}/analysis.log"


is this the only way to get output from Eclair ?

Doesn't it support writing the analysis to a file directly ?

Are you referring to the tee?

The tee is not related to ECLAIR (or to the analysis results), it just saves the west invocation logs in
analysis.log. I'm fine with removing it. In case of problems, we still will have something reported in
$ECLAIR_DIAGNOSTICS_OUTPUT.

Doing that, if the build fails it will result in an ECLAIR error in $ECLAIR_DIAGNOSTICS_OUTPUT,
but the actual build error will not be present (maybe this is not even necessary, maybe west already
put these logs somewhere).

Since a broken build is not related to ECLAIR I'm completely fine in removing the tee.

tejlmand · 2024-02-05T12:40:36Z

cmake/sca/eclair/sca.cmake

+message(STATUS "ECLAIR BUILD DIR is: ${ECLAIR_BUILD_DIR}")
+
+add_custom_target(eclair ALL
+  COMMAND sh -c "ECLAIR_PROJECT_NAME=${ECLAIR_PROJECT_NAME} \


we should avoid depending on a specific shell, especially this will not work on windows, and afaict Eclair also supports running on Windows.

If needing to set env settings and run a tool, one can use:

${CMAKE_COMMAND} -E env <options> -- <command> ...

See more: https://cmake.org/cmake/help/latest/manual/cmake.1.html#run-a-command-line-tool

tejlmand · 2024-02-05T12:44:54Z

cmake/sca/eclair/sca.cmake

+endif()
+
+# Create Output Directory
+file(REMOVE_RECURSE ${ECLAIR_OUTPUT_DIR})


This will only be cleared when CMake re-runs, not for normal incremental builds.

Is that intentionally ?
So far, looking at this PR, i'm not convinced the current behavior is what is really desired.

No, you're right. The directory is supposed to be deleted at every run.

tejlmand · 2024-02-05T12:57:01Z

cmake/sca/eclair/sca.cmake

+)
+
+add_custom_command(
+  TARGET eclair POST_BUILD


why is this a POST_BUILD command ?

The add_custom_target() support running multiple commands, like:

add_custom_target(<target> <command1> <args1> COMMAND <command2> <args2> COMMAND <command3>....)

Or alternative, switch purely to add_custom_command() with outputs an then have the target drive those commands.

tejlmand · 2024-02-05T12:58:56Z

cmake/sca/eclair/sca.cmake

+  COMMAND_EXPAND_LISTS
+)
+
+unset(ECLAIR_RULES_SET CACHE)


I don't see ECLAIR_RULES_SET being set in the cache.

Can you explain ?

tejlmand · 2024-02-05T13:05:42Z

cmake/sca/eclair/sca.cmake

+set(LD_ALIASES "${CMAKE_LINKER}")
+set(AR_ALIASES "${CMAKE_ASM_COMPILER_AR} ${CMAKE_C_COMPILER_AR} ${CMAKE_CXX_COMPILER_AR}")
+if(NOT ECLAIR_RULES_SET)
+  set(ECLAIR_RULES_SET first_analysis)


not sure what the intention with this setting is ?

This variable is only accessible during CMake configure time, which is not the same as build time.

The custom target and command below are only executed during build time, and thus eclair may be invoked multiple times before CMake re-runs.

tejlmand · 2024-02-05T14:03:43Z

cmake/sca/eclair/sca.cmake

+unset(ECLAIR_metrics_tab CACHE)
+unset(ECLAIR_reports_tab CACHE)
+unset(ECLAIR_summary_txt CACHE)
+unset(ECLAIR_summary_doc CACHE)
+unset(ECLAIR_summary_odt CACHE)
+unset(ECLAIR_full_txt_areas CACHE)
+unset(ECLAIR_full_txt CACHE)
+unset(ECLAIR_full_doc_areas CACHE)
+unset(ECLAIR_full_doc CACHE)
+unset(ECLAIR_full_odt CACHE)


Not understood, what's the intention ?

Unset eventually externally provided variables that alter the behavior of the tool.
Just to avoid unexpected behaviors from the tool caused by previous runs settings.

tejlmand · 2024-02-05T15:12:19Z

cmake/sca/eclair/ECL/reports.ecl

-#-metrics_tab=join_paths(output_dir,"metrics")
-# Output reports for use with spreadsheet applications
-#-reports_tab=join_paths(output_dir,"reports")
+if(string_equal(or(getenv("ECLAIR_metrics_tab"),"false"),"true")


Using env variables here might be very convenient for someone who wrote the integration but it seems very hidden and undocumented for everyone else.

Also picking up environment variables during build can lead to hard to reproduce builds.
We should probably consider to allow SCA tools to provide a Kconfig file.

They are not undocumented, you can find the documentation in sca/eclair.rst.
In any case, I've got the point. I'm not really familiar with Kconfig, I'll give a look
at it.

They are not undocumented, you can find the documentation in sca/eclair.rst.

Try read my comment again.
I'm not talking about the settings themselves, but the fact they are being picked up from environment.
That behavior is hidden and undocumented.

zephyr/doc/develop/sca/eclair.rst

Lines 66 to 67 in 6b67643

ECLAIR can generate additional report formats (e.g. DOC, ODT, XLSX) in addition to the

default ecd file. To enable them, you can set the following variables to true:

zephyr/doc/develop/sca/eclair.rst

Line 93 in 6b67643

west build -b mimxrt1064_evk samples/basic/blinky -- -DZEPHYR_SCA_VARIANT=eclair -DECLAIR_summary_txt=true

That is true allowing SCA tools to provide a Kconfig would be good thing to make it clearer what can be configured for the tool itself and also would document the configuration and behavior of the sca tooling better.

But is there actual a better way to do something like this with SCA and cmake? Because I couldn't find a better way to do this right now.

doc/develop/sca/eclair.rst

simhein · 2024-03-19T10:18:34Z

@tejlmand Simone (@sballari ) and me worked on this PR and we are ready to push the recent updates, we also included the configuration of the SCA via Kconfig.
One question remains open, shall we put that addition of the Kconfig into a separate PR or can we include it into this one, because as you mentioned: "we should consider SCA tools to allow Kconfigs" is a topic we might need to discuss project wide.

simhein · 2024-03-26T11:18:51Z

Last push address the changes requested from @tejlmand and introduce a solution to use Kconfig with a SCA tool.

tejlmand

was looking at the updated PR.

Please cleanup the commit history, as it is hard to review commit-by-commit when coding issues identified in earlier review is still present in PR because new commits like those 99ccd51a5a9615227ee98b457d6935e26fb6dde1, 0322ed29e9e42c35d2fb1543f51b494c5b934f01, ...
are just appended to the PR.

tejlmand · 2024-04-10T12:04:44Z

cmake/sca/eclair/sca.cmake

with the new hw model in place, should this then be Zephyr-${BOARD}${BOARD_QUALIFIERS} ?

Non-blocking comment.

Yes with the new model in place this will be switched to ${BOARD}${BOARD_QUALIFIERS}

Time to switch this over now that HWMv2 is the norm

simhein · 2024-04-12T14:26:51Z

was looking at the updated PR.

Please cleanup the commit history, as it is hard to review commit-by-commit when coding issues identified in earlier review is still present in PR because new commits like those 99ccd51, 0322ed2, ... are just appended to the PR.

Ok I will look into it. So just to confirmation that I understood it correct, it is better to squash the new commits into the old ones get rid of the older changes?

simhein · 2024-05-21T09:29:41Z

@tejlmand cleaned up the complete PR to get rid of the appended commits and make it easier to review.

dleach02 · 2024-05-30T21:07:49Z

@simhein can you address the compliance checks?

simhein · 2024-06-03T17:03:02Z

@simhein can you address the compliance checks?

@dleach02 I tried to address them, but I couldn't find out yet how to fix them. It seems that the check have issues with introducing the Kconfig for SCA tools.

simhein · 2024-10-23T13:22:05Z

@tejlmand @pdgendt I have removed the Kconfig approach and implemented the option() variant with the the cmake options file.
As discussed the Kconfig maybe should only be for FW configurations, I will create a Issue to discuss these options and to find a common solution for sca integrations.

pdgendt · 2024-10-23T13:38:47Z

@tejlmand @pdgendt I have removed the Kconfig approach and implemented the option() variant with the the cmake options file. I as discussed the Kconfig maybe should only be for FW configurations, I will create a Issue to discuss these options and find a common solution for sca integrations.

Thanks for the continued efforts! I think the last updates are very reasonable and align with current zephyr documentation. Will do a review soon.

cmake/sca/eclair/ECL/common_config.ecl

pdgendt · 2024-10-23T15:43:49Z

cmake/sca/eclair/ECL/extra.ecl

What about networking linker sections?

Yes they might need to be extended here if they start showing up in violations when the tool can't find those entities.
But I would recommend to add them if the tool starts showing this kind of violations or warnings or infos

cmake/sca/eclair/ECL/public_APIs.ecl

cmake/sca/eclair/ECL/toolchain.ecl

cmake/sca/eclair/sca.cmake

pdgendt

Nit

cmake/sca/eclair/ECL/language_extensions.ecl

simhein · 2024-10-24T13:00:58Z

@pdgendt I adapted the configuration in the last push. If people use the preset rulesets all configurations are used which are defined in the new config file zephyr_common_config.ecl.
Useres which are using ECLAIR_USER_RULESET_NAME need to configure everthing in the user rulset and the tool will simply analyze everthing if not suppresed or configured by the user configuration.

The next step after the integration would be to revisit the zephyr_common_config.ecl and review the whole configuration for the project.

cmake/sca/eclair/sca.cmake

Add the Eclair configuration files, which are needed to configure the static code analysis tool for the zephyr coding guidelines. Signed-off-by: Simon Hein <[email protected]>

cmake/sca/eclair/sca.cmake

Add the ECLAIR calls for the zephyr cmake environment to call ECLAIR while the firmware is build by replacing the actual compiler call and setup the eclair environment and call the compiler through the eclair. The Integration accepts a kconfig file for configuring the analysis and the generation of the reports. The path of the kconfig file should be provided via the variable ECLAIR_CONFIG. db_generation.ecl has be created and introduced instead of reports.ecl because the report generation is handled by the sca.cmake directly. Signed-off-by: Simon Hein <[email protected]>

Add the documentation for the eclair from Bugseng with the pre configuration for the zephyr project. Signed-off-by: Simon Hein <[email protected]>

Add a cmake file which uses the cmake options feature and include it inot the sca.cmake file to set up and describe the options for the ECLAIR tool. Signed-off-by: Simon Hein <[email protected]>

pdgendt · 2024-10-24T15:05:53Z

Can't really comment on the ecl files, but otherwise LGTM

simhein · 2024-10-29T16:09:05Z

ping @tejlmand

tejlmand

Thanks for the effort, and sorry for the KConfig vs. CMake back and forth.

But I do think that the latest CMake approach looks better than the initially proposed CMake variant which hid the supported options too much.

Also I think having the sca_options.cmake established as the user facing configuration allows us too extend functionality without having to change user's workflow.

Approved.

tejlmand · 2024-11-11T15:46:16Z

cmake/sca/eclair/sca.cmake

+find_program(ECLAIR_REPORT eclair_report REQUIRED)
+message(STATUS "Found eclair_report: ${ECLAIR_REPORT}")
+
+if(ECLAIR_OPTIONS_FILE)


Not blocking as we can continue work on Eclair integration, but should the initial value of ECLAIR_OPTIONS_FILE be fetched using zephyr_get(ECLAIR_OPTIONS_FILE) so that it also uses the intended file when invoked through sysbuild ?

zephyrbot added area: Build System area: Documentation labels Jan 31, 2024

zephyrbot requested review from 57300, carlescufi, jeremybettis, kartben, nashif, nordicjm and tejlmand January 31, 2024 08:39

zephyrbot assigned tejlmand Jan 31, 2024

simhein force-pushed the eclair_env_integration branch 2 times, most recently from 77079e8 to 6b67643 Compare February 1, 2024 07:31

simhein added the area: Coding Guidelines Coding guidelines and style label Feb 2, 2024

simhein added this to the v3.6.0 milestone Feb 2, 2024

tejlmand requested changes Feb 5, 2024

View reviewed changes

henrikbrixandersen removed this from the v3.6.0 milestone Feb 6, 2024

nordicjm removed their request for review February 7, 2024 08:14

dleach02 reviewed Mar 5, 2024

View reviewed changes

doc/develop/sca/eclair.rst Show resolved Hide resolved

simhein force-pushed the eclair_env_integration branch from 6b67643 to 08be9f6 Compare March 26, 2024 11:15

simhein force-pushed the eclair_env_integration branch from 08be9f6 to 57dbf4d Compare March 26, 2024 15:47

tejlmand requested changes Apr 10, 2024

View reviewed changes

simhein force-pushed the eclair_env_integration branch from 57dbf4d to 84cf55c Compare May 21, 2024 09:28

simhein requested a review from tejlmand May 21, 2024 12:16

simhein force-pushed the eclair_env_integration branch from 84cf55c to 1d4b4b8 Compare May 28, 2024 06:59

simhein force-pushed the eclair_env_integration branch from c08d51b to 4dca8fd Compare October 23, 2024 13:18

simhein force-pushed the eclair_env_integration branch from 4dca8fd to 93e5781 Compare October 23, 2024 13:24

pdgendt reviewed Oct 23, 2024

View reviewed changes

simhein force-pushed the eclair_env_integration branch from 93e5781 to 75dc43c Compare October 24, 2024 12:56

pdgendt previously approved these changes Oct 24, 2024

View reviewed changes

cmake/sca/eclair/ECL/language_extensions.ecl Outdated Show resolved Hide resolved

simhein dismissed pdgendt’s stale review via 3dc1181 October 24, 2024 13:03

simhein force-pushed the eclair_env_integration branch from 75dc43c to 3dc1181 Compare October 24, 2024 13:03

pdgendt reviewed Oct 24, 2024

View reviewed changes

cmake/sca/eclair/sca.cmake Outdated Show resolved Hide resolved

cmake/sca/eclair/sca.cmake Outdated Show resolved Hide resolved

sca: Add ECLAIR SCA configuration files.

dfccc74

Add the Eclair configuration files, which are needed to configure the static code analysis tool for the zephyr coding guidelines. Signed-off-by: Simon Hein <[email protected]>

simhein force-pushed the eclair_env_integration branch from 3dc1181 to a0897a8 Compare October 24, 2024 14:29

pdgendt reviewed Oct 24, 2024

View reviewed changes

cmake/sca/eclair/sca.cmake Outdated Show resolved Hide resolved

cmake/sca/eclair/sca.cmake Outdated Show resolved Hide resolved

simhein added 3 commits October 24, 2024 16:59

doc: sca: Add Bugseng eclair documentation

00ee881

Add the documentation for the eclair from Bugseng with the pre configuration for the zephyr project. Signed-off-by: Simon Hein <[email protected]>

sca: Add cmake options file for tool configuration

5c7c25d

Add a cmake file which uses the cmake options feature and include it inot the sca.cmake file to set up and describe the options for the ECLAIR tool. Signed-off-by: Simon Hein <[email protected]>

simhein force-pushed the eclair_env_integration branch from a0897a8 to 5c7c25d Compare October 24, 2024 14:59

pdgendt approved these changes Oct 24, 2024

View reviewed changes

mmahadevan108 modified the milestones: v4.0.0, v4.1.0 Nov 8, 2024

nashif approved these changes Nov 8, 2024

View reviewed changes

carlescufi requested a review from tejlmand November 11, 2024 10:49

tejlmand reviewed Nov 11, 2024

View reviewed changes

tejlmand approved these changes Nov 11, 2024

View reviewed changes

nashif merged commit d4da23e into zephyrproject-rtos:main Nov 16, 2024
23 checks passed

simhein deleted the eclair_env_integration branch November 18, 2024 07:56

simhein mentioned this pull request Nov 18, 2024

SCA: Get eclair options file value for sysbuild invokation #81512

Merged

	ECLAIR can generate additional report formats (e.g. DOC, ODT, XLSX) in addition to the
	default ecd file. To enable them, you can set the following variables to true:

SCA: Add Eclair support #68324

SCA: Add Eclair support #68324

Uh oh!

Conversation

simhein commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tejlmand left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

simhein commented Mar 19, 2024

Uh oh!

simhein commented Mar 26, 2024

Uh oh!

tejlmand left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simhein commented Apr 12, 2024

Uh oh!

simhein commented May 21, 2024

Uh oh!

dleach02 commented May 30, 2024

Uh oh!

simhein commented Jun 3, 2024

Uh oh!

simhein commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pdgendt commented Oct 23, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pdgendt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

simhein commented Oct 24, 2024

simhein commented Jan 31, 2024 •

edited

Loading

simhein commented Oct 23, 2024 •

edited

Loading