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

iox-#1368 Add cmake linter and formatter scripts #1885

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

zmostafa
Copy link
Contributor

@zmostafa zmostafa commented Feb 8, 2023

Signed-off-by: Ziad Mostafa [email protected]

Pre-Review Checklist for the PR Author

  1. Code follows the coding style of CONTRIBUTING.md
  2. Tests follow the best practice for testing
  3. Changelog updated in the unreleased section including API breaking changes
  4. Branch follows the naming format (iox-123-this-is-a-branch)
  5. Commits messages are according to this guideline
  6. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  7. Relevant issues are linked
  8. Add sensible notes for the reviewer
  9. All checks have passed (except task-list-completed)
  10. All touched (C/C++) source code files from iceoryx_hoofs are added to ./clang-tidy-diff-scans.txt
  11. Assign PR to reviewer

Notes for Reviewer

@dkroenke I create this PR to address issue #1368 .

I am using cmakelang and have some open points stll:

  1. The linter/formatter file in this PR contains the default config used by the tools, I just changed the tab size to 4 instead of 2, I think we need to revisit this config as applying them locally drastically changed the files.
  2. CI is not yet configured with the format/lint step, I will address this in later commits.

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Code according to our coding style and naming conventions
  • Unit tests have been written for new behavior
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

@zmostafa zmostafa marked this pull request as draft February 8, 2023 12:09
@mossmaurice mossmaurice added the tooling All iceoryx related tooling (scripts etc.) label Feb 8, 2023
@dkroenke dkroenke self-requested a review February 8, 2023 17:14
@@ -1,17 +1,17 @@
# Copyright (c) 2019 - 2021 by Robert Bosch GmbH. All rights reserved.
# Copyright (c) 2020 - 2022 by Apex.AI Inc. All rights reserved.
# Copyright (c) 2019 - 2021 by Robert Bosch GmbH. All rights reserved. Copyright
Copy link
Member

Choose a reason for hiding this comment

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

The copyright header should not be reformatted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I will need to fence this block from formatting, so disabling the formatter before it and then enable it after

Comment on lines 57 to 66
BUILD_INTERFACE
${PROJECT_SOURCE_DIR}/include
${PROJECT_SOURCE_DIR}/legacy/include
${PROJECT_SOURCE_DIR}/memory/include
${PROJECT_SOURCE_DIR}/container/include
${PROJECT_SOURCE_DIR}/vocabulary/include
${PROJECT_SOURCE_DIR}/utility/include
${PROJECT_SOURCE_DIR}/primitives/include
${PROJECT_SOURCE_DIR}/design/include
${CMAKE_BINARY_DIR}/generated/iceoryx_hoofs/include
Copy link
Member

Choose a reason for hiding this comment

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

Can we have an additional layer additional indentation?

Suggested change
BUILD_INTERFACE
${PROJECT_SOURCE_DIR}/include
${PROJECT_SOURCE_DIR}/legacy/include
${PROJECT_SOURCE_DIR}/memory/include
${PROJECT_SOURCE_DIR}/container/include
${PROJECT_SOURCE_DIR}/vocabulary/include
${PROJECT_SOURCE_DIR}/utility/include
${PROJECT_SOURCE_DIR}/primitives/include
${PROJECT_SOURCE_DIR}/design/include
${CMAKE_BINARY_DIR}/generated/iceoryx_hoofs/include
BUILD_INTERFACE
${PROJECT_SOURCE_DIR}/include
....
INSTALL_INTERFACE
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried my best to maintain the same form of indentation, but it seems that I do not have a control on the number of spaces/tabs between the columns. Please check the last commit

if(CMAKE_SYSTEM_NAME MATCHES Linux OR CMAKE_SYSTEM_NAME MATCHES Darwin)
option(BUILD_SHARED_LIBS "Create shared libraries by default" ON)
endif()

set(PREFIX iceoryx/v${CMAKE_PROJECT_VERSION})

#
########## build iceoryx hoofs lib ##########
# ######### build iceoryx hoofs lib ##########
Copy link
Member

Choose a reason for hiding this comment

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

I guess the formatter interprets this as comment, maybe another separator symbol would be good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The formatter forces a space between the comment mark and the being of the comment, I am disabling the format before this section as a solution,what do you think ?

@zmostafa zmostafa force-pushed the iox-1368-add-cmake-format-and-lint branch from 001866c to cc2ff14 Compare February 13, 2023 18:54
@codecov
Copy link

codecov bot commented Feb 13, 2023

Codecov Report

Merging #1885 (0acf769) into master (ace5f51) will decrease coverage by 0.51%.
The diff coverage is n/a.

❗ Current head 0acf769 differs from pull request most recent head cc2ff14. Consider uploading reports for the commit cc2ff14 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1885      +/-   ##
==========================================
- Coverage   75.47%   74.97%   -0.51%     
==========================================
  Files         383      383              
  Lines       15166    14692     -474     
  Branches     2146     2099      -47     
==========================================
- Hits        11447    11015     -432     
+ Misses       3047     3031      -16     
+ Partials      672      646      -26     
Flag Coverage Δ
unittests 74.63% <ø> (-0.50%) ⬇️
unittests_timing 15.29% <ø> (-0.13%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...eoryx_hoofs/design/source/functional_interface.cpp 0.00% <0.00%> (-100.00%) ⬇️
...ryx_hoofs/source/error_handling/error_handling.cpp 0.00% <0.00%> (-100.00%) ⬇️
iceoryx_hoofs/source/cxx/requires.cpp 33.33% <0.00%> (-66.67%) ⬇️
iceoryx_binding_c/source/c_runtime.cpp 52.63% <0.00%> (-27.37%) ⬇️
iceoryx_hoofs/source/concurrent/loffli.cpp 74.28% <0.00%> (-10.57%) ⬇️
iceoryx_posh/source/mepoo/mepoo_config.cpp 90.62% <0.00%> (-9.38%) ⬇️
...design/include/iox/detail/functional_interface.inl 87.80% <0.00%> (-7.32%) ⬇️
...posh/include/iceoryx_posh/internal/roudi/roudi.hpp 77.77% <0.00%> (-5.56%) ⬇️
...include/iceoryx_hoofs/internal/concurrent/sofi.inl 85.00% <0.00%> (-4.48%) ⬇️
...osh/include/iceoryx_posh/internal/popo/trigger.inl 90.00% <0.00%> (-3.75%) ⬇️
... and 150 more

@elBoberido
Copy link
Member

@zmostafa are you planing to continue working on this PR?

@zmostafa
Copy link
Contributor Author

@elBoberido Yes, I am waiting for @dkroenke 's feedback for the new format, since applying cmake lint will change the current files format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling All iceoryx related tooling (scripts etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Linter/Formatter for CMake files
4 participants