Attempting a larger bug fix III - Discussion and Results

Over the past few of weeks I've been working on a bugfix in a GitHub Action. In this blog post I will share discuss my current results and the work I did in the past few weeks.

Background

This work was inspired by my attempt to modify a linting CI workflow in Skyrim Community Shaders (CS) an Open Source graphics mod for Skyrim. For those interested, my attempts to modify the workflow are discussed in this blog post. In short, when working on this CI workflow I found a bug in a GitHub action the workflow uses, which is preventing me from finishing my work on the CS linting workflow.
The GitHub action I investigated became the focus of this bug fix.

The CI workflow

The CI workflow uses an external GitHub action, Doozy/clang-format-lint, to run Clang-Format on the codebase. Currently in the workflow, clang-format-lint is run on the entire codebase. My work on this CI workflow is a modification that passes a list of modified files to the clang-format-lint action, which would prevent untouched files from being analyzed, and would ostensibly speed up the workflow.

My initial work

My idea was to pass a list of changed files to clang-format-lint, either by adding another external GitHub action or modifying the workflow itself. By checking the clang-format-lint discussions and issues, I found an Issue of a user who wanted to use tj-actions/changed-files for this exact purpose, and figured out a method. The changed-files workflow stores a list of all modified files in one of its outputs, all_changed_files. Meaning I could first run the changed-files action, then pass the all_changed_files output to the clang-format-lint action. But what would this look like?

I was able to find where they applied their changes, and referred to it for my own modifications to the Skyrim Shaders workflow:

Let's compare this step to the original workflow:

In my modified workflow outputs.all_changed_files (all changed files) is passed to the clang-format-lint action instead of the entire codebase (.).

The Bug

My modified workflow works great, here's a successful workflow run:

However, this is only when C++ source files are modified. The workflow should also target shader files (files with the .hlsl or .hlsli extensions):

What happens when you modify shader files and run my workflow?

Some of the filepaths are broken apart. This was the original outputs.all_changed_files variable:

features/Complex Parallax Materials/Shaders/ComplexParallaxMaterials/CRPM.hlsli features/Complex Parallax Materials/Shaders/ComplexParallaxMaterials/ComplexParallaxMaterials.hlsli src/Features/DistantTreeLighting.cpp src/Features/DistantTreeLighting.h

The clang-format-lint action should run on the following four (4) filepaths:

features/Complex Parallax Materials/Shaders/ComplexParallaxMaterials/CRPM.hlsli
features/Complex Parallax Materials/Shaders/ComplexParallaxMaterials/ComplexParallaxMaterials.hlsli
src/Features/DistantTreeLighting.cpp
src/Features/DistantTreeLighting.h

...but instead it targets the eight (8):

features/Complex,
Parallax,
Materials/Shaders/ComplexParallaxMaterials/CRPM.hlsli,
features/Complex,
Parallax,
Materials/Shaders/ComplexParallaxMaterials/ComplexParallaxMaterials.hlsli,
src/Features/DistantTreeLighting.cpp,
src/Features/DistantTreeLighting.h

It's clear that filepath with whitespaces are broken apart, but why? When using the original workflow (with . as the source instead of outputs.all_changed_files, these filepaths aren't a problem:

My Investigation

Investigating tj-actions/changed-files

At the time my workflow failed, I wasn't sure exactly where the bug was. Using the original workflow, filepaths with whitespaces didn't cause an issue. I initially thought the changed-files job was breaking apart the filepaths. You see, one of the changed-files usage examples includes a step that lists all changed files:

      # Example 1
      - name: Get changed files
        id: changed-files
        uses: tj-actions/changed-files@v40

        # To compare changes between the current commit and the last pushed remote commit set `since_last_remote_commit: true`. e.g
        # with:
        #   since_last_remote_commit: true 

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

What does the List all changed files step look like in the failed workflow?

A Fool's Errand

Based on the above image, I thought the changed-files action was the cause, and poured a lot of time modifying the changed-files step to fix this bug. I even added a Discussion Post to the changed-files repo and found a potential solution that became the basis of my attempts to fix my workflow. However, 100+ workflow runs later, I accepted this solution would not fix my issue. In fact, the changed-files workflow wasn't breaking apart the outputs.all_changed_files at all!

