Skip to content

WIP: proposal for ABI version generation #1933

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.

Sign up for GitHub

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

Draft
illwieckz wants to merge 1 commit into
base: for-0.56.0/sync
Choose a base branch
from
+11 −6
Draft

WIP: proposal for ABI version generation #1933

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/common/IPC/Primitives.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,4 +590,10 @@ SharedMemory SharedMemory::Create(size_t size)
}
#endif

std::string AbiVersion()
{
return std::string(IPC::SYSCALL_ABI_VERSION)
+ (IPC::DAEMON_HAS_COMPATIBILITY_BREAKING_SYSCALL_CHANGES ? "+compatbreak" : "");
Copy link
Contributor

@sweet235 sweet235 Mar 17, 2026

Choose a reason for hiding this comment

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

Why cannot we simply increase the ABI version when something breaks compatibility?

Copy link
Member Author

@illwieckz illwieckz Mar 17, 2026

Choose a reason for hiding this comment

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

This is exactly what we do.

Once 0.55.0 is released, the ABI version is increased to 0.55.

Once breaking the ABI we create a “future” branch named for-0.56.0 and we increase the ABI version to 0.55+compatbreak.

Once we are releasing 0.56.0, the ABI changes are then considered stabilized for the 0.56 cycle and we increase the ABI version to 0.56.

This change makes that ABI version bump more obvious and easier to deal with, by providing a function that returns the current ABI version, bumping it for us when needed.

Copy link
Member Author

@illwieckz illwieckz Mar 17, 2026

Choose a reason for hiding this comment

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

Instead of modifying by hand the 0.55 string to 0.55+compatbreak, we just turn one boolean to true to tell a breaking change is on the go. Unlike a string, the boolean is typed and the compiler will catch possible typos, and the base version string is left untouched, preventing typos as well.

Then this function generates the ABI version string for us.

When releasing the game, we run a script. That script bumps the ABI base version string from 0.55 to 0.56 for us, and reset the “work in progress” boolean to false for us, automatically.

Copy link
Contributor

@sweet235 sweet235 Mar 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

This is the opposite of "easy to deal with". No innocent looking function like this should ever return anything but the true ABI version. You should at least rename this function to something that induces the necessary caution in the reader, like HairyAbiVersion.

If you want to change the ABI version in a development branch, give it an actual version string.

Copy link
Contributor

@sweet235 sweet235 Mar 17, 2026

Choose a reason for hiding this comment

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

Unlike a string, the boolean is typed and the compiler will catch possible typos, and the base version string is left untouched, preventing typos as well.

Requiring a type system to type a version string is, of course, utter derangement. If someone is unable to write a version string, then they should not change it.

Copy link
Member Author

@illwieckz illwieckz Mar 17, 2026

Choose a reason for hiding this comment

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

This thread doesn't discuss what real men would do.

Right now the code used in the release candidate would make the in-game server browser wrongly report all game servers as incompatible servers. It would be very bad to release a new version of the game with such a serious bug. This branch is about fixing that bug and we're discussing the implementation of that fix. We're not discussing the feature itself that is fixed.

If you still want one word from me about the feature itself that is fixed, the ABI version string generation is part of an effort to automatize even more the release process to reduce the manpower required to release the game. The version bump is meant to be scripted. From this point of view, the fact that someone is able or not to write a string is out of topic, as the purpose is to require less work from the man to begin with and delegate that work to the machine. That's all I have to say on the topic of ABI version string generation itself.

I know you are well aware of the small bus factor of the project and other manpower shortcoming we struggle with, so I will not spend more time on discussions that would only have effect to put more work on the shoulders of people. I do believe we will agree giving people more work to do is not the way to go if we want to reduce the amount of work required to release things.

Copy link
Contributor

@sweet235 sweet235 Mar 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

This thread doesn't discuss what real men would do.

Your sarcasm is inappropriate.

You are proposing confusing semantics for something that should be most simple: a version tag. The only goal I could identify so far is that you need this for some code-generating script you are using. That does not seem to justify the addition of technical debt to the code base.

This is even more so during the late phase in the release process. You agreed to a freeze.

I know you are well aware of the small bus factor of the project and other manpower shortcoming we struggle with, so I will not spend more time on discussions that would only have effect to put more work on the shoulders of people.

There are several 0.56 test servers online, I host one of them. Do you have any problems with my server?

Copy link
Member Author

@illwieckz illwieckz Mar 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not proposing that version string + boolean thing. This design predates my PR.
I'm not the one who designed that version string + boolean thing.

You're blaming me for something I haven't designed.
You're blaming me for something I'm dealing with.
You're blaming me for something I explain to you.

I'm fine with many designs and solutions. Here I propose one solution around the design we currently have. I explain to you the situation I'm dealing with. Stop blaming me for random things I'm not even in fault at.

There are several 0.56 test servers online, I host one of them. Do you have any problems with my server?

Why are you turning it personal? This has nothing to do with your server itself or any effort people would put into their server. I have nothing with your server and you know it.

Just do the test, run the RC2 build, and open the in-game server browser, you'll see that all entries are listed with an empty version string. An empty version string is not equal to the engine version string, so all servers are considered incompatible, and if we release the game with this bug all servers will be wrongly considered incompatible. This includes any server, including yours. If we don't fix the bug, the game will wrongly discourage people to join servers, including your server, wrongly believing the server is incompatible while it will not be.

unvanquished_2026-03-18_180958_000 unvanquished_2026-03-18_181102_000

If you don't want the 0.56.0 release to ship with a bug so serious it would discourage people to play the game and to even play your own server, stop hindering the resolution of this bug.

At this point the bug is so serious that any fix, be it ugly or suboptimal, has to be done. Even an ugly workaround hack would be better than shipping the bug.

At this point we should not mind if the solution is clean or not, if the design we're dealing with is clean or not. Priority is to fix the bug before the release. Discussing a better design can even be postponed to after the release if we can make the release unbuggy with whatever solution we have, and it's OK to do that.

This is a classic “important versus urgent” thing.

Doing a better design is important, fixing the bug is urgent. Classifying and planning things among important and urgent things is part of very first lessons in ITIL training.

Let's quote that first sentence from that Wikipedia page on ITIL:

ITIL (previously and also known as Information Technology Infrastructure Library) is a framework with a set of practices (previously processes) for IT activities such as IT service management (ITSM) and IT asset management (ITAM) that focus on aligning IT services with the needs of the business.

Here the needs of the business (that means our needs and your needs) is to get the release not discouraging players to connect servers including yours because of a bug wrongly reporting a server incompatibility.

Right now I'm working on aligning the IT service (the dæmon engine and the game code) with the needs of the business (you getting players joining your servers). Good practices in such situation is to classify urgent and important thing. Fixing the bug is urgent, redesigning the infrastructure is important. I'm fixing the bug first to satisfy the customers (which here are both players and server owners like you), and I'm OK to postpone any discussion on the redesign to be done after the customer is satisfied, so we can prepare more future satisfaction in a better way.

This is project management 101.

I assume you have the expectation that the engine and gamecode doesn't wrongly discourage players to join your server because of a bug, so I'm working on making sure the engine and gamecode meets that expectation.

This is me putting your satisfaction first. Please stop hindering me putting your satisfaction first, please stop hindering me making sure I deliver you what your satisfaction requires.

Copy link
Member Author

@illwieckz illwieckz Mar 18, 2026

Choose a reason for hiding this comment

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

When I say “This thread doesn't discuss what real men would do“, I emphasize on the fact this thread is not about discussing a better design, but about fixing a bug before a release to meet a dead line.

Discussing what real men would do can be fine, but it's an important thing.

Fixing the bug and delivering is the urgent thing, this thread is about fixing the bug and delivering, not about designing.

I'm not saying we cannot discuss what real men would do, I'm just saying this is not the topic of this thread and that such topic, if it should happen, may be postponed to meet the dead line.

}

} // namespace IPC
2 changes: 2 additions & 0 deletions src/common/IPC/Primitives.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ namespace IPC {
size_t size;
};

