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

Clang format ldmx-sw #1362

Merged
merged 17 commits into from
Jun 7, 2024
Merged

Clang format ldmx-sw #1362

merged 17 commits into from
Jun 7, 2024

Conversation

tvami
Copy link
Member

@tvami tvami commented Jun 7, 2024

I am updating ldmx-sw, here are the details.

What are the issues that this addresses?

This resolves #1294

Check List

  • I successfully compiled ldmx-sw with my developments
  • I ran my developments and the following shows that they are successful.

Running

git ls-tree -r HEAD --name-only | egrep '(\.h|\.cxx)$' > format.list
clang-format --style=Google -Werror --dry-run $(cat format.list)

returns nothing now!
There is absolutely nothing expected to change, this is a cosmetics only PR.

After this is merged, we should change the CI to make the clang format checks automatically.

Copy link
Member

@tomeichlersmith tomeichlersmith left a comment

Choose a reason for hiding this comment

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

I'm thinking we could introduce the clang-format testing in this PR as well and I don't think it would be too hard given we've done it for other submodules in the past.

  • Copy .clang-format from Framework/.clang-format (it's just saying using the Google style right now, but having a file will make it easier in the future to define ldmx-sw wide format definitions)
  • Copy over action from Framework or SimCore (under .github/workflows/format-check.yml in either of those directories). To avoid submodules, probably would want to update the script that either of those runs .github/format. I'd be happy with either writing the few lines of code into the action yaml or putting a format script into our scripts/ directory which we can call from CI.
  • Remove the formatting infra from Framework/SimCore. git rm -r Framework/.clang-format Framework/.github SimCore/.clang-format SimCore/.github (just cleaning up)

@tvami
Copy link
Member Author

tvami commented Jun 7, 2024

I'd be happy with either writing the few lines of code into the action yaml

I'm not sure how to skip the submodules. For now I did the rest in 7c988da

@tvami
Copy link
Member Author

tvami commented Jun 7, 2024

oh I think it works out of the box: https://github.com/LDMX-Software/ldmx-sw/actions/runs/9421674449/job/25956257560
Screenshot 2024-06-07 at 11 43 58

@tomeichlersmith
Copy link
Member

For skipping submodules, it would mainly entail changing the find ... command to the git ls-tree ... command to avoid files in another repo. It looks like the files in the other repos either follow the Google style as well or have a different extension so we don't check them.

find . -type f \( -name '*.h' -o -name '*.cxx' \) > ${TMPDIR:-/tmp}/files-to-format.list

That is what I meant, but luckily there is another way! Just have the action not even clone the submodules :) then we are all good.

- uses: actions/checkout@v3

@tvami tvami merged commit 8bbabd1 into trunk Jun 7, 2024
2 checks passed
@tvami tvami deleted the iss1294-clang-format branch June 7, 2024 18:51
@tvami
Copy link
Member Author

tvami commented Jun 7, 2024

Just have the action not even clone the submodules :)

ahh that's why it worked, because I didnt have the recursive option setup. Gotcha!

@tvami tvami mentioned this pull request Jul 18, 2024
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.

Clang Format the Source Code
3 participants