Attempting a larger bug fix III - Discussion and Results
Published: 2023-12-12 02:50:25

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)](https://github.com/doodlum/skyrim-community-shaders) an Open Source graphics mod for Skyrim. For those interested, my attempts to modify the workflow are discussed in [this blog post](https://dev.to/rjwignar/delving-deeper-in-a-project-2-modifying-a-ci-workflow-2j9n). 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](https://github.com/doodlum/skyrim-community-shaders/blob/a1f70c8224a370de6a6cb91d8212137fdc09cfe5/.github/workflows/clang_format.yml#L31-L37) uses an external GitHub action, [Doozy/clang-format-lint](https://github.com/DoozyX/clang-format-lint-action), to run [Clang-Format](https://clang.llvm.org/docs/ClangFormat.html) 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](https://github.com/DoozyX/clang-format-lint-action/issues/44) of a user who wanted to use [tj-actions/changed-files](https://github.com/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](https://docs.github.com/en/actions/using-jobs/defining-outputs-for-jobs), [all_changed_files](https://github.com/tj-actions/changed-files#output_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](https://github.com/apollo-lhc/cm_mcu/commit/c2792682211c2252063eaa1261df2af014a78f01), and referred to it for [my own modifications](https://github.com/rjwignar/skyrim-community-shaders/blob/309bac8f3b1714ec68cb4b09746be8cc2b7fc2c6/.github/workflows/clang_format.yml#L43-L51) to the Skyrim Shaders workflow:

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/dxgueh7ehkdqkm1w5q6c.png)

Let's compare this step to the original workflow:
![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/nm7bxgnzt0stzf0esnds.png)

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](https://github.com/rjwignar/skyrim-community-shaders/actions/runs/6987335046/job/19013758175):

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/176jv100cfkhs41ffljp.png)

However, this is only when C++ source files are modified. The workflow should also target shader files (files with the `.hlsl` or `.hlsli` extensions):
![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/f0znatq9e4nv0agjif2v.png)

What happens when you modify shader files and [run my workflow](https://github.com/rjwignar/skyrim-community-shaders/actions/runs/6984314480/job/19006907593)?

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/7l3433bwf82i4gs2xqca.png)

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](https://github.com/doodlum/skyrim-community-shaders/actions/runs/6940700959/job/18880093848#step:4:11):

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/r9lpug6fwlao8rb4zz2r.png)

## My Investigation

### Investigating [tj-actions/changed-files](https://github.com/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:
```shell
      # 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?

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/rn2aegtu1u4mvgtvssdf.png)

### 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](https://github.com/tj-actions/changed-files/discussions/1738) to the changed-files repo and found a [potential solution](https://github.com/tj-actions/changed-files/discussions/1738#discussioncomment-7663815) that became the basis of my attempts to fix my workflow. However, **<u>100+</u>** 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:
```yaml
      - 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](https://github.com/rjwignar/skyrim-community-shaders/actions/runs/6984314480/job/19006907593#step:6:10):

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/s97dr3grq9n2cpuy1g0v.png)

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

### Investigating [clang-format-lint](https://github.com/DoozyX/clang-format-lint-action)

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:

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/q985ldnk7f1spa5mjor5.png)

The list of filepaths was being split by whitespaces, I decided to investigate [run-clang-format.py](https://github.com/DoozyX/clang-format-lint-action/blob/master/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](https://git-scm.com/docs/git-grep) on `run-clang-format.py`, searching for instances of the [Python string split method](https://www.w3schools.com/python/ref_string_split.asp):
```
git grep split run-clang-format.py
```

Running `git grep` gave me a few places to look:
![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/3xacf3e8rjhzxam48bls.png)

Let's take a look at the [split_list_arg()](https://github.com/DoozyX/clang-format-lint-action/blob/master/run-clang-format.py#L245-L251) function:

```python
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](https://github.com/DoozyX/clang-format-lint-action/blob/master/run-clang-format.py#L351-L355):

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/r86ys3duay7vblq919j9.png)

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](https://github.com/tj-actions/changed-files#examples-) when receiving a list of changed files:

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/2jnjzek8pfqahx3ng7ow.png)

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:

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/9v0k3vtpqarko6x12de4.png)

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

```command
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](https://docs.python.org/3/library/re.html#re.split), 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:

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/t6daaqdejt2a8j740og2.png)

Command

```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](https://docs.python.org/3/library/os.path.html#os.path.normpath). 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](https://www.w3schools.com/python/ref_string_replace.asp) to remove the backslashes:

```python
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](https://github.com/DoozyX/clang-format-lint-action/issues/62), 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](https://github.com/DoozyX/clang-format-lint-action/pull/14) from over three years ago that added `split_list_arg()` to `run-clang-format.py`:

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/qypkeyy1shh7fprq16az.png)

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

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/b16yybv1yvhfj86ozxoi.png)

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](https://github.com/DoozyX/clang-format-lint-action/pull/63):

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/4er3u41pb3z68ooyqouu.png)

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](https://github.com/doodlum/skyrim-community-shaders/issues/128)
  - 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](https://github.com/rjwignar/skyrim-community-shaders/actions/workflows/clang_format.yml)
  - 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](https://dev.to/rjwignar/markdown-first-contributions-and-pull-requests-5d7o), 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](https://github.com/doodlum/skyrim-community-shaders/issues/128#issuecomment-1826528344). 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](https://github.com/DoozyX/clang-format-lint-action/issues/44) on the `clang-format-lint` repo and finding [their work](https://github.com/apollo-lhc/cm_mcu/commit/c2792682211c2252063eaa1261df2af014a78f01).

Shortly after making my [discussion post](https://github.com/tj-actions/changed-files/discussions/1738), I found a [previous Issue](https://github.com/tj-actions/changed-files/issues/609#issuecomment-1236559836) 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](https://github.com/DoozyX/clang-format-lint-action/pull/14) 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](https://github.com/doodlum/skyrim-community-shaders)
8. Submit a Pull request to fix the [original Issue](https://github.com/doodlum/skyrim-community-shaders/issues/128) 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