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

Overhaul Accelerator repository #13

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

Kenzzer
Copy link
Collaborator

@Kenzzer Kenzzer commented Jan 28, 2024

Added protobuf and breakpad as submodule

It's no secret that google's tools to fetch their repositories are absolutely horrendous to use, not counting that they're also outdated in regard to python's version. Fortunately for us, google had the decency to make public mirrors of their repository over to github, let's take advantage of that and link to these repositories.

Edit : It seems cloning protobuf is completly unnecessary to compile breakpad

Updated breakpad.sh to compile breakpad for both 32bits and 64bits

Just like the title implies, I've updated the script to compile breakpad for each architecture. I've also revised some of the patches, so they work with the breakpad submodule I decided to set the hash to. One of these patches also became unnecessary, as its change became official.

Edit : This is now part of the ambuild scripts

Overhauled AMBuild scripts

Those were some very old, and confusing to look at build scripts. I brought them up to modern ambuild syntax.

Updated the extension code

I modified a few lines of the extension, so that it may compile with the newest breakpad code. As well as the definition of GetSpew function, which was invalid for 64 bits.

Update Gamedata

TF2 x64 bits is out, gamedata needs a little update

Added a CI

CI(s) are cool ! It compiles the extension for latest ubuntu and windows, and also on an older version of both OSes.

@adriansmares
Copy link

adriansmares commented Apr 22, 2024

I've tried to use the Linux artifacts on L4D2 under Debian bookworm (same base as Ubuntu 22.04). I managed to make it work, but I have some comments:

  1. zlib is now a dependency - this is different from the latest HEAD, and probably should be documented somewhere. I only had to install zlib1g:i386 and this stopped being an issue.
  2. The libstdc++ which is bundled with L4D2 seems to be quite old and results in this issue:
[SM] Unable to load extension "accelerator.ext": bin/libstdc++.so.6: version `GLIBCXX_3.4.21' not found (required by /home/steam/srcds/left4dead2/addons/sourcemod/extensions/accelerator.ext.so)

Since the libraries in the server bin folder have priority, whatever I installed on the host did not really matter. I fixed this by statically linking libgcc and libstdc++. This is a blatant copy paste from SourceMod, so maybe there are side effects or it could simply be shorter (specifically I don't think -lm is needed).

@Kenzzer
Copy link
Collaborator Author

Kenzzer commented Apr 25, 2024

  1. zlib is now a dependency - this is different from the latest HEAD, and probably should be documented somewhere. I only had to install zlib1g:i386 and this stopped being an issue.

ZLib will allow breakpad greater symbol coverage, which in turn should benefit the generated minidumps. Extensions dependencies are usually not documented (wrongly so). I will let @asherkin decide whether or not it should be listed somewhere, or if we should swap to static linkage or take it out entirely.

I fixed this by statically linking libgcc and libstdc++.

Thank you valve. I will address this at the same time as the zlib issue in order to avoid two more commits. As the excessive amount was the reason this branch was asked to be rebased and squashed by Asherkin.

@adriansmares
Copy link

On TF2 x64 the SourcePawn JIT binary is unavailable (there is no x64 JIT):

[CRASH] WARNING: Failed to load SourcePawn library /home/steam/srcds/tf/addons/sourcemod/bin/x64/sourcepawn.jit.x86.so: /home/steam/srcds/tf/addons/sourcemod/bin/x64/sourcepawn.jit.x86.so: cannot open shared object file: No such file or directory

It seems that the VM binary however exposes the same interface, and accelerator seems to load well if that binary is used.

@asherkin
Copy link
Owner

ZLib will allow breakpad greater symbol coverage, which in turn should benefit the generated minidumps.

This is for compressed DWARF symbols, right?

Statically linking all 3 feels correct.

@sapphonie
Copy link
Collaborator

@Kenzzer lmk when i should review this 👀

Kenzzer and others added 9 commits October 4, 2024 12:51
add protobuf

change repo url

try to restore github python syntax highlighter

move breakpad into third_party, update packagescript
remove unnecessary files

Move the git patching into ambuild

move lss to a patch

Add windows compilation support

remove breakpad.bat

move postlink libs
* make cwd_cmd spew stdout and stderr

* add proper docker build support

* Overhaul ci (#6)

* Setup CI

* fix checkout

* fix yaml syntax

* no fail fast

* setup CI cache

* Fix pip install

* remove pip git

* update actions, ditch node 16

* small syntax cleanups

* more CI changes

* github doc lied

---------

Co-authored-by: Kenzzer <[email protected]>

* final push for perfect dockerbuilds in every scenario that i have been able to find

* rename cicd->dockerbuild

---------

Co-authored-by: Kenzzer <[email protected]>
update names of dockerbuild folder in sh files
@Kenzzer
Copy link
Collaborator Author

Kenzzer commented Oct 4, 2024

Sorry for the long delay @adriansmares other x64 fixes took priority (namely sourcehook & dhooks). This should be enough for accelerator to work again on l4d2.

@asherkin Zlib had to be added as a submodule because statically linking against the one provided by linux apt packages will result in compilation error.
/usr/bin/ld: /lib/x86_64-linux-gnu/libz.a(zutil.o): relocation R_X86_64_PC32 against symbol 'z_errmsg' can not be used when making a shared object; recompile with -fPIC

As such we have to recompile zlib ourselves with -fPIC

@sapphonie sapphonie assigned sapphonie and unassigned sapphonie Oct 5, 2024
Copy link
Owner

@asherkin asherkin left a comment

Choose a reason for hiding this comment

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

Really nice work on this - left a smattering of comments.

Copy link
Owner

Choose a reason for hiding this comment

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

This docker stuff isn't used by the CI from what I can see, right? It's just a helper for manual builds?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made by sappho, and she really wanted me to keep this in since this could help people building the extension manually. Though I'll leave further comments to her.

# we do this so that we can be agnostic about where we're invoked from
# meaning you can exec this script anywhere and it should work the same
thisiswhereiam=${BASH_SOURCE[0]}
# this should be /whatever/directory/structure/Open-Fortress-Source
Copy link
Owner

Choose a reason for hiding this comment

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

It should be?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ditto. @sapphonie

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, copied this build script from myself somewhere else 👀 , the directory structure shouldn't matter

You can yeet the entire dockerbuild/ folder if you want, it's not really needed with proper GH actions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Noted! I'll rebase this one last time.

extension/smsdk_config.h Outdated Show resolved Hide resolved
extension/extension.cpp Show resolved Hide resolved
third_party/Patch Show resolved Hide resolved
@sapphonie
Copy link
Collaborator

Unknown if this fixes #20 because actions aren't set up, can those be enabled on PR please 👍

@sapphonie
Copy link
Collaborator

sapphonie commented Oct 11, 2024

#20 fixed by #22, actions are now being built on overhaul branch which i merged to this repo ( #21 )

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.

4 participants