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 CI for plugins #41

Merged
merged 4 commits into from
Jul 25, 2023
Merged

Add CI for plugins #41

merged 4 commits into from
Jul 25, 2023

Conversation

KitRifty
Copy link
Collaborator

This extends the CI to compile the test plugins. There was an issue with some includes not being packaged with the releases, so compilation of the test plugins should be able to catch this next time.

Changes:

  • Fixed nextbot/path includes not being packaged with releases
  • Test plugins are now packaged with releases (in disabled folder)
  • Fixed compilation for cbasenpc_example.sp
  • Fixed compilation warnings for baseanimatingoverlay.inc

Fix path includes not being packaged
Package test plugins with releases
Fix compilation for cbasenpc_example.sp
Fix compilation warnings for baseanimatingoverlay.inc
@Kenzzer Kenzzer self-requested a review July 24, 2023 21:45
Copy link
Collaborator

@Kenzzer Kenzzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firstly, thank you for the fix.

Secondly, been thinking back and forth about this PR. And while I like having the plugins as part of the build script, I don't know if we need to split the pipeline between SM 1.11 and SM 1.12. This was a question I asked myself during #38

Sourcemod is by nature strongly backwards compatible when it comes to the plugins API, therefore it makes little sense to have a build line on both SM 1.11 and SM 1.12. If plugins build line succeed on SM 1.11 then it will also succeed on SM 1.12, we don't need to test it.

Thirdly, adding plugins to the CI means we shall add a 3rd job to the required workflows. And currently the naming of the job will fail us if SM ever gets to version 1.13 or later. I ask that the plugins build workflow is simply named SourcePawn CI / SM-Stable providing a future proof workflow name for the required workflows.

@KitRifty
Copy link
Collaborator Author

Oh yeah, I wasn't entirely sure if compiling the plugins against SM's dev build was necessary as you mentioned backwards-compatibility and all. I can just have the CI compile against 1.11.

And currently the naming of the job will fail us if SM ever gets to version 1.13 or later.

If the job names were hardcoded, yes. But the job name is based on the SM version that the SP compiler action was configured to use which would have to be manually updated in the future anyways. Updating the SM version will automatically update the job name too.

@Kenzzer
Copy link
Collaborator

Kenzzer commented Jul 25, 2023

If the job names were hardcoded, yes. But the job name is based on the SM version that the SP compiler action was configured to use which would have to be manually updated in the future anyways.

Yes but that is exactly why I'm requesting to hardcode the job name. I want a non changing job name, so that the repository settings don't have to be changed.

Just like currently the repo requires ubuntu-latest and windows-latest to succeed in order to allow the PR. I need a job named SM Stable, and not SM 1.11.x etc, as those are dynamic names.

Copy link
Collaborator

@Kenzzer Kenzzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are almost there, if those if statements could be turned into functions of the class (much like versioning) this PR will be good to be merged.

AMBuildScript Outdated Show resolved Hide resolved
AMBuildScript Outdated Show resolved Hide resolved
AMBuildScript Outdated Show resolved Hide resolved
AMBuildScript Outdated Show resolved Hide resolved
AMBuildScript Outdated Show resolved Hide resolved
AMBuildScript Outdated Show resolved Hide resolved
@KitRifty
Copy link
Collaborator Author

I want a non changing job name, so that the repository settings don't have to be changed.

Okay that makes sense. I thought you wanted the name change just for readability purposes, but since it's actually for a repository setting I understand why a static name is needed. Thanks for the clarification!

In any case I've already changed the name, it should be SourcePawn CI / SM (Stable).

@Kenzzer Kenzzer merged commit aa60c6b into master Jul 25, 2023
7 checks passed
@Kenzzer Kenzzer deleted the sp-integration branch July 25, 2023 13:45
@Kenzzer Kenzzer mentioned this pull request Aug 23, 2023
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