std::string AbiVersion();

} // namespace IPC

namespace Util {
Expand Down
3 changes: 1 addition & 2 deletions src/engine/framework/VirtualMachine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

static Cvar::Cvar<std::string> abiVersionCvar(
"version.daemon.abi", "Virtual machine IPC ABI version", Cvar::SERVERINFO | Cvar::ROM,
std::string(IPC::SYSCALL_ABI_VERSION) +
(IPC::DAEMON_HAS_COMPATIBILITY_BREAKING_SYSCALL_CHANGES ? "+compatbreak" : ""));
Copy link
Member Author

@illwieckz illwieckz Mar 17, 2026

Choose a reason for hiding this comment

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

It was the only place where that string was constructed, but it was needed in more places. It was actually the only place where using that custom string did nothing.

IPC::AbiVersion());

static Cvar::Cvar<bool> workaround_naclArchitecture_arm64_disableQualification(
"workaround.linux.arm64.naclDisableQualification",
Expand Down
6 changes: 2 additions & 4 deletions src/engine/server/sv_main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,10 +596,8 @@ static void SVC_Info( const netadr_t& from, const Cmd::Args& args )
}

info_map["gamename"] = GAMENAME_STRING; // Arnout: to be able to filter out Quake servers
info_map["abi"] = IPC::SYSCALL_ABI_VERSION;
Copy link
Member Author

