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

Modernize Sample Extensions's AMBuildScript #2107

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

Conversation

caxanga334
Copy link
Contributor

Summary

This PR aims to modernize the AMBuildScripts of the sample extensions. Bringing them closer to SM own AMBuildScript and adding support for compiling 64 bits extensions.

Sample

The build script is based on SM own build script. It was updated to use the HL2SDK manifests and allow compiling for multiple archs. Minimum AMBuild version increased to 2.2.

Two new arguments added to configure.py.

  • --hl2sdk-manifest-path : Path to the HL2SDK manifests, this is optional if the manifests already exists in the project root. Otherwise a copy of the manifests is created on the project root folder.
  • --targets : Overrides target build arch.

Sample (NO SDK)

The build script was updated to allow compiling for multiple archs. Minimum AMBuild version increased to 2.2.

One new argument added to configure.py.

  • --targets : Overrides target build arch.

if not cxx.target.arch in sdk['platforms'][cxx.target.platform]:
continue

binary = Extension.HL2ExtConfig(project, builder, cxx, extName + sdk['extension'], sdk)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a dot at extName + sdk['extension']? I tried to compile it, got tank.ext2.tf2.so instead of tank.ext.2.tf2.so

@caxanga334
Copy link
Contributor Author

Hi @FortyTwoFortyTwo . Yeah I indeed missed a dot. Updated the scripts. Could you please test again?

@FortyTwoFortyTwo
Copy link
Contributor

Hi @FortyTwoFortyTwo . Yeah I indeed missed a dot. Updated the scripts. Could you please test again?

All works fine for Linux, though trying in windows gives me LINK : warning LNK4044: unrecognized option, ignoring four TF2 windows libs. Not sure if it's my fault or it's the build script.

@caxanga334
Copy link
Contributor Author

caxanga334 commented Apr 13, 2024

All works fine for Linux, though trying in windows gives me LINK : warning LNK4044: unrecognized option, ignoring four TF2 windows libs. Not sure if it's my fault or it's the build script.

I was unable to reproduce this when compiling the sample_ext. Tried both x86 and x86_64 MSVC (Visual Studio 2022).

@bottiger1
Copy link
Contributor

bottiger1 commented May 13, 2024

Thanks for the update. However I noticed that when you use -static-libstdc++ it crashes when you pipe a number bigger than a char into iostream like this.

std::cout << (uintptr64_t)123;

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x00007f1db217b09b in char32_t std::(anonymous namespace)::read_utf8_code_point<char>(std::(anonymous namespace)::range<char const, true>&, unsigned long)
#1  0x00007f1db217cf3f in std::codecvt_base::result std::(anonymous namespace)::utf16_in<char, char16_t>(std::(anonymous namespace)::range<char const, true>&, std::(anonymous namespace)::range<char16_t, true>&, unsigned long, std::codecvt_mode, std::(anonymous namespace)::surrogates) ()
#2  0x00007f1db217d0a4 in std::codecvt<char16_t, char, __mbstate_t>::do_in(__mbstate_t&, char const*, char const*, char const*&, char16_t*, char16_t*, char16_t*&) const ()
#3  0x00007f1db21df14a in std::ostream& std::ostream::_M_insert<unsigned long>(unsigned long) ()
#4  0x00007f1db2107c1d in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > PLH::int_to_hex<unsigned long>(unsigned long) ()
#5  0x00007f1db20fff0e in PLH::x64Detour::hook()

I am not sure we can do anything about this or whether it is preferential to dynamically link to stdc++ instead. SRCDS appears to dynamically link.

However if we don't statically link to stdc++ it would mean extensions wouldn't be able to load on an OS running older versions of stdc++ which seems to be quite often. stdc++ versions often change every OS release.

There should probably be a very big notice alerting the user about these problems either way.

@Kenzzer
Copy link
Member

Kenzzer commented May 13, 2024

I am not sure we can do anything about this or whether it is preferential to dynamically link to stdc++ instead. SRCDS appears to dynamically link.

The issue is loosely connected to alliedmodders/metamod-source#167 the fix from that part needs to be backported to metamod 1.12

And applied there as well (that's how extensions are loaded)
https://github.com/alliedmodders/amtl/blob/e38cfe3cbf2047916e4a68017840bd325a8f7080/amtl/os/am-shared-library.h#L64

@bottiger1
Copy link
Contributor

I am not sure we can do anything about this or whether it is preferential to dynamically link to stdc++ instead. SRCDS appears to dynamically link.

The issue is loosely connected to alliedmodders/metamod-source#167 the fix from that part needs to be backported to metamod 1.12

And applied there as well (that's how extensions are loaded) https://github.com/alliedmodders/amtl/blob/e38cfe3cbf2047916e4a68017840bd325a8f7080/amtl/os/am-shared-library.h#L64

I'm assuming you mean sourcemod? It is already in metamod.

I just added RTLD_DEEPBIND, recompiled sourcemod, and it still crashes.

@Kenzzer
Copy link
Member

Kenzzer commented May 13, 2024

I'm assuming you mean sourcemod? It is already in metamod.

I did mean metamod. Currently the fix was applied to metamod 2.0, not 1.12. I was pointing this out in the event an extension is compiled as a metamod plugin, as it will be loaded by metamod instead. Actually that part might be inaccurate, nevertheless the fix should be backported to 1.12 from 2.0, otherwise sourcemod (which is a metamod plugin) will be subject to the same issue, and potentially corrupt the symbols space.

I just added RTLD_DEEPBIND, recompiled sourcemod, and it still crashes.

Hmmm, I'll try investigating as well when I get the chance.

@bottiger1
Copy link
Contributor

I just checked, I'm already using 2.0, and double checked that it has the deepbind change.

Metamod:Source Version Information
Metamod:Source version 2.0.0-dev+1289
Plugin interface version: 16:14
SourceHook version: 5:5
Loaded As: Valve Server Plugin
Compiled on: Apr 20 2024 14:02:31
Built from: alliedmodders/metamod-source@b4aa055
Build ID: 1289:b4aa055
http://www.metamodsource.net/

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.

None yet

4 participants