SM 1.13 (API v9) support & housekeeping - #50
Conversation
|
Please approve tested and running normally |
Kenzzer
left a comment
Kenzzer
left a comment
There was a problem hiding this comment.
I think the CI change should have been its own PR, this was really difficult to review. Lots of comments made. Changes to the extension are fine, makes sense to comment out the part that simply won't compile against 1.13 so no problems there. And finally I just need clarifications on each patch change/removal, as the last time that happened it introduced subtle bugs. See #45
| cache-key: | ||
| type: string | ||
| required: true | ||
| cache-dir: |
There was a problem hiding this comment.
Why is the cache being removed ?
There was a problem hiding this comment.
In my opinion the cache didn't really add much. A shallow checkout is already quite fast and with build-containers, AMBuild is now preinstalled, so one less checkout on Linux.
One pinned revision couldn't have built 1.12 and 1.13 at once anyway. If desired, I can pin the SourceMod checkout to a fixed commit (that doesn't need the cache back), but I fear it will drift too far again like the 2023 one did. After all, isn't it (part of) the CI's purpose to report breakage due to dependency updates?
That said, this is probably the most opinionated change and I can restore the cache if needed.
There was a problem hiding this comment.
CI's purpose to report breakage due to dependency updates
That is true if we did build against dev SM, but since we were not. Cache here did make sense.
For the newly introduced 1.13 builds by the PR, you can omit the cache. But I would like to retain the cache for stable SM.
Mikusch
left a comment
Mikusch
left a comment
There was a problem hiding this comment.
Ah, my apologies if the CI changes made reviewing this a bit difficult. I appreciate you taking the time to do so. I have given some explanations with links to official sources.
| cache-key: | ||
| type: string | ||
| required: true | ||
| cache-dir: |
There was a problem hiding this comment.
In my opinion the cache didn't really add much. A shallow checkout is already quite fast and with build-containers, AMBuild is now preinstalled, so one less checkout on Linux.
One pinned revision couldn't have built 1.12 and 1.13 at once anyway. If desired, I can pin the SourceMod checkout to a fixed commit (that doesn't need the cache back), but I fear it will drift too far again like the 2023 one did. After all, isn't it (part of) the CI's purpose to report breakage due to dependency updates?
That said, this is probably the most opinionated change and I can restore the cache if needed.
4 participants
This PR makes the extension compatible with the SM 1.13 extension API v9 as well as general housekeeping.
The breakpad submodule has been updated to its latest commit and the C++ standard has been upgraded to C++20. Four patches that were no longer useful have been deleted and the remaining two have been rebased onto the new tree.
The CI now builds against latest SM 1.12 and SM 1.13, but limetech upload will only use the 1.12 one. Linux builds now use the debian11-clang22 image from build-containers (https://github.com/alliedmodders/build-containers). As a side effect the released Linux binary targets glibc 2.31 instead of 2.35, so it should work on older servers.
This also fixes #47.