chore(falco): apply code formatting #3350

poiana · 2024-09-27T11:51:05Z

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind cleanup

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area engine

/area tests

/area CI

What this PR does / why we need it:

As per title and linting files in the repo.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

sgaist

Might be a silly question but is the code formatter configured with the correct definition ?
Many of the changes are moving the opening curly bracket to the end of the line rather than to a new line as is the usual in the code base.

sgaist · 2024-09-27T12:03:45Z

cmake/modules/static-analysis.cmake

+	add_custom_target(
+		cppcheck
+		COMMAND
+			${CPPCHECK} "--enable=all" "--force" "--inconclusive" "--inline-suppr" # allows to
+																				   # specify
+																				   # suppressions
+																				   # directly in
+																				   # source code
+			"--xml" # we want to generate a report
+			"--output-file=${CMAKE_CURRENT_BINARY_DIR}/static-analysis-reports/cppcheck/cppcheck.xml" # generate
+																									  # the report
+																									  # under the
+																									  # reports
+																									  # folder in
+																									  # the build
+																									  # folder
+			"-i${CMAKE_CURRENT_BINARY_DIR}" # exclude the build folder
+			"${CMAKE_SOURCE_DIR}"
+	)


This block would require better formatting than what is suggested.

sgaist · 2024-09-27T12:04:29Z

cmake/modules/static-analysis.cmake

