Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GitHub Actions CI support #45

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

smiley
Copy link

@smiley smiley commented Oct 18, 2020

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

@NiyaShy
Copy link
Owner

NiyaShy commented Oct 18, 2020

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

  • 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.

  • 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.

@smiley
Copy link
Author

smiley commented Oct 18, 2020

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
Copy link
Owner

NiyaShy commented Oct 18, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants