Improve GitHub Actions - Githubissues

Seercat3160 commented 4 months ago

Lints and checks formatting - CI will fail on pull requests if this fails
- Any clippy warnings fail the build - ensures code quality!
Tags starting with v (e.g. v1.0.0) will create a draft release if the build succeeds, with the artifacts uploaded to the release automatically
- These will need to be manually published, to ensure that everything works properly each time

Seercat3160 commented 4 months ago

As you can see, the existing format failures caused the PR checks to fail.

Seercat3160 commented 4 months ago

Something of note: the binaries built by the CI don't have any webui files bundled, so can't be used in the same way as the manually-built binaries attached to previous releases. Also, despite there being a codepath (here) for having no bundled files, it's not possible to reach because of how the web state is instantiated (here).

In my opinion, the choice of whether to bundle files at build time should handled by conditional compilation, defaulting to false. This would fit with the idea of using some kind of launcher to start the client going forward, which could use something like #138 to provide the webui files.

At the current position in the project, I don't think I should just go and decide something like that, so I'm leaving this as a question for the dev team to answer.

Seercat3160 commented 4 months ago

All the docs I can find read to me like public repositories have no limit on Actions minutes or storage, only private repos count towards any usage limitations. Without that limitation I think uploading artifacts for PRs makes sense. I don't see any reason not to, and it could come in handy. What do you think?

Should I do anything regarding not bundling UI? I could probably call the include_ui script in the runner but it would be a bit of a bad solution in my opinion (especially as it decouples the CI success from just this repo's code - it would break if the particularities of that changed and fixing would require open PRs to rebase onto the changes). If you instead think adding conditional compilation would be good, so it hits that "no bundled files" codepath unless specifically told to bundle at build time, it's probably better to just merge this and I'll add that change into my other PR (after pulling these changes into there) so there's fewer merge conflicts to fix.

Regarding clippy: I'll add those as #![warn(clippy::whatever)] directives in main.rs as that's the more idiomatic way to set lints for the whole project. That way anyone doing cargo clippy gets them, so no surprises when it hits CI, and lints can be changed without messing with Actions. I'm pretty sure the command line lints are just if you want to do something as a one-off check.

I see that you used -D, meaning deny, there but I'm using warn: this just means it only shows up as warnings rather than errors, which makes more sense for lints in my opinion even when it will still fail CI. The RUSTFLAGS: -Dwarnings env var means that all warnings are treated as errors when running the CI (that is, they fail it) so it's really only a visual difference for developer experience. Let me know if I should change it to deny.

Bash-09 commented 4 months ago

All the docs I can find read to me like public repositories have no limit on Actions minutes or storage, only private repos count towards any usage limitations. Without that limitation I think uploading artifacts for PRs makes sense. I don't see any reason not to, and it could come in handy. What do you think?

I seem to remember there being limits on repositories of free organizations, but I can't be bothered to find it again. Might have been on some CI pricing page. Regardless, it probably won't be an issue for us anyways so I'm not gonna make a fuss about it.

Should I do anything regarding not bundling UI?

I wouldn't bother for now. It wasn't there previously so there is no loss. Plus with the dynamic web serving, a user can always grab the UI files separately and still use them without having to compile anything (assuming the UI build is available)

Regarding clippy: I'll add those as #![warn(clippy::whatever)] directives in main.rs as that's the more idiomatic way to set lints for the whole project.

Yep sounds reasonable.

I see that you used -D, meaning deny, there but I'm using warn: this just means it only shows up as warnings rather than errors, which makes more sense for lints in my opinion even when it will still fail CI.

Yeah that was just what I happened to use, doesn't matter to me.

MegaAntiCheat / client-backend

Improve GitHub Actions #139