Add automatic formatter for CMake scripts #14011

multiplemonomials · 2020-12-08T05:40:06Z

Summary of changes

This PR implements a basic auto-formatter for CMake scripts and runs it on all the CMake scripts in the repository. If we are going to mandate specific formatting conventions on CMake scripts, then it's great to have a way to enforce those automatically. The scripts use the cmake_format tool and require you to install it before running them.

I tried to configure the formatting config file to match the current code format as close as I could, but there are some things that can't be configured to match (especially related to how certain types of things are wrapped onto different lines). Please check over the formatted code and let me know if we need to change the file in any way.

To use the formatter, just run the script from the root directory of the repository and wait a minute or two (the tool is a bit pokey).

Note: I noticed an odd issue on my system where the Windows and Linux versions of the tool produced somewhat different output for the same files (!!!), so we might want to standardize on using it on only one OS or the other. It could also be a WSL glitch or something, though.

Impact of changes

All CMake code can be automatically formatted by the script. Formatting of some existing code will change.

Migration actions required

None

Documentation

None needed, unless we want to add instructions for running it somewhere (not sure exactly where this would go).

Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@hugueskamba @0xc0170

ciarmcom · 2020-12-08T06:00:22Z

@multiplemonomials, thank you for your changes.
@hugueskamba @0xc0170 @ARMmbed/mbed-os-core @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-test @ARMmbed/mbed-os-connectivity @ARMmbed/mbed-os-maintainers please review.

0xc0170 · 2020-12-08T08:26:21Z

Thanks @multiplemonomials for this formatter. I'll fetch it this week to try locally. Note, most of us (CMake related) are away later this week, it might take longer to review this.

mergify · 2020-12-10T11:04:20Z

This PR cannot be merged due to conflicts. Please rebase to resolve them.

0xc0170 · 2020-12-10T11:34:53Z

Dont rebase yet, we are merging now couple of CMake PRs, for the release. We should be done by tomorrow

adbridge · 2021-01-07T15:50:29Z

@multiplemonomials I'm thinking this should be able to be rebased now. @0xc0170 @hugueskamba any reason to hold back?

ciarmcom · 2021-01-20T12:51:15Z

This pull request has automatically been marked as stale because it has had no recent activity. @multiplemonomials, please carry out any necessary work to get the changes merged. Thank you for your contributions.

multiplemonomials · 2021-01-24T21:06:08Z

@0xc0170 bump^

0xc0170 · 2021-02-01T16:30:13Z

Sorry @multiplemonomials We are having lot of on our plate now to complete. It's on our todo to review in detail, I'll try to get to all oustanding CMake PRs this week. We should definitely adopt this!

rwalton-arm

Looks great, just one comment.

rwalton-arm · 2021-02-02T10:10:58Z

cmake_autoformat.bat

@@ -0,0 +1,22 @@
+@echo off


Could we run cmake-format using the pre-commit tool rather than adding our own custom scripts? There is a pre-commit hook here we could use: https://github.com/cheshirekow/cmake-format-precommit. We get a lot of useful stuff out of the box with pre-commit. It will also allow us to add more formatting and code style checks easily in the future without the requiring us to add multiple "runner" scripts alongside them.

This would be a good start to start running astyle checks as well 👍

0xc0170

I would like to proceed wit this PR for the upcoming release to keep our CMake files consistent similar to the rest of coding files in the tree.

I left only couple of questions, we should write the rules are going to enforce (see my questions) - basically we should have style guideline for CMakelists. Can you capture what style this enforces ? As I haven't used this formatter, neither are familiar what "styles" are there for CMake, I would like to find out more.

How was the config for the formatter chosen (based on other project, or most used style, etc) ?

Update: the format is close to what is there as indicated earlier.

0xc0170 · 2021-02-02T14:44:14Z

.cmake-format.py

+    disable = False
+
+    # How wide to allow formatted cmake files
+    line_width = 80


Can we extend this to 120 (this is what we suggest in code files as well).

UNITTESTS/empty_baseline/unittest.cmake

-  ../connectivity/mbedtls/include/mbedtls/
-  ../connectivity/mbedtls/platform/inc/
-  ../platform/mbed-trace/include/mbed-trace/
+set(unittest-includes


0xc0170 · 2021-02-02T14:49:35Z

tools/cmake/toolchains/GCC_ARM.cmake

-        INTERFACE
-            ${common_options}
-    )
+    target_compile_options(${target} INTERFACE ${common_options})


What options is affecting this line and line above 32 ? is the list above formatted differently because it is over 80 characters long so would not fit on the same line?

0xc0170 · 2021-02-02T14:55:15Z

tools/cmake/profiles/release.cmake

