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

feature: Add pipeline to check code formatting #1783

Merged
merged 4 commits into from
Jan 7, 2025

Conversation

johnbowen42
Copy link
Contributor

@johnbowen42 johnbowen42 commented Jan 7, 2025

Resolves #1717

@johnbowen42 johnbowen42 force-pushed the feature/bowen/clang-format-ci-pipeline branch 3 times, most recently from 16163f8 to 4918545 Compare January 7, 2025 02:14
@johnbowen42 johnbowen42 force-pushed the feature/bowen/clang-format-ci-pipeline branch from 4918545 to 8bc8bb8 Compare January 7, 2025 02:20
@johnbowen42 johnbowen42 requested review from rhornung67, a team and davidbeckingsale January 7, 2025 19:32
@davidbeckingsale
Copy link
Member

Why is the script removed?

@rhornung67
Copy link
Member

Why is the script removed?

Same question from me.

@rhornung67
Copy link
Member

@johnbowen42 do we need your branch for this PR activated in RTD anymore?

// decltype(add.eval(tile))>(left.eval(tile),
// right.eval(tile), add.eval(tile)))
ADD_TYPE const&
add) //->
Copy link
Member

Choose a reason for hiding this comment

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

This change looks odd. Any idea why clang-format did this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

honestly, no, this one is perplexing

@johnbowen42
Copy link
Contributor Author

Why is the script removed?

Same question from me.

This script was old and is not how we apply clang format now. The consistent way to apply clang format is either with the provided pre-commit git hook or the style cmake target.

@@ -71,6 +71,15 @@ RUN cmake -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_BUILD_TYPE=Debug -DENABLE_OPENMP
ctest -T test --output-on-failure && \
make clean

FROM ghcr.io/llnl/radiuss:clang-14-ubuntu-22.04 AS clang14_style
USER root
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we want to specify "root" as the user?

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'm pretty sure we want to specify root. This is how axom does it https://github.com/LLNL/axom/blob/511ae0ae6fe474448fa7dd5dfe16ae6d91fc47b9/azure-pipelines.yml#L124, because cmake needs write permissions for the files

Copy link
Member

Choose a reason for hiding this comment

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

Good catch @adayton1 We probably don't want or need that.

@johnbowen42 johnbowen42 merged commit b44606e into develop Jan 7, 2025
33 checks passed
@rhornung67
Copy link
Member

@johnbowen42 if you don't want yout PR branch anymore, please delete.

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.

Add clang-format to RAJA CI checks
4 participants