Let's look at the "list all changed files" step, specifically the for loop:

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

The for loop in this step iterates through outputs.all_changed_files as a whitespace-separated list, but outputs.all_changed_files is still preserved! You can even see the intact value (as a string) in the clang-format-lint step of my failed workflow run:

Therefore, the bug would have to be located in the clang-format-lint action

Investigating clang-format-lint

After investigating the changed-files action, it was clear the bug was in clang-format-lint. Looking at the repo shows a number of files:

The list of filepaths was being split by whitespaces, I decided to investigate run-clang-format.py, the wrapper script that passes all the clang-format-lint options from the workflow to the clang-format tool. My first instinct was to run git grep on run-clang-format.py, searching for instances of the Python string split method:

git grep split run-clang-format.py

Running git grep gave me a few places to look:

Let's take a look at the split_list_arg() function:

def split_list_arg(arg):
    """
    If arg is a list containing a single argument it is split into multiple elements.
    Otherwise it is returned unchanged
    Workaround for GHA not allowing list arguments
    """
    return arg[0].split() if len(arg) == 1 else arg

Let's take a look where split_list_arg() is used:

After some investigating, I determined that args.files is the variable containing the list of filepaths. The docstring for split_list_arg reveals that GitHub Actions doesn't allow list arguments, which matches my observation that outputs.all_changed_files is a single string of whitespace-separated filepaths.

In the original workflow, args.files is the entire codebase (.), so the clang-format-lint action wouldn't break any filepaths inside the root. However, in my workflow args.files is a string of filepaths:

features/Complex Parallax Materials/Shaders/ComplexParallaxMaterials/CRPM.hlsli features/Complex Parallax Materials/Shaders/ComplexParallaxMaterials/ComplexParallaxMaterials.hlsli src/Features/DistantTreeLighting.cpp src/Features/DistantTreeLighting.h

The filepaths containing whitespaces are broken apart by split_list_arg()!

My Bug Fix

Initial Idea - Add a 'Separator' option

Since whitespace-containing filepaths were being broken apart by split_list_arg(), I needed a way to differentiate whitespaces inside a filepath from whitespaces actually separating filepaths. My initial idea was to extend clang-format-lint with a separator argument. The changed-files job actually lets users define a filepath separator when receiving a list of changed files:

If a separator option were added to the clang-format-lint action, a separator argument could be passed from changed-files to properly parse filepaths. However, after consulting my class instructor, I learned this idea may not be ideal. It would require modifying files other than run-clang-format.py, such as action.yml, and due to the scope of these changes, my Pull Request would likely be rejected.
I had to find a better way. I also needed a way to test my changes.

How to test my changes

Because the GitHub Action consists of many files, I wasn't exactly sure how to test my changes. During my earlier investigation I tested the job by running my modified workflow. Would I have to do the same thing to test my changes to the GitHub action? Is there even a way to do that locally, without hosting my own version of the clang-format-lint action? After consulting my class instructor, I learned that I could test the run-clang-format.py file normally. When testing run-clang-format.py I wanted to replicate as many options used in the workflow file:

Development Environment

I tried doing this on Windows, but I found it too unwieldy. I had an easier time replicating these options on Ubuntu. On Ubuntu I was able to easily install different versions of clang-format and was able to run calls like the following:

python3 run-clang-format.py --clang-format-executable /usr/bin/clang-format-16 --inplace "true" --exte
nsions "h,cpp,c,hlsl,hlsli" --exclude "extern include" "[filepaths]"

My final idea - Differentiate whitspaces using RegEx

The RegEx

I wanted to make as few changes to run-clang-format.py as possible to support filepaths with whitespaces.
After playing around with the original run-clang-format.py, I noticed I could escape the filepath whitespaces with backslashes.
For example, the whitespaces within the filepath:

features/Complex Parallax Materials/file.hlsli

Would look like this after escaping the whitespaces:

features/Complex\ Parallax\ Materials/Shaders/file.hlsli

