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

Fix matchmaking_ds for gamedata #1504

Merged
merged 5 commits into from
Jun 24, 2021
Merged

Fix matchmaking_ds for gamedata #1504

merged 5 commits into from
Jun 24, 2021

Conversation

accelerator74
Copy link
Contributor

@accelerator74 accelerator74 commented Jun 19, 2021

In continuation of this problem - #1028. It still exists (on linux). My option is to make the library find correct

This has been done before and it worked :) I restored it.

@accelerator74
Copy link
Contributor Author

Example plugin: https://forums.alliedmods.net/showthread.php?p=2750363
At the moment it cannot find the matchmaking_ds library. With my fix, everything finds.

@accelerator74
Copy link
Contributor Author

accelerator74 commented Jun 19, 2021

By the way, I tried to do this:
ke::SharedLib::Open("matchmaking_ds" SOURCE_BIN_SUFFIX SOURCE_BIN_EXT, NULL, 0)
but for some reason the library on linux still does not find

P.S.: ke::SharedLib::Open("matchmaking_ds_srv.so", NULL, 0) also not working :)

@accelerator74
Copy link
Contributor Author

Windows build is also have this bug: https://forums.alliedmods.net/showpost.php?p=2750542&postcount=19

Copy link
Member

@KyleSanderson KyleSanderson left a comment

Choose a reason for hiding this comment

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

Adjust the branch for a fallback if the library is outstanding.

core/logic_bridge.cpp Show resolved Hide resolved
@accelerator74
Copy link
Contributor Author

accelerator74 commented Jun 22, 2021

Done. But I have never seen the name of the file libmatchmaking_ds in any game, as it actually forms this name from FORMAT_SOURCE_BIN_NAME. In addition, the path is not explicitly specified, that you need to look for the library in the folder of a specific mod (ex. csgo/bin). There is no problem with the server and engine, because their addresses are obtained in a different way.

There are very few plugins (even I would say there are practically no plugins) who would use this library and write to you if problems are found :)

@accelerator74
Copy link
Contributor Author

accelerator74 commented Jun 22, 2021

BMS: none
CSGO: csgo/bin/matchmaking_ds.dll, csgo/bin/x64/matchmaking_ds.dll, csgo/bin/matchmaking_ds.so, csgo/bin/linux64/matchmaking_ds.so, csgo/bin/osx64/matchmaking_ds​.dylib
DOD: none
CSS: none
DOI: doi/bin/matchmaking_ds.dll, doi/bin/x64/matchmaking_ds.dll, doi/bin/matchmaking_ds_srv.so, doi/bin/matchmaking_ds.dylib, doi/bin/osx64/matchmaking_ds.dylib
EP1: none
EP2: none
HL2DM: none
Insurgency: insurgency/bin/matchmaking_ds.dll, insurgency/bin/x64/matchmaking_ds.dll, insurgency/bin/matchmaking_ds_srv.so, insurgency/bin/matchmaking_ds.dylib, insurgency/bin/osx64/matchmaking_ds.dylib
L4D: left4dead/bin/matchmaking_ds.dll, left4dead/bin/matchmaking_ds.so, left4dead/bin/matchmaking_ds.dylib
L4D2: left4dead2/bin/matchmaking_ds.dll, left4dead2/bin/matchmaking_ds_srv.so, left4dead2/bin/matchmaking_ds.dylib
SWARM: swarm/bin/matchmaking_ds.dll
DARKM: none
BGT: none
EYE: none
PORTAL2: none
BLADE: none
CONTAGION: contagion/bin/matchmaking_ds.dll
ND: none
TF2: none
SDK2013: none

by SteamDB

@accelerator74
Copy link
Contributor Author

Maybe it is still worth using my first option, but with minor changes to work with x64 libraries?

@KyleSanderson
Copy link
Member

KyleSanderson commented Jun 22, 2021

#1504 (comment)

A gentleman and a scholar.

By the way, I tried to do this:
ke::SharedLib::Open("matchmaking_ds" SOURCE_BIN_SUFFIX SOURCE_BIN_EXT, NULL, 0)
but for some reason the library on linux still does not find
P.S.: ke::SharedLib::Open("matchmaking_ds_srv.so", NULL, 0) also not working :)

Isn't there something weird on linux where it's ../bin/matchmaking_ds_srv.so or bin/matchmaking_ds_srv.so or something? Feels odd to land this if it doesn't work for the majority of srcds consumers.

@psychonic
Copy link
Member

I think the confusion is that the only games that utilize this binary also don't use the lib prefix or srv suffix. If those games were to backport that library, they would surely use their own naming convention. While it's unlikely to happen, we should be using the same prefix/suffix handling for all libraries belonging to an engine.

For example, the CS:GO Linux client adds a _client suffix to all binaries instead of using an _srv suffix on the server binaries, and sure enough, it's called matchmaking_ds_client.so there.

@accelerator74
Copy link
Contributor Author

Isn't there something weird on linux where it's ../bin/matchmaking_ds_srv.so or bin/matchmaking_ds_srv.so or something? Feels odd to land this if it doesn't work for the majority of srcds consumers.

The problem is that these libraries are not in the root/bin folder, but in the root/mod/bin folder.

we should be using the same prefix/suffix handling for all libraries belonging to an engine.

Yes, but what's the point if the server doesn't find the library we need in this case? :)

@psychonic
Copy link
Member

I was commenting on the prefix and suffix of the filename itself. We should be looking for the binary in the same places that the engine does, including the standard library path and gameinfo's GameBin path.

@accelerator74
Copy link
Contributor Author

So should I change in the code somehow or is my commit ok?

@accelerator74
Copy link
Contributor Author

@psychonic, I looked at your change from #1028. The problem with the old code was that it used PLATFORM_FOLDER, which on a 32 bit system adds the win32/linux32/osx32 folder, which in fact is not there. Therefore, the library was not found.

@accelerator74
Copy link
Contributor Author

I made some more small changes to the code, maybe this will be better then?)

@accelerator74
Copy link
Contributor Author

accelerator74 commented Jun 23, 2021

I tried the cs go server for the test on linux:
L 06/23/2021 - 08:41:21: [SM] Unrecognized library "matchmaking_ds" (gameconf "/root/Steam/steamapps/common/Counter-Strike Global Offensive Beta - Dedicated Server/csgo/addons/sourcemod/gamedata/test.txt")

Those, in fact, the current code, which is in the sourcemod, can work correctly only on windows/osx, since SOURCE_BIN_PREFIX is none.

But even so, in the current situation, I think it would be correct to also use my current changes + ke::SharedLib::Open(MATCHMAKINGDS_LIBRARY, NULL, 0)).

@psychonic
Copy link
Member

I took a deeper dive into this because there is clearly some issue, but some of the information wasn't making sense.

Some clarifications:

  • This functionality does work on Windows as-is, at least to get the matchmakingFactory pointer.
  • The library suffix is not at all an issue (and we should indeed keep it, as it seems very likely that any game with a suffix would keep its usual suffix for this library).

There are two specific issues, both affecting Linux and maybe Mac(?).

The prefix

For a handful of games on Linux (and again, maybe Mac?), Valve started prefixing the filenames of tier0, vstdlib, and any new shared objects with "lib". In Source 1, they did not apply it to the remaining, pre-existing shared objects (unlike the _srv or _client suffix on some engine). (In Source 2, they do aggressively use the "lib" prefix on all).

The path

On Windows, LoadLibrary only needs the filename if the binary is already loaded in the process. On Linux, it follows ld's rules instead, regardless of whether the shared object is currently loaded. Since the GameBin path is not typically in LD_LIBRARY_PATH by default, the binary isn't found by dlopen, even with the correct filename.

I think that a better fix for the name issue would be to have a different helper function for forming the binary name, either multiple macros or a single runtime function. And for the path, looking up the GameBin folder from the gameinfo.txt rather than hardcoding "bin".

All that aside, I think the fix in the PR is Good Enough if it works. I don't have the energy to do the above.

@psychonic psychonic self-requested a review June 24, 2021 02:52
@psychonic
Copy link
Member

psychonic commented Jun 24, 2021

I couldn't leave well enough alone. An alternative approach to fix all the potential problem cases noted, making the engine's filesystem interface do the work for us with minimal code.

You could scrap the other changes and replace:

if (ke::RefPtr<ke::SharedLib> mmlib = ke::SharedLib::Open(FORMAT_SOURCE_BIN_NAME("matchmaking_ds"), NULL, 0)) {
	this->matchmakingDSFactory =
	  mmlib->get<decltype(sCoreProviderImpl.matchmakingDSFactory)>("CreateInterface");
}

with:

if (auto mmlib = ::filesystem->LoadModule("matchmaking_ds", "GAMEBIN")) {
	this->matchmakingDSFactory = (void*)Sys_GetFactory(mmlib);
}

@accelerator74
Copy link
Contributor Author

if (auto mmlib = ::filesystem->LoadModule("matchmaking_ds", "GAMEBIN")) {
	this->matchmakingDSFactory = (void*)Sys_GetFactory(mmlib);
}

Its pretty work on l4d2 linux, but don't forget about SOURCE_BIN_SUFFIX :)

@psychonic
Copy link
Member

Its pretty work on l4d2 linux, but don't forget about SOURCE_BIN_SUFFIX :)

That shouldn't be necessary with this approach.

@accelerator74
Copy link
Contributor Author

accelerator74 commented Jun 24, 2021

Library is finded, but not the one we need. matchmaking_ds.so instead matchmaking_ds_srv.so. Otherwise, it will turn out that all the work will be carried out with a different library from which the server works (patches and the like will not work, tested).

@psychonic psychonic merged commit 2778b13 into alliedmodders:master Jun 24, 2021
@accelerator74 accelerator74 deleted the matchmaking_ds branch June 24, 2021 14:25
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.

3 participants