Delving Deeper in a Project #2, modifying a CI workflow

This is Part 2 of a two-part series in which I share my experiences going deeper into an Open Source project by making more contribution. The project I contributed to for this series is Skyrim Community Shaders (CS), a popular graphics mod for Skyrim. Part 1 of this blog is available here for those interested.

My Goals - Work on a CI Workflow

In Part 1 of this series, I discussed how I built upon my past work on this project and refactored tooltips for all feature code. For my next contribution I wanted to try something new. Previously in my Open Source class I learned about Static Analysis Tools such as automatic code formatters, and even added them to my own project. In my class I also learned about Continuous Integration (CI) and added a CI workflow to my project using GitHub Actions (GHA). Luckily for me, CS had an issue related to these two concepts.

The Issue - Make Code Formatting Action Touch Changed files

Currently, CS has a GitHub Actions workflow that runs ClangFormat, a C++ automatic code formatter, on C++ source files and shader files. Although the workflow works, the code formatting step is currently configured to work on the entire codebase:

#...
      - uses: DoozyX/clang-format-lint-action@v0.16.2
        with:
          source: "."
          exclude: "extern include"
          extensions: "h,cpp,c,hlsl,hlsli"
          clangFormatVersion: 16
          inplace: True # commit the changes (if there are any)
#...

That is, the formatting step checks if every C++ and shader file satisfies the project's ClangFormat configurations and formats them if they don't. The workflow can be sped up by only passing modified files to the ClangFormat step, since it should be only the modified files that might require reformatting.

My Approach - Pass a list of modified files to the Format Step

My approach for this Issue was to add a step in the workflow that pulls a list of modified files and passes it to the ClangFormat step. The ClangFormat step is actually handled by an external workflow, clang-format lint action (repo). With this in mind, surely there'd be a Marketplace action for retrieving a list of modified files.

Check documentation first

Before considering adding another Marketplace action, I read the clang-format-lint-action documentation to see if it already has an optional argument that makes it run on modified files only. For example, there is a ClangFormat Git integration that runs ClangFormat on modified lines
If clang-format-lint-action somehow used the Git integration or implements something similar, then I would only have to modify the ClangFormat step itself to enable this feature.

You should really check the documentation and repo

Unfortunately, reading the clang-format-lint-action documentation revealed this feature did not currently exist. I added a Discussion post to inquire about this. However, sometime after adding my post I searched past Issues that discussed this feature. I found that another user tried to do the same thing I wanted to do, and figured out how.

The changed-files strategy

It turns out the user used tj-actions/changed-files (repo) to retrieve a list of changed files, and then passed it to clang-format-lint-action. In fact, I was able to find specifically how the user made these changes to their workflow:

# changed-files step above
#...
    - name: Format changed files
      uses: DoozyX/clang-format-lint-action@v0.14
      if: steps.changed-files.outputs.any_changed == 'true'
      with:
        source: ${{ steps.changed-files.outputs.all_changed_files }}
        extensions: 'h,c'
        clangFormatVersion: 11
        inplace: true
        style: file
#...

Open Source is awesome

I like Open Source. Someone had a similar idea to me and figured it out how to implement it. Because their work was done on an Open Source project, I was able to see their work and apply it to my own.

So where's the Pull Request?

At this point, I knew the changes I'd have to make to CS's workflow file. I would add a step that uses tj-actions/changed-files and pass the output to the ClangFormat step. Sounds simple enough, so where's the Pull Request?

It's not that simple

I was actually able to add tj-actions/changed-files to CS's code format workflow and get it working when C++ source files were modified. The successful workflow is available here. However, in the original workflow, I noticed the ClangFormat step is also applied to shader files (.hlsl, .hlsli) files:

#...
      - uses: DoozyX/clang-format-lint-action@v0.16.2
        with:
          # other parameters
          extensions: "h,cpp,c,hlsl,hlsli"
          # other parameters
#...

With this in mind, I tested the new workflow with changes to shader files, and the workflow failed. I examined the workflow deeper:

Notice that the error messages don't appear for the C++ source files.

In a successful run, there are no errors in the formatting step, such as below:

Here are the filepaths of the files modified in my failed workflow:

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

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

src/Features/DistantTreeLighting.cpp

src/Features/DistantTreeLighting.h

Here are the filepaths the ClangFormat step attempts to format:

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

Notice the issue? The filepaths containing spaces in them are being broken apart. But was it happening in the tj-changed-files action or clang-format-lint? The workflow has a step that lists all the changed file before passing it to clang-format:

      - name: List all changed files
        run: |
          for file in ${{ steps.changed-files.outputs.all_changed_files }}; do
            echo "$file was changed"
          done

I initially thought it was the changed-files action breaking apart the list like this, and made a discussion post asking for solutions. I didn't notice it at the time, but pay attention to the underlined portion:

The list of filepaths only appeared broken up when print each string using the default whitespace separator. The list of filepaths was actually fine. Too bad it took me over 100 workflow runs to figure this out (what's that popular definition of insanity, again?):

So where's the bug?

It's clear the bug must be present in the way clang-format-lint parses multiple arguments. The workflow succeeded when I modified only C++ source files, but that's because their file paths don't contain any whitespaces. If a file path contains whitespaces, the action separates the file path into separate strings. I already have plans on fixing this issue in the workflow, but I want to discuss that in the future. I also need to think about some things, like:

• How do I test a GitHub Action locally?
• Since this is an external action, do I need to test it by putting it inside another workflow and test it with that workflow (like I've been doing with my 100+ workflows, but there has to be a better way)?
• How do I test my changes to ensure it doesn't break other features?

The current setup I've been using to debug my workflow was simple. I make changes to my workflow file and run it in GitHub Actions. But how do I debug and test the GitHub Action my workflow relies on?

Why not modify the directory names?

One simple solution to this is to truncate the directory names in CS so that no filepath contains whitespaces. While that is possible, I'd rather not have to modify an entire directory structure to make it play nice with an external action. Additionally, there's the risk that some of the CS codebase depends on the current directory names.

Is there a Pull Request?

There is currently no Pull Request.
I could have made a Pull Request and advise the maintainers to only use the workflow on C++ source files (whose filepaths don't contain whitespaces) and ignore shader files, but I want to instead attempt to fix the bug inside clang-format-lint. For now, in the GitHub Issue I informed the maintainers of my plans.