Doing this, it becomes possible to differentiate whitespaces that separates filepaths from whitespaces within filepaths via a Regular Expression. Using the following Regular Expression:

(?<!\\)\s+

I could match all whitespaces except those preceded by a backslash (i.e. ignore whitespaces inside filepaths).

RegEx Dissection

Here's a quick dissection of the Regular Expression:

• \s: Matches a single whitespace character
• \s+: Matches one (1) or more whitespace characters
• (?<!\\)\s+ - Matches (1) or more whitespace characters that are not preceded by a backslash (note the backslash is escaped by another backslash)

Using RegEx to differentiate filepath whitespaces and whitespace separators

By using the re.split() method, I was able to upgrade split_list_arg() to split the list of filepaths through the above RegEx. This would add support for filepaths containing whitespaces, as long as those whitespaces are escaped with a backslash.

Normalizing Backslashes

At this point, I've modified split_list_arg() such that filepaths with whitespaces will be preserved. However, if filepaths are passed as-is, the following error results:

Command

python3 ../../clang-format-lint-action/run-clang-format.py --clang-format-executable /usr/bin/clang-format-16 --inplace "true" --extensions "h,cpp,c,hlsl,hlsli" --exclude "extern include" "features/Complex\ Parallax\ Materials/Shaders/ComplexParallaxMaterials/ComplexParallaxMaterials.hlsli src/Features/DistantTreeLighting.cpp"

Output

Processing 2 files: features/Complex\ Parallax\ Materials/Shaders/ComplexParallaxMaterials/ComplexParallaxMaterials.hlsli, src/Features/DistantTreeLighting.cpp
run-clang-format.py: error: [Errno 2] No such file or directory: 'features/Complex\\ Parallax\\ Materials/Shaders/ComplexParallaxMaterials/ComplexParallaxMaterials.hlsli'

The backslashes must be normalized. A quick Google search suggested I could normalize these paths using the os.path.normpath() method. However, after some tests I found this did not solve my issue. I instead opted to remove these backslashes by using the String replace method to remove the backslashes:

def normalize_paths(paths):
    """
    Normalizes backward slashes in each path in list of paths
    Ex)
        "features/Test\ Features/feature.cpp" => "features/Test Features/feature.cpp"
    """
    return [path.replace("\\","") for path in paths]

Issue and Pull Request

In my past experiences contributing to Open Source projects, I either found an Issue to get assigned to or made one myself before working on code changes. However, for this bug I didn't file an Issue until I made my changes. I did this because at the time I found the bug, I wasn't exactly sure which GitHub Action caused the bug. Even when I saw a possible cause in split_list_arg(), I wasn't confident the bug was here until I fixed it myself. In hindsight, this might have been a bad idea, especially since it's possibly my Pull Request might be rejected.

A Funny Coincidence

While writing my Issue, I searched through the clang-format-lint repository one more time to see if an Issue or Pull Request was ever made related to filepaths with whitespaces, and I found this Pull Request from over three years ago that added split_list_arg() to run-clang-format.py:

The PR contributor even noted their changes did not support filepaths containing whitespaces:

I don't blame the contributor for ignoring folders with whitespaces as they are uncommon. However, I found it hilarious that three years later, I independently noticed the same limitation and upgraded their changes in my Pull Request:

My only regret is that I didn't find this Pull Request sooner, doing so would have immediately showed me where to make my changes, and would have saved me a full day of investigating tj-actions/changed-files!

What I learned

Preparing this Pull Request was a long process. Though the changes are small, a lot of work when into investigating the bug, interacting with different groups of maintainers, and figuring out an appropriate way to make my changes. Over the past few weeks there were a few key events that led to me making this Pull Request:

• working on my contribution to Skyrim Community Shaders
  • figuring a way to pair tj-actions/changed-files to DoozyX/clang-format-lint
  • applying this method to the Skyrim Community Shaders CI workflow
  • finding the broken filepaths on this workflow
• investigating this bug
  • investigating tj-actions/changed-files
  • playing around with tj-actions/changed files in 100+ workflow runs
  • investigating DoozyX/clang-format-action
  • locating split_list_arg
  • determining an appropriate bugfix implementation
