search

NiyaShy / XB1ControllerBatteryIndicator

A tray application that shows a battery indicator for an Xbox-ish controller and gives a notification when the battery level drops to (almost) empty.
GNU General Public License v2.0
710 stars 53 forks source link

Add GitHub Actions CI support #45

Open smiley opened 3 years ago

smiley commented 3 years ago

This PR adds a GitHub Actions workflow file for building the app automatically, and runs on each open PR and commit on master. Since this is a fairly simple C# app using NuGet for dependencies, it's a classic fit for Actions.

It passes CI in my own fork: smiley/XB1ControllerBatteryIndicator#1

NiyaShy commented 3 years ago

Hey, thanks a lot for the PR. This looks very interesting and (from what I can see on your fork) works fine. I just have 2 "nitpicks":

smiley commented 3 years ago

The resulting archives contain too many (unnecessary) files. Since the program is mainly targeting gamers and not that many of them are tech/coding-savvy, I wanna keep everything to the bare minimum to avoid confusion. The only needed files are the main .exe and the .config file, pdb, manifest and the app.publish folder should not get included.

Hmm, I thought the app.publish folder was the real "production executable" folder. The parent folder was included as a debugging aid.

Also, this was meant more as "everything you might need from this build to use and debug", releases should still be done manually (since artifacts aren't saved forever). Maybe we can add two artifacts per config, one with just the .exe and .config, another with that and the rest?

The build warnings about "set-env" and "add-path" worry me a bit since Github explicitly states that they can get deprecated/removed any time and that you should avoid them.

Sadly, I can't resolve them, they come from NuGet/setup-nuget@v1.0.2 and depend on them to fix it. (Someone already opened an issue: NuGet/setup-nuget#11)

NiyaShy commented 3 years ago

From all the (local) builds I did in the last years I could never find a real difference between the "main" and app.publish exe files, so I ignored the latter and used the former. Also just checked the CRC-32 values for both files from the last auto-build in your fork, and they are identical. So it shouldn't make any difference. Good to know that the build artifacts are just temporary. Haven't worked with Actions yet, so I kinda thought that they create releases 😅 But now that you mentioned it: I try to keep any code changes (aside from new translations) away from the master branch. PRs with new features happen very rarely, so it wouldn't trigger that often. And (so far) I did code testing myself.

And thanks for the source of those warnings. Already thought it was kind of weird that the mentioned commands aren't even used in your file... Hope they get fixed soon-ish.