-            INTERFACE
-                $<$<COMPILE_LANGUAGE:CXX>:${cxx_compile_options}>
+        target_compile_options(
+            ${target}


what is the difference in formatting here in this (putting target on the second line), compare to set() here https://github.com/ARMmbed/mbed-os/pull/14011/files#diff-04b1fabd6daf85a88decfec2f6395fb7b0f18a9967b94b20a73859f90a34c713L6 (same line) ?

I am just trying to understand the rules of the formatter so I can manually do the same style :)

0xc0170 · 2021-02-03T12:33:15Z

Shall we use this also on our CMakelists ? They should also be consistent. I am trying it locally but can't figure it out how format works, using the config from here, running cmake-format -c tools\cmake\scripts\cmake-format.py -i drivers/CMakeLists.txt:

target_include_directories(
    mbed-core INTERFACE . ./include ./include/drivers
                        ./include/drivers/internal
)

target_sources(
    mbed-core
    INTERFACE source/AnalogIn.cpp
              source/AnalogOut.cpp

it treats includes differently than sources, different alignment there. I would expect both to be aligned the same way.

I did something similar long time ago for astyle and it took lot of effort to just get it in and execute on our files. Following my latest review, the style set up in cmake-format is not any of "known styles" is it that we could refer to, there is no "standard", is it? For instance like K&R for C/C++. It is more about this is the cmake style we use, here's the tool that styles it for you, don't need to do anything. If that is the case, let's focus here on the configuration of the tool to have and we figure out the details how to get apply this to all files and do it automatically for users.

Steps for adding cmake format:

Configure it for .cmake and CMakelists that are ours. We should use .codecheckignore to ignore the files we can't change.
Update entire codebase to follow the style configured
Add precommit hook for easy to use
Add the style to our documentation

Have I missed anything?

adbridge · 2021-03-10T14:11:31Z

@multiplemonomials any update on this? looks like it also needs a rebase.

multiplemonomials · 2021-03-10T22:09:13Z

I realized that I didn't have time to deal with this properly, so a couple weeks ago I talked to @ladislas and he agreed to take over development and try to fix the remaining issues. He's going to submit a new PR when he has time.

ladislas · 2021-03-11T08:33:04Z

Yep we also need this for our project so when it's setup I'll make a PR here.

Don't have any ETA for the moment though.

0xc0170 · 2021-03-11T11:18:50Z

Thank you both

multiplemonomials added 2 commits December 7, 2020 21:25

Add cmake format config file and script to run the formatter

aea1702

Run autoformatter on all cmake scripts

3c43769

mergify bot added the needs: work label Dec 8, 2020

ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Dec 8, 2020

ciarmcom requested review from 0xc0170, hugueskamba and a team December 8, 2020 06:00

ciarmcom added the needs: review label Dec 8, 2020

0xc0170 requested review from rajkan01 and removed request for a team December 8, 2020 08:25

mergify bot removed the needs: review label Dec 10, 2020

ciarmcom added the stale Stale Pull Request label Jan 20, 2021

0xc0170 mentioned this pull request Feb 1, 2021

Refactor ST Mbed targets to be CMake buildsystem targets #14199

Merged

rwalton-arm reviewed Feb 2, 2021

View reviewed changes

0xc0170 reviewed Feb 2, 2021

View reviewed changes

multiplemonomials closed this Mar 10, 2021

mergify bot removed needs: work release-type: patch Indentifies a PR as containing just a patch stale Stale Pull Request labels Mar 10, 2021

0xc0170 mentioned this pull request Jun 29, 2021

Add cmake-format check to CI #14841

Open

multiplemonomials deleted the cmake-autoformat branch May 17, 2023 06:54

Add automatic formatter for CMake scripts #14011

Add automatic formatter for CMake scripts #14011

Uh oh!

Conversation

multiplemonomials commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Dec 8, 2020

Uh oh!

0xc0170 commented Dec 8, 2020

Uh oh!

mergify bot commented Dec 10, 2020

Uh oh!

0xc0170 commented Dec 10, 2020

Uh oh!

adbridge commented Jan 7, 2021

Uh oh!

ciarmcom commented Jan 20, 2021

Uh oh!

multiplemonomials commented Jan 24, 2021

Uh oh!

0xc0170 commented Feb 1, 2021

Uh oh!

rwalton-arm left a comment

Choose a reason for hiding this comment

Uh oh!

rwalton-arm Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

0xc0170 Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

0xc0170 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

0xc0170 Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

0xc0170 Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adbridge commented Mar 10, 2021

Uh oh!

multiplemonomials commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ladislas commented Mar 11, 2021

Uh oh!

0xc0170 commented Mar 11, 2021

Uh oh!

Uh oh!

multiplemonomials commented Dec 8, 2020 •

edited

Loading

0xc0170 left a comment •

edited

Loading

0xc0170 commented Feb 3, 2021 •

edited

Loading

multiplemonomials commented Mar 10, 2021 •

edited

Loading