• applying my bugfix

The following are a few things I've learned or found interested during this process.

Small changes aren't always easy changes

Though my Pull Request only adds a few lines to the codebase, these changes took a few weeks. In the last few weeks, a lot of time was spent investigating the bug, experimenting with two external GitHub Actions. Once I located the bug in the clang-format-lint action, I had to skim run-clang-format.py for hints based on what I observed (broken paths suggested the use of the split() method for example). Even after that, I had to evaluate and decide between possible implementations of this bug fix.

How to evaluate different bugfix implementations.

Before upgrading the split_list_arg() method I considered modifying the entire GitHub Action to support a separator option just to make it pair well with tj-actions/changed-files. After consulting my class instructor I decided not to pursue this solution because of its invasiveness (multiple files would have to be modified).

Make as few changes as possible

In one of my earlier blog posts, I noted that one of the key tips I learned when contributing to Open Source projects is to make a few changes as possible. In other words, you should aim to write or change as little code possible to get a feature or bugfix working. By following this philosophy, my Pull Request was minimally invasive, and ended up directly building on a previous Pull Request made three years ago!

Interacting with the Open Source Community

Throughout this entire process, I've interacted with the GitHub community in various ways. For one, this entire work was done after discussing with the maintainers of Skyrim Community Shaders. The bug I found could be avoided if all directory names in the Skyrim Community Shaders codebase were truncated (had no whitespaces). We decided not to do this, as some of the code relies on the current naming convention. My discussion with the Skyrim Community Shaders maintainers is what inspired me to investigate both GitHub Actions.

The bread crumb trail of Community Issues, Discussions, and PRs

More important than these posts however are the interactions I've made with past Issues, Discussions, and Pull Requests. I figured how to pair changed-files with clang-format-lint by finding this Issue on the clang-format-lint repo and finding their work.

Shortly after making my discussion post, I found a previous Issue where the changed-files author recommended a possible solution. Although recommendation did not fix my issue, I gained some experience using BASH to modify the all_changed_files output. If my Pull Request is approved,
this experience will be useful in the future, when I need to add backslashes to filepaths containing whitespaces before passing the list of filepaths to clang-format-lint.

Although I found it too late, the Pull Request in clang-format-action that added support for multiple filepaths would have immediately pointed me in the right direction for my changes.

The Open Source community is a library of knowledge

In all of these cases, previous GitHub Issues, Discussions, and Pull Requests have or would have provided breadcrumbs directing my investigation and ultimately, my contribution. These are things people posted up to three years ago, yet months later they provided me lots of insight. If these people didn't post these, I may not have been able to make this Pull Request.

Results

In Part 1 of this series, I listed the following objectives:

1. Confirm the source of the Issue in one of the external GitHub Actions
2. File an Issue in the appropriate GitHub Action
3. Apply the bug fix in the GitHub Action.
4. Test and confirm the fix
5. Make a pull request to the GitHub Action
6. Work with the GitHub Action maintainer(s) on the Pull Request.
7. If/when the pull request is approved, continue working on my original Issue on Skyrim Community Shaders
8. Submit a Pull request to fix the original Issue I was working on

So far, I've completed objectives 1-5. At this point, my Pull Request hasn't been approved, as it needs to be reviewed by the maintainers. I was hoping to have my PR reviewed in time for this blog post but the maintainers might be preoccupied at the moment. Perhaps I should have submitted my PR a few days earlier. The best I can do now is outline my next steps to complete objectives 6-8.

Next Steps

If the changes in my PR are considered, I'll also have to update the clang-format-lint documentation with instructions on handling filepaths with whitespaces. If my PR is approved and merged, I may also have to add a Discussion post to the changed-files repo with instructions on passing whitespace-containing filepaths to clang-lint-format.
Over the next few weeks I plan to do the following:

• update my PR
• work with the maintainers to finalize my PR
• update my work on the Skyrim Community Shaders CI workflow with the new clang-format-lint version.
• submit a PR to Skyrim Community Shaders with the updated CI workflow