-  message(STATUS "cppcheck command not found, static code analysis using cppcheck will not be available.")
+	message(
+		STATUS
+			"cppcheck command not found, static code analysis using cppcheck will not be available."


I would align it to STATUS

Suggested change

"cppcheck command not found, static code analysis using cppcheck will not be available."

"cppcheck command not found, static code analysis using cppcheck will not be available."

sgaist · 2024-09-27T12:05:31Z

cmake/modules/falcosecurity-libs.cmake

-  add_definitions(-DHAVE_STRLCAT)
+	message(
+		STATUS
+			"Existing strlcpy and strlcat found, will *not* use local definition by setting -DHAVE_STRLCPY and -DHAVE_STRLCAT."


Suggested change

"Existing strlcpy and strlcat found, will *not* use local definition by setting -DHAVE_STRLCPY and -DHAVE_STRLCAT."

"Existing strlcpy and strlcat found, will *not* use local definition by setting -DHAVE_STRLCPY and -DHAVE_STRLCAT."

sgaist · 2024-09-27T12:07:16Z

cmake/modules/CompilerFlags.cmake

-		_HAS_STD_BYTE=0
-		_CRT_SECURE_NO_WARNINGS
-		WIN32
-		MINIMAL_BUILD
-		WIN32_LEAN_AND_MEAN
-		NOMINMAX
+		_HAS_STD_BYTE=0 _CRT_SECURE_NO_WARNINGS WIN32 MINIMAL_BUILD WIN32_LEAN_AND_MEAN NOMINMAX


If possible, I'd rather keep the column variant.

sgaist · 2024-09-27T12:08:45Z

unit_tests/CMakeLists.txt

+				falco/app/actions/test_configure_interesting_sets.cpp
+				falco/app/actions/test_configure_syscall_buffer_num.cpp


Suggested change

falco/app/actions/test_configure_interesting_sets.cpp

falco/app/actions/test_configure_syscall_buffer_num.cpp

falco/app/actions/test_configure_interesting_sets.cpp

falco/app/actions/test_configure_syscall_buffer_num.cpp

sgaist · 2024-09-27T12:10:43Z

unit_tests/CMakeLists.txt

+			${CMAKE_BINARY_DIR}/userspace/falco # we need it to include indirectly `config_falco.h`
+												# file
+			${CMAKE_SOURCE_DIR}/userspace/engine # we need it to include indirectly `falco_common.h`
+												 # file
+			${CMAKE_CURRENT_BINARY_DIR} # we need it to include `falco_test_var.h`


Should we put the comments above each line ?

Suggested change

${CMAKE_BINARY_DIR}/userspace/falco # we need it to include indirectly `config_falco.h`

# file

${CMAKE_SOURCE_DIR}/userspace/engine # we need it to include indirectly `falco_common.h`

# file

${CMAKE_CURRENT_BINARY_DIR} # we need it to include `falco_test_var.h`

# we need it to include indirectly `config_falco.h` file

${CMAKE_BINARY_DIR}/userspace/falco

# we need it to include indirectly `falco_common.h` file

${CMAKE_SOURCE_DIR}/userspace/engine

# we need it to include `falco_test_var.h`

${CMAKE_CURRENT_BINARY_DIR}

sgaist · 2024-09-27T12:21:55Z

userspace/engine/falco_engine_version.h

+#define FALCO_ENGINE_VERSION                                                               \
+	__FALCO_ENGINE_STRINGIFY(FALCO_ENGINE_VERSION_MAJOR)                                   \
+	"." __FALCO_ENGINE_STRINGIFY(FALCO_ENGINE_VERSION_MINOR) "." __FALCO_ENGINE_STRINGIFY( \
+	        FALCO_ENGINE_VERSION_PATCH)


I find this one less clear to parse. I would rather keep the original.

FedeDP · 2024-09-27T12:40:49Z

@sgaist yep there might be something wrong with this PR since the format code job is still failing 🤣

LucaGuerra · 2024-09-27T12:51:04Z

Some lint was not applied correctly, and also it looks like CI is still failing. Will probably need some tweaking 🤔

Andreagit97 · 2024-09-27T14:27:46Z

Some lint was not applied correctly, and also it looks like CI is still failing. Will probably need some tweaking 🤔

Yep I've done the same for libs. There are usually 2/3 places where clang format cannot converge without your help, so probably you need to change these lines manually... Moreover probably is better to run it more than once to be sure a convergence is reached (maybe just run the CI jobs some times before merging the PR)

For what concern the style I would keep the same we used in libs, with the same clang-format, cmake-format, but of course this is just my opinion

sgaist · 2024-09-27T16:01:51Z

Some lint was not applied correctly, and also it looks like CI is still failing. Will probably need some tweaking 🤔

Yep I've done the same for libs. There are usually 2/3 places where clang format cannot converge without your help, so probably you need to change these lines manually... Moreover probably is better to run it more than once to be sure a convergence is reached (maybe just run the CI jobs some times before merging the PR)

For what concern the style I would keep the same we used in libs, with the same clang-format, cmake-format, but of course this is just my opinion

This would make a big change in Falco but I am all for a consistent code style across repositories.
Does anyone have an objection regarding that ?

FedeDP · 2024-09-30T08:22:01Z

Does anyone have an objection regarding that ?

I don't, since it is the same formatting we now follow in libs, it's ok for me.

LucaGuerra · 2024-09-30T08:56:12Z

Ok for me. It's a start to have a code style :)

Signed-off-by: Poiana <[email protected]>

FedeDP

/approve

poiana · 2024-09-30T11:24:11Z

LGTM label has been added.

Git tree hash: 355da5b3177a66870377abbc4d30b7ebadc37d6a

poiana · 2024-09-30T11:24:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, poiana

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [FedeDP]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

FedeDP · 2024-09-30T11:24:18Z

/milestone 0.39.0

sgaist

Approving as most the concerns have been fixed by an actual run of the linter.

Details will be fixed in followup work.

poiana added release-note-none kind/cleanup dco-signoff: yes area/build area/engine area/tests area/ci size/XXL labels Sep 27, 2024

poiana requested review from Kaizhe and sgaist September 27, 2024 11:51

sgaist requested changes Sep 27, 2024

View reviewed changes

poiana assigned sgaist Sep 27, 2024

poiana force-pushed the chore/falco-code-formatting branch from 15eaa13 to 6a1a33b Compare September 27, 2024 12:36

poiana force-pushed the chore/falco-code-formatting branch from 6a1a33b to 36078f4 Compare September 30, 2024 08:54

chore(falco): apply code formatting

b671a6f

Signed-off-by: Poiana <[email protected]>

poiana force-pushed the chore/falco-code-formatting branch from 36078f4 to b671a6f Compare September 30, 2024 10:06

FedeDP approved these changes Sep 30, 2024

View reviewed changes

poiana assigned FedeDP Sep 30, 2024

poiana added the lgtm label Sep 30, 2024

poiana added the approved label Sep 30, 2024

poiana added this to the 0.39.0 milestone Sep 30, 2024

sgaist approved these changes Sep 30, 2024

View reviewed changes

poiana merged commit 50b98b3 into master Sep 30, 2024
36 checks passed

poiana deleted the chore/falco-code-formatting branch September 30, 2024 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(falco): apply code formatting #3350

chore(falco): apply code formatting #3350

poiana commented Sep 27, 2024

sgaist left a comment

sgaist Sep 27, 2024

sgaist Sep 27, 2024

sgaist Sep 27, 2024

sgaist Sep 27, 2024

sgaist Sep 27, 2024

sgaist Sep 27, 2024

sgaist Sep 27, 2024

FedeDP commented Sep 27, 2024

LucaGuerra commented Sep 27, 2024

Andreagit97 commented Sep 27, 2024 •

edited

Loading

sgaist commented Sep 27, 2024

FedeDP commented Sep 30, 2024

LucaGuerra commented Sep 30, 2024

FedeDP left a comment

poiana commented Sep 30, 2024

poiana commented Sep 30, 2024

FedeDP commented Sep 30, 2024

sgaist left a comment

	"cppcheck command not found, static code analysis using cppcheck will not be available."
	"cppcheck command not found, static code analysis using cppcheck will not be available."

	"Existing strlcpy and strlcat found, will not use local definition by setting -DHAVE_STRLCPY and -DHAVE_STRLCAT."
	"Existing strlcpy and strlcat found, will not use local definition by setting -DHAVE_STRLCPY and -DHAVE_STRLCAT."

		falco/app/actions/test_configure_interesting_sets.cpp
		falco/app/actions/test_configure_syscall_buffer_num.cpp

chore(falco): apply code formatting #3350

chore(falco): apply code formatting #3350

Conversation

poiana commented Sep 27, 2024

sgaist left a comment

Choose a reason for hiding this comment

sgaist Sep 27, 2024

Choose a reason for hiding this comment

sgaist Sep 27, 2024

Choose a reason for hiding this comment

sgaist Sep 27, 2024

Choose a reason for hiding this comment

sgaist Sep 27, 2024

Choose a reason for hiding this comment

sgaist Sep 27, 2024

Choose a reason for hiding this comment

sgaist Sep 27, 2024

Choose a reason for hiding this comment

sgaist Sep 27, 2024

Choose a reason for hiding this comment

FedeDP commented Sep 27, 2024

LucaGuerra commented Sep 27, 2024

Andreagit97 commented Sep 27, 2024 • edited Loading

sgaist commented Sep 27, 2024

FedeDP commented Sep 30, 2024

LucaGuerra commented Sep 30, 2024

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Sep 30, 2024

poiana commented Sep 30, 2024

FedeDP commented Sep 30, 2024

sgaist left a comment

Choose a reason for hiding this comment

Andreagit97 commented Sep 27, 2024 •

edited

Loading