@illwieckz illwieckz Mar 17, 2026 •
edited
Loading

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 the game was comparing abi with IPC::SYSCALL_ABI_VERSION to check if that was different, but it was the same.

Copy link
Contributor

@VReaperV VReaperV Mar 17, 2026

Choose a reason for hiding this comment

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

What do you mean it was the same?

Copy link
Member Author

@illwieckz illwieckz Mar 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

On game side it was doing:

if ( Q_stricmp( rocketInfo.data.servers[netSrc]->abiVersion.c_str(), IPC::SYSCALL_ABI_VERSION ) )

Though there was two mistakes making abiVersion an empty string there, but once those two mistakes were fixed (info string being named abi and not abiVersion, and the right pointer being used), the game code intended to compare info_map["abi"] with IPC::SYSCALL_ABI_VERSION.

The comparison would always have been true when comparing a game built on 0.55 and a game built on 0.55 + incompatible things.

Copy link
Contributor

@VReaperV VReaperV Mar 17, 2026

Choose a reason for hiding this comment

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

The comparison would always have been true when comparing a game built on 0.55 and a game built on 0.55 + incompatible things.

Wat

I literally have screenshots in the original pr that added the check that disprove this.

Copy link
Member Author

@illwieckz illwieckz Mar 17, 2026

Choose a reason for hiding this comment

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

Here I'm talking about the current code from for-0.56.0/sync here. The one we can test with the RC2 build and test server.

I'm not saying it may have not worked in the past, but the code in the RC doesn't work. I have not investigated when it broke and how. I wouldn't exclude some mistake may have slipped in, I don't know.

This branch focuses on fixing what is currently in for-0.45.0/sync.

Copy link
Contributor

@VReaperV VReaperV Mar 17, 2026

Choose a reason for hiding this comment

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

I wouldn't exclude some mistake may have slipped in, I don't know.

One might say sliphered in

Copy link
Member Author

@illwieckz illwieckz Mar 17, 2026

Choose a reason for hiding this comment

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

As I said I have not spent time to git blame the code, I focused on making it work, starting from what I had in hands.

Sometime I spend a lot of time investigating things and tracing code from history, like in this comment where I traced code 20 years back.

The release is now expected to happen very soon, so I'm ironing the last bits and so right now I focus more on that polishing effort itself than knowing why it is broken.

// Add the engine version. But is that really what we want? Probably the gamelogic version would
// be more interesting to players. Oh well, it's what's available for now.
info_map["daemonver"] = ENGINE_VERSION;
Copy link
Member Author

@illwieckz illwieckz Mar 17, 2026

Choose a reason for hiding this comment

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

This is removed for now.


info_map["abi"] = IPC::AbiVersion();

Net::OutOfBandPrint( netsrc_t::NS_SERVER, from, "infoResponse\n%s", InfoMapToString( info_map ) );
}
Expand Down
Loading