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

ci: clang_format.yml lints only modified source files #196

Merged
merged 8 commits into from
Jul 4, 2024

Conversation

rjwignar
Copy link
Contributor

@rjwignar rjwignar commented Feb 29, 2024

This fixes #128

Summary

Added two steps to .github/workflows/clang_format.yml to lint only modified C++ source files and shader files (.hlsli, .hlsl)

changed-files

The changed-files step uses tj-actions/changed-files to retrieve a comma-separated list of changed C++ and shader files. The comma-separated list is then processed and passed to the Format changed files step

process-list

The process-list step processes the filelist to be properly passed to DoozyX/clang-format-lint-action

filepaths with whitespaces are made unix-friendly
(e.g., Complex Parallax Materials/file.hlsl --> Complex\ Parallax\ Materials/file.hlsl)

commas replaced with whitespaces to separate multiple filepaths
(e.g., file.hlsl,src/Features/DistantTreeLighting.cpp --> file.hlsl src/Features/DistantTreeLighting.cpp)

Changes to Format changed files step

Upgraded DoozyX/clang-format-lint-action from v0.16.2 to v0.17 to allow filepaths containing whitespaces

This step now only executes if the changed-files step retrieves any modified files.

Test runs

A test version of this workflow is available at rjwignar/issue-128-test, where rjwignar/issue-128-test was added as a triggering branch.
Here are some example runs:

@rjwignar
Copy link
Contributor Author

Leaving draft PR up for now, will finalize the PR tomorrow.

@rjwignar rjwignar changed the title Rjwignar/issue 128 CI: clang_format.yml lints only modified source files Feb 29, 2024
@rjwignar rjwignar changed the title CI: clang_format.yml lints only modified source files ci: clang_format.yml lints only modified source files Feb 29, 2024
@rjwignar rjwignar marked this pull request as ready for review February 29, 2024 16:58
id: changed-files
uses: tj-actions/changed-files@v42.0.5
with:
files: |
Copy link
Collaborator

@FlayaN FlayaN Feb 29, 2024

Choose a reason for hiding this comment

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

This glob needs to be improved, we also have cpp source files directly under src (Not only features subfolder). Also under include

Can look here for some glob hints
https://github.com/doodlum/skyrim-community-shaders/blob/dev/cmake/AddCXXFiles.cmake

Copy link
Collaborator

Choose a reason for hiding this comment

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

@FlayaN you think you can handle the globs easily?

Copy link
Collaborator

@FlayaN FlayaN Mar 13, 2024

Choose a reason for hiding this comment

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

Hello! I don't have time for the next 2 weeks to fix the globs (And test them). I can however guess :P

Looking at documentation at: https://github.com/tj-actions/changed-files

It looks like they support the src/** glob syntax.
So my guess for a good glob would be:

files: |
  src/**
  include/**
  **/*.hlsl
  **/*.hlsli

Copy link
Collaborator

Choose a reason for hiding this comment

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

@FlayaN since you're on a roll, you want to take this over the line?

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 can give this another look in a couple days

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 recently updated the glob with the help of https://codepen.io/mrmlnc/pen/OXQjMe:

  files: |
    src/**/*.{cpp,h}
    include/**/*.{h,hpp}
    **/*.hlsl
    **/*.hlsli

Here's a couple example runs of the updated glob on rjwignar/issue-128-test:

Test Commit Clang-Format Job
Update cpp/h files in src rjwignar@c17a2f6 https://github.com/rjwignar/skyrim-community-shaders/actions/runs/9409659767/job/25920004938
Update h/hpp files in include rjwignar@51b9322 https://github.com/rjwignar/skyrim-community-shaders/actions/runs/9409737196/job/25920220892

The recent force-push was due to a rebase of my branch (rjwignar/issue-128) on top of the latest dev branch
Please let me know if additional changes are required.

@alandtse
Copy link
Collaborator

alandtse commented Mar 2, 2024

Thanks again. Please note I am waiting for the issues raised by @FlayaN to be resolved before merging.

@alandtse
Copy link
Collaborator

@rjwignar thanks again. Just checking to see if you think you'll be able to address or if we should merge and someone else will pick up the other source code locations.

@rjwignar
Copy link
Contributor Author

Hi @alandtse thanks for reaching out.

I've been meaning to update the glob but I've been bogged down in class commitments.
I likely won't have time to work on this again until April.

If the team can wait until then we can keep this open.
Otherwise, it might be best to merge this PR and address the remaining source code locations in a follow-up, provided this PR is approved.

@rjwignar rjwignar force-pushed the rjwignar/issue-128 branch from 9088c8a to 9e1a4ff Compare June 7, 2024 00:50
@alandtse
Copy link
Collaborator

alandtse commented Jun 8, 2024

Thanks. We're in a major refactor right now but after that's in, I'll see about merging.

@alandtse
Copy link
Collaborator

alandtse commented Jul 4, 2024

Ok, refactor is done. Thanks again for the help!

@alandtse alandtse merged commit 28f58d7 into doodlum:dev Jul 4, 2024
3 checks passed
alandtse added a commit that referenced this pull request Jul 13, 2024
@alandtse alandtse mentioned this pull request Jul 13, 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 action should probably only touch changed files
3 participants