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

Merged
merged 8 commits into from
Oct 25, 2024

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/

@caxanga334 caxanga334 marked this pull request as draft July 9, 2024 17:15
nosoop added a commit to nosoop/SMExt-TF2Items that referenced this pull request Jul 11, 2024
- Pull alliedmodders/sourcemod#2107 wholesale to modernize the
build script (multiplat, hl2sdk-manifests, safetyhook) and make
some minor adjustments to support CDetour.
- Stub out plugin compilation for now; it will be added in a
separate commit to preserve readability.
- Hoist reading 'tf2.items.nosoop' and add it as a fallback for
GiveNamedItem.  SM's gamedata updater will stomp all over any
changes we make in asherkin's.
- Fix sizing for reverse-engineered TF2 classes to match the 64-bit
release.
- Rename smsdk_ext.hpp since CDetour is picky about the filename.
nosoop added a commit to nosoop/SMExt-TF2Items that referenced this pull request Jul 11, 2024
- Pull alliedmodders/sourcemod#2107 wholesale to modernize the
build script (multiplat, hl2sdk-manifests, safetyhook) and make
some minor adjustments to support CDetour.
- Stub out plugin compilation for now; it will be added in a
separate commit to preserve readability.
- Hoist reading 'tf2.items.nosoop' and add it as a fallback for
GiveNamedItem.  SM's gamedata updater will stomp all over any
changes we make in asherkin's.
- Fix sizing for reverse-engineered TF2 classes to match the 64-bit
release.
- Rename smsdk_ext.hpp since CDetour is picky about the filename.
Copying the HL2SDK manifest is no longer needed.
Requires latest AMBuild.
@caxanga334 caxanga334 marked this pull request as ready for review July 31, 2024 18:02
Check if the manifest already exists in the project folder. If not, then search in the path specified by --hl2sdk-manifest-path.
public/sample_ext_nosdk/AMBuildScript Outdated Show resolved Hide resolved
public/sample_ext_nosdk/AMBuildScript Outdated Show resolved Hide resolved
public/sample_ext_nosdk/AMBuildScript Outdated Show resolved Hide resolved
public/sample_ext_nosdk/AMBuildScript Outdated Show resolved Hide resolved
public/sample_ext_nosdk/AMBuildScript Outdated Show resolved Hide resolved
public/sample_ext_nosdk/AMBuildScript Outdated Show resolved Hide resolved
public/sample_ext/AMBuildScript Outdated Show resolved Hide resolved
public/sample_ext/AMBuildScript Outdated Show resolved Hide resolved
@caxanga334
Copy link
Contributor Author

Hi, @Kenzzer

I've made the changes you requested. Please review and see if I didn't forgot anything.

Both scripts compiled successfully on Debian with Clang 11.

Copy link
Member

@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.

Thanks for the changes! I spotted one more issue, my apologies for not catching it in the earlier review. Otherwise I believe this is good to merge.

public/sample_ext/AMBuildScript Show resolved Hide resolved
@Kenzzer
Copy link
Member

Kenzzer commented Oct 25, 2024

I'm ready to take this in. However I'd rather not shake up the build environments for current 3rd-party extensions, and introduce two competing standards, so I'm adding extra env var checks. Anyways, thanks for your work!

@Kenzzer Kenzzer merged commit 600f493 into alliedmodders:master Oct 25, 2024
4 checks passed
F1F88 added a commit to F1F88/sm-ext-log4sp that referenced this pull request Nov 10, 2024
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