Skip to content

Conversation

@nashif
Copy link
Member

@nashif nashif commented Jan 8, 2025

Add cmake style guidelines.

Signed-off-by: Anas Nashif [email protected]

@nashif nashif force-pushed the topic/cmake/style branch 2 times, most recently from 7305c71 to 98d0920 Compare January 8, 2025 22:08
@nashif nashif marked this pull request as ready for review January 9, 2025 12:41
Copy link
Contributor

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Looks OK minus nit. in examples

Copy link
Contributor

Choose a reason for hiding this comment

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

these use 4 space indent

Copy link
Contributor

Choose a reason for hiding this comment

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

it still uses 4 space indent.

@nashif nashif force-pushed the topic/cmake/style branch from 98d0920 to 3bbe790 Compare January 13, 2025 16:12
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps: Use Uppercase for Cache Variables or variables shared across CMake files

as we do have variables that are shared / re-used throughout the CMake tree but are not cache variables.

Copy link
Contributor

@tejlmand tejlmand Jan 14, 2025

Choose a reason for hiding this comment

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

Do you mean function arguments ?
You cannot have two CMake commands on a single line.

Also the example code block has two args on same line my_target and PRIVATE, which I think in this case is perfectly valid, but as PRIVATE technically is independent flag, then it becomes a bit unclear when this is acceptable, and when it should be avoided.

I think I know your intentions, and I agree with them, but current sentence is imprecise.

Sorry for not having a better suggestion atm.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, maybe make this about files instead of commands, so this becomes clearte, i.e. One file per line

@nashif nashif force-pushed the topic/cmake/style branch from 3bbe790 to f2a382a Compare January 16, 2025 12:17
Copy link
Contributor

Choose a reason for hiding this comment

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

it still uses 4 space indent.

Comment on lines 49 to 50
Copy link
Contributor

Choose a reason for hiding this comment

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

this uses four space indent.

Btw. do we want just the indent or alignment ?
I'm fine either way, but prefer consistency.

And should function calls spanning multiple lines wrap first arg also, that is if we only indent and not align the args.

A)

target_sources(my_target PRIVATE
  src/file1.cpp
  src/file2.cpp
)

A.a)

target_sources(
  my_target PRIVATE
  src/file1.cpp
  src/file2.cpp
)

B)

target_sources(my_target PRIVATE
               src/file1.cpp
               src/file2.cpp
)

Copy link
Contributor

Choose a reason for hiding this comment

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

this uses 4 space indent.

@nashif nashif force-pushed the topic/cmake/style branch from 93c56c2 to f82fc91 Compare January 16, 2025 14:39
tejlmand
tejlmand previously approved these changes Jan 16, 2025
Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

A good start 👍

@rettichschnidi
Copy link
Contributor

rettichschnidi commented Jan 17, 2025

Bit late to the party, but has https://github.com/BlankSpruce/gersemi/ been considered to be used instead of putting formatting details (use 2 spaces, no space after if, etc.) into a textual guideline?

@pdgendt
Copy link
Contributor

pdgendt commented Jan 17, 2025

Bit late to the party, but has BlankSpruce/gersemi been considered to be used instead of putting formatting details (use 2 spaces, no space after if, etc.) into a textual guideline?

Only attempt of CMake formatting was #82839, I think.

@tejlmand
Copy link
Contributor

tejlmand commented Jan 20, 2025

but has https://github.com/BlankSpruce/gersemi/ been considered to be used instead of putting formatting details (use 2 spaces, no space after if, etc.) into a textual guideline?

not sure if this specific formatter has been tried.
I tried various other formatters in the past with no good results, and @nordicjm has recently tried a lot of other formatters / linters with #82839 as the most promising one.

As a reference, I can also point to: https://discourse.cmake.org/t/cmake-linter/8391 from June 2023 which contains a reply from Craig Scott.

[buildSystemPerson]
CMake linter?
Does anyone have any recommendations for a CMake linter?

[Craig Scott]
I realise this is controversial, but my response at the moment is, frankly, don’t use one. In all cases where I’ve seen them used in any meaningfully complicated project, it gives a worse result than manual formatting.

...

So atm I think the best we can do is to have CMake format recommendations in writing.

pdgendt
pdgendt previously approved these changes Jan 20, 2025
Add cmake style guidelines.

Signed-off-by: Anas Nashif <[email protected]>
@kartben kartben merged commit 6d6808f into zephyrproject-rtos:main Jan 20, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants