Skip to content

WIP: use the new ABI version generation#3493

Draft
illwieckz wants to merge 2 commits intofor-0.56.0/syncfrom
illwieckz/mismatch/sync
Draft

WIP: use the new ABI version generation#3493
illwieckz wants to merge 2 commits intofor-0.56.0/syncfrom
illwieckz/mismatch/sync

Conversation

@illwieckz
Copy link
Copy Markdown
Member

@illwieckz illwieckz commented Mar 17, 2026

Fix ABI mismatch check.

Engine companion:

@illwieckz illwieckz force-pushed the illwieckz/mismatch/sync branch 2 times, most recently from 288c33a to 020e13f Compare March 17, 2026 01:45
static void CG_Rocket_ExecServerList( const char *table )
{
int netSrc = CG_StringToNetSource( table );
if ( Q_stricmp( rocketInfo.data.servers[netSrc]->abiVersion.c_str(), IPC::SYSCALL_ABI_VERSION ) ) {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This couldn't work because it was using rocketInfo.data.servers[netSrc][0].abiVersion instead of rocketInfo.data.servers[netSrc][rocketInfo.data.serverIndex[netSrc]].abiVersion

Copy link
Copy Markdown
Member Author

@illwieckz illwieckz Mar 25, 2026

Choose a reason for hiding this comment

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

@slipher This part of the patch was forgotten, now pushed as:

I'm building a 0.56.1. 🫤️

Q_strncpyz( mapname, Info_ValueForKey( info.c_str(), "mapname" ), sizeof( mapname ) );

const std::string version = Info_ValueForKey( info.c_str(), "version" );
const std::string abiVersion = Info_ValueForKey( info.c_str(), "abiVersion" );
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This couldn't work because it is named abi on engine side.

maxClients = atoi( Info_ValueForKey( info.c_str(), "sv_maxclients" ) );
Q_strncpyz( mapname, Info_ValueForKey( info.c_str(), "mapname" ), sizeof( mapname ) );

const std::string version = Info_ValueForKey( info.c_str(), "version" );
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This couldn't work because it was named daemonver on engine side. It is removed for now anyway.

@slipher
Copy link
Copy Markdown
Contributor

slipher commented Mar 17, 2026

Thanks for investigating the root cause of this. I don't get why we should get rid of the daemonver info though? So I propose the alternative of just fixing the info key names: #3495. With the addition of DaemonEngine/Daemon#1934 it would additionally work for release candidates.

@illwieckz
Copy link
Copy Markdown
Member Author

I don't get why we should get rid of the daemonver info though?

Me removing the engine version string isn't me saying we should not tell the engine version, it's me saying it looks to be too early to deal with engine or game versions yet.

Since we don't need engine or game version to check for ABI, it's better to only implement ABI for now.

We may have deeper redesigns of things to do first, especially to query the status string.

Then once our server queries will be fully functional we can start thinking about how we want to properly send versions. For example once we get proper status parsing in client, then we wouldn't need such custom code for the info string but simply use cvars, etc.

Right now we act like everything is a nail because all we have is a hammer. Since we only need the ABI version for now, we better only take care about ABI.

Quote from DaemonEngine/Daemon#1933 (comment):

Displaying engine and/or game version would require more work because we better store the game and engine version in the status string instead of the info string.

The info string sould be kept as short and unmodified as possible for various reasons:

  • The info string is the public API for third party server query tools and server browsers (like qstat, XQF and many others).
  • When querying thousands of server, we better keep small the minimal information to be transferred over the network to produce a list, then possibly complete the information with the status string.

For example in our server browser, we may only display what is currently displayed with this branch, but when clicking a server, display map levelshot, player names, engine version, mod name, mode version, etc. Many stuff meant to be in status string.

The status string is basically the custom game-specific protocol, while the info string is a somewhat standard protocol like HTTP: one doesn't modify HTTP to tweak his web page.

Some minor changes being done to the info string can be accepted if it is really worth it, like the way we disclose the amount of bots (non-standard), and the api (non-standard), because it makes easy and fast to build a server list without querying and parsing the status string that can be much much longer.

As an example, just run qstat -json -carets -P -R -unvanquisheds unvanquished.net and observer that neither bots neither abi is or would be printed, while version.daemon.abi is, without us tweaking qstat.

@illwieckz
Copy link
Copy Markdown
Member Author

Our game not having millions of players on thousands of server may let us forget why the protocols have been designed this way. I do remember when XQF/QStat was able to query steampowered.com with all of those thousands of CS:GO servers. 😄️

We better think twice before cramming something into the info string.

@slipher
Copy link
Copy Markdown
Contributor

slipher commented Mar 17, 2026

I think it's fine as long as the info response fits in a single packet. If you want to balance it out by removing something, we can drop the stats URL which obviously doesn't belong there.

@illwieckz
Copy link
Copy Markdown
Member Author

illwieckz commented Mar 17, 2026

If you want to balance it out by removing something, we can drop the stats URL which obviously doesn't belong there.

Oh lol yes, this one looks totally out of place, and we send it in status string anyway… We better strip that!

I think it's fine as long as the info response fits in a single packet.

Well, sure, but daemonver doesn't look to be a good name, I do believe we need to think twice when adding something to the response packets, and to think even twice more when adding things to the info packet itself, and I don't really want to spend time on the naming of it right now.

One thing is that from a server list point of view, the software version isn't as useful as the protocol version or the abi version.

I feel no urge to decide on what to do with engine and game version. We can totally postpone this topic for after the 0.56.0 release. Also, there is a larger work to be done regarding versions, especially the topic of splitting engine and game version, and the topic of the response packet can be considered at the same time.

The ABI version on the contrary has an immediate usefulness.

@slipher
Copy link
Copy Markdown
Contributor

slipher commented Mar 17, 2026

The version has an immediate usefulness to be displayed in the server list like on unvanquished.net/servers. For example if a new release has just been made and you want to play on an up-to-date server, you can check the version. This is already implemented in the in-game server list, so it is relevant for 0.56. We can always change the name of info keys for a new major release.

@illwieckz illwieckz marked this pull request as draft March 18, 2026 21:59
@illwieckz illwieckz changed the title fix ABI mismatch check WIP: use the new ABI version generation Mar 18, 2026
@illwieckz
Copy link
Copy Markdown
Member Author

The variable name fix itself isn't needed anymore as it has already been merged:

The rest of the work using the proposed ABI version generation may be rebased over that fix, or even dropped if we decide to drop the boolean the proposed ABI version generation is using.

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.

2 participants