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

Add an option for using clang format and set it to off by default #509

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

jmcarcell
Copy link
Member

@jmcarcell jmcarcell commented Oct 30, 2023

BEGINRELEASENOTES

  • Add an option for using clang format and set it to off by default. It is significantly slower to run cmake with this option on.

ENDRELEASENOTES

I don't think most users care and we could run clang-format for the builds in the stacks, but if you are building I think you would rather get to whatever you want to do sooner. We could turn on the option for the stack builds but for developer builds the OFF option makes more sense in my opinion.

I have some times for configure without downloading the test files. Rebuild means when for example you make a change in a CMakeLists.txt file so that configure has to run again when you try to build:

On my machine (all local):

From scratch Rebuild
With clang-format 16 s 14 s
Without clang-format 6 s 4 s

On a build node (Alma 9):

From scratch Rebuild
With clang-format 27 s 20 s
Without clang-format 20 s 13 s

@jmcarcell jmcarcell changed the title Create a variant using clang format and set it to off by default Add an option for using clang format and set it to off by default Oct 31, 2023
@tmadlener
Copy link
Collaborator

I think cmake -DPODIO_USE_CLANG_FORMAT=OFF should already work right now without these changes, since we set this here:

set(PODIO_USE_CLANG_FORMAT AUTO CACHE STRING "Try to use clang-format to format the code generated by podio")
set_property(CACHE PODIO_USE_CLANG_FORMAT PROPERTY STRINGS AUTO ON OFF)

@jmcarcell
Copy link
Member Author

Yes, the change here would be to have it OFF by default and for stacks and whoever wants it to have it ON since it doesn't have any impact unless you look at the files (and how often does this happen anyway?)

@tmadlener
Copy link
Collaborator

If we set this to OFF by default, can we at least have one CI workflow where we have it AUTO / ON with a suitable clang-format available? Just to avoid inadvertently breaking anything in the future.

@tmadlener tmadlener merged commit 401e71a into AIDASoft:master Nov 6, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants