Clang format action should probably only touch changed files

[alandtse]

https://github.com/doodlum/skyrim-community-shaders/blob/dev/.github/workflows/clang_format.yml

[rjwignar]

I'd like to look into this issue.

[rjwignar]

The Plan

My plan for this was to use another Marketplace job that could retrieve a list of changed files and then pass that list to the clang-format action.

The Good News

The good news is I've been able to use tj-actions/changed-files to make and pass a list of changed files to the clang-format action. Here's a successful run of the updated workflow working on CPP source files (*.cpp, *.h)
https://github.com/rjwignar/skyrim-community-shaders/actions/runs/6987335046/job/19013758175

One version of the updated workflow (targeting only CPP feature code).
https://github.com/rjwignar/skyrim-community-shaders/actions/runs/6987335046/workflow

The Bad News

The clang-format action doesn't play nice with filepaths containing whitespace and will fail if these are passed. I tried extended the changed-files job to also search for changed shader files (*.hlsl, *.hlsli) but many of these files are located in directories with whitespaces, like:

features\Complex Parallax Materials\Shaders\ComplexParallaxMaterials\ComplexParallaxMaterials.hlsli

Here's an example of what it looks like, from a different workflow run and workflow version:

Notice that the CPP source files (with no spaces in their filepaths) are successfully passed to the clang-format job, but the other paths are broken up like below:

features/Complex
Parallax
Materials/Shaders/ComplexParallaxMaterials/CRPM.hlsli

A Possible Solution - Truncate Filepaths

I've tried modifying the updated workflow to process the filepaths retrieved from the changed-files step, such as by wrapping the filepaths in double quotes, single quotes, or escaping the whitespaces. In all cases, the clang-format action will incorrectly parse the list of filepaths, and the workflow file becomes an eyesore.

Truncating all directories with whitespaces (like Complex Parallax Materials to ComplexParallaxMaterials) is one solution, but I wanted to consult with you before making any changes to directory names and submitting a Pull Request

[alandtse]

This can't be an unsolved problem. Have you checked clang-format to see if they've seen this issue? What about piping the list of names into a temporary text file and then ingesting that for clang-format?

The issue with changing the path conventions is I believe some of the code is reliant on parsing the feature path name. I'd try to avoid modifying paths unless it's absolutely not possible.

[rjwignar]

I agree, I'd rather modify just the workflow file.
Clang-format supports passing a file of pathnames using the --files argument but the linting action does that in the background (the action doesn't support us passing our own file).

I've looked at alternatives for DoozyX/clang-format-lint, specifically jidicula/clang-format-action that seems to have solved the filepath parsing issue. However, they don't support the -i flag for inline edits.

I think this an issue with how DoozyX/clang-format-lint-action parses the filepaths.

I'm actually looking into submitting a PR to DoozyX/clang-format-lint-action for this. This might take a few days.

[rjwignar]

Just a short update.

After my experiments with the workflow a couple weeks back, I read through the clang-format-lint repo I found the Pull Request that added multiple-file support. In it, the PR contributor noted that the feature doesn't support filepaths with whitespaces.

In the past couple of weeks, I extended the GitHub Action to support filepaths with whitespaces, and recently submitted a Pull Request to the clang-format-lint repo with my modifications.

The PR still needs to be reviewed and approved by the maintainers, but if my additions are approved, then I can get my enhancements to the clang-format Linter action to work with any changed file inside directories with whitespaces. Until then I hope to work with the maintainers to add this feature.

[alandtse]

Awesome. Thanks for the investigation and work.

[rjwignar]

Apologies for the radio silence.
I've been occupied with the Winter semester after the holidays.
In that time, my PR to the clang-format-lint repo was merged, making this enhancement possible.

I've been able to work on this again during my mid-semester break, and after some experimentation, I've got the clang-format action working on only modified source files (C++ source code and shader files).

I'll have a PR ready tomorrow.

[alandtse]

No worries. Thanks for reaching out.

[FlayaN]

Can this be changed to not be just the previous commit but all the commit in the last push? See this branch for example:
https://github.com/doodlum/skyrim-community-shaders/commits/skylighting-sh/

In this case only the last commit were formatted, not the one before. Due to both of the commits being part of the same git push

For this case the feat: spherical harmonic skylighting commit contained multiple format issues that should be resolved