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 action should probably only touch changed files #128

Open
alandtse opened this issue Oct 10, 2023 · 8 comments · May be fixed by #196
Open

Clang format action should probably only touch changed files #128

alandtse opened this issue Oct 10, 2023 · 8 comments · May be fixed by #196
Assignees
Labels
Build/CI Related to build or CI systems enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@alandtse
Copy link
Collaborator

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

@alandtse alandtse added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Oct 10, 2023
@rjwignar
Copy link
Contributor

I'd like to look into this issue.

@rjwignar
Copy link
Contributor

rjwignar commented Nov 25, 2023

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:
image

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
Copy link
Collaborator Author

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
Copy link
Contributor

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
Copy link
Contributor

rjwignar commented Dec 10, 2023

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
Copy link
Collaborator Author

Awesome. Thanks for the investigation and work.

@rjwignar
Copy link
Contributor

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
Copy link
Collaborator Author

No worries. Thanks for reaching out.

@alandtse alandtse added the Build/CI Related to build or CI systems label Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build/CI Related to build or CI systems enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants