ci: add action to upload built patch files #325

jonathanlinat · 2024-12-31T14:55:27Z

This PR aims to enhance the current CI workflow by uploading the built patch files to GitHub Actions as artifacts so we can download them and test the changes locally.

It also cleans up the CI.yml file for better readability and usability.

The uploaded artifacts will have the following naming pattern: dashfaction-<git-commit-head-sha>-<mingw|msvc>.zip

is-this-c · 2025-01-01T05:29:38Z

I like this but why is retention-days set to 1 and why does it zip up build? build/bin contains final artifacts.

cmd is deprecated and pwsh is not needed so imo we should set defaults.run.shell to bash for simplicity and use a blank line for clarity between steps.

jonathanlinat · 2025-01-01T22:34:27Z

@is-this-c Got it! I've applied the changes you asked for.

I've reapplied blank lines between the steps for clarity.
I've switched all the steps to bash.
I've set the upload-artifact's path to build/bin.
I've deleted the upload-artifact's retention days property as the default value is set within the current repository settings.

rafalh

Looking at my local build/Release/bin directory and files there take 134.4 MB. Compressed it is 40.5 MB. From what I see in my account billing page only 500 MB of storage is free (Storage for Actions and Packages section). This means that for MinGW alone limit will be reached after 13 commits... Default retention is 3 months. 13 commits for 3 months is not a lot and this is only MinGW. We could filter out debug info (in Mingw build it is in .debug files), then it would be just 14 MB, 5 MB compressed. That would be 100 commits for 3 months. But if MSVC is also stored that would be less (50?). Storing output from two compilers seems redundant, maybe MinGW only would be enough?
Also this storage is shared between projects so if I wanted to store artifacts in my other project that would conflict...
Do we really need this? I know there was no release for some time but I was thinking about one soon. Maybe that will "solve" the problem?

cmd is deprecated and pwsh is not needed so imo we should set defaults.run.shell to bash for simplicity and use a blank line for clarity between steps.

To be honest I prefer cmd over pwsh and I don't think it's officially "deprecated" even if Microsoft tries hard to force everyone to use PowerShell. Anyway I think it's best to stick to the defaults unless it makes things hard. Bash is not native on Windows but it is probably much easier to read last commit with it so it's okay to use it.

rafalh · 2025-01-02T01:34:41Z

.github/workflows/ci.yml

    - name: Install packages
      run: |
        sudo dpkg --add-architecture i386
        sudo apt-get update
        sudo apt-get install --no-install-recommends g++-mingw-w64-i686-posix ninja-build wine32 wine
+      shell: bash


why set shell everywhere? On Linux it is Bash by default

Agreed, I'll remove it from build-mingw.

What about Windows? Do I keep it as an explicit value, as Git provides a Bash shell within Windows-based runners?

rafalh · 2025-01-02T01:36:05Z

.github/workflows/ci.yml


    - uses: actions/checkout@v4

+    - name: Set Current Git Commit Head SHA
+      run: echo "GIT_CURRENT_COMMIT_HEAD_SHA=$(git rev-parse --short=10 HEAD)" >> $GITHUB_ENV


--short by default uses 8 characters, that's probably enough?

Agreed. In fact, I will remove this flag as it is optional.

rafalh · 2025-01-02T01:41:24Z

.github/workflows/ci.yml

@@ -6,50 +6,80 @@ on:

 env:
  BUILD_TYPE: Release
+  WORKING_DIRECTORY: ${{ github.workspace }}/build


not sure we need that var. It is not a working dir in every step so its name may be confusing. Maybe it should be BUILD_DIR?
But from docs I also see that they don't prefix it with ${{github.workspace}} so it is pretty much BUILD_DIR: ./build. With such a simple value I'm not convinced it actually brings much value.

I introduced this global variable as this path is used in multiple places. This is only for consistency and to not repeat.

I agree that the name is not the best. Do you want me to revert this change or only change the variable's name?

is-this-c · 2025-01-02T08:47:37Z

even if Microsoft tries hard to force everyone to use PowerShell.

No one is using cmd these days. Edit: https://x.com/richturn_ms/status/1265155820083240962

What about build artifacts just for whatever commit is latest?

rafalh · 2025-01-02T14:09:12Z

What about build artifacts just for whatever commit is latest?

What do you mean? AFAIK artifacts are linked to workflow runs and I don't know if there is a way to remove artifacts from older runs when a new one finishes. But if there was that would be a good option probably.

jonathanlinat · 2025-01-02T15:07:08Z

Thank you for the reviews. Based on what you said, I would like to propose a couple of things here.

As you mentioned, there are limitations with GitHub Actions regarding hosting artifacts, etc.

What about adjusting the workflow to be triggered only when a Pull Request is open and updated? That will prevent the uploading of artifacts for every commit.
What about re-introducing the retention days property to the Upload action and setting it to 7 days, which is enough to access the generated binaries for testing purposes? If it is needed to get access to the expired binaries, the author of the PR or you can re-run the workflow to generate them back.
What about introducing the option to the Upload action to override the existing binaries if the Pull Request is updated?

Now, I don't have enough experience with the low-coding ecosystem. I would appreciate your help in understanding.

Why does the workflow generate two types of build (mingw and msvc)? What's the purpose? When the patch is officially released, the end user can only access a single binary (As I can see for v1.8.0).
What about keeping only one?

Regarding the rest of the comments, I'll apply the changes you asked for.

is-this-c · 2025-01-03T08:38:50Z

I changed my mind. PR artifacts are not a good idea.

AFAIK artifacts are linked to workflow runs and I don't know if there is a way to remove artifacts from older runs when a new one finishes. But if there was that would be a good option probably.

I believe it is possible. What about this action?

Why does the workflow generate two types of build (mingw and msvc)? What's the purpose?

Ensures compatibility.

rafalh · 2025-01-04T16:36:28Z

What about adjusting the workflow to be triggered only when a Pull Request is open and updated? That will prevent the uploading of artifacts for every commit.

I am against publishing binary artifacts for PR that were not reviewed because they may contain security issues (e.g. a malicious person could upload a vulnerability and then ask people to test it for them). Instead I was thinking about publishing snapshots for code on master branch instead so people can use them before official release.

Why does the workflow generate two types of build (mingw and msvc)? What's the purpose? When the patch is officially released, the end user can only access a single binary (As I can see for v1.8.0).

I want the project to be compatible with both MSVC and MinGW so people can use a compiler they prefer. Unfortunately there are some differences and a code that is successfully compiled by MSVC may fail to compile in MinGW (or the opposite). Having it compatible with two compilers increases the changes that it is standard-compliant.
When I release Dash Faction I use MinGW for compiling because Linux is my main OS (if I remember correctly some older versions of Dash were built using MSVC, but I don't remember when I switched).

jonathanlinat · 2025-01-07T14:32:48Z

Got it. So, do I have to close this PR, or is there anything that you want me to do specifically?

ci: add action to upload built patch files

72ee284

ci: remove retention days for upload and switch to bash shell

79aa4f9

rafalh reviewed Jan 2, 2025

View reviewed changes

jonathanlinat requested a review from rafalh January 2, 2025 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: add action to upload built patch files #325

ci: add action to upload built patch files #325

jonathanlinat commented Dec 31, 2024 •

edited

Loading

is-this-c commented Jan 1, 2025

jonathanlinat commented Jan 1, 2025

rafalh left a comment •

edited

Loading

rafalh Jan 2, 2025

jonathanlinat Jan 2, 2025

rafalh Jan 2, 2025

jonathanlinat Jan 2, 2025

rafalh Jan 2, 2025

jonathanlinat Jan 2, 2025

is-this-c commented Jan 2, 2025 •

edited

Loading

rafalh commented Jan 2, 2025

jonathanlinat commented Jan 2, 2025 •

edited

Loading

is-this-c commented Jan 3, 2025

rafalh commented Jan 4, 2025

jonathanlinat commented Jan 7, 2025

ci: add action to upload built patch files #325

Are you sure you want to change the base?

ci: add action to upload built patch files #325

Conversation

jonathanlinat commented Dec 31, 2024 • edited Loading

is-this-c commented Jan 1, 2025

jonathanlinat commented Jan 1, 2025

rafalh left a comment • edited Loading

Choose a reason for hiding this comment

rafalh Jan 2, 2025

Choose a reason for hiding this comment

jonathanlinat Jan 2, 2025

Choose a reason for hiding this comment

rafalh Jan 2, 2025

Choose a reason for hiding this comment

jonathanlinat Jan 2, 2025

Choose a reason for hiding this comment

rafalh Jan 2, 2025

Choose a reason for hiding this comment

jonathanlinat Jan 2, 2025

Choose a reason for hiding this comment

is-this-c commented Jan 2, 2025 • edited Loading

rafalh commented Jan 2, 2025

jonathanlinat commented Jan 2, 2025 • edited Loading

is-this-c commented Jan 3, 2025

rafalh commented Jan 4, 2025

jonathanlinat commented Jan 7, 2025

jonathanlinat commented Dec 31, 2024 •

edited

Loading

rafalh left a comment •

edited

Loading

is-this-c commented Jan 2, 2025 •

edited

Loading

jonathanlinat commented Jan 2, 2025 •

edited

Loading