Expose client version information in non-debug builds

[rollerozxa]

Closes #14151.

The client version information that has previously only been available in debug builds are now always available. Serialisation version and client status still remain behind the debug check.

To do

This PR is a Ready for Review.

How to test

e.g. worldedit //lua command in singleplayer:

//lua minetest.log(dump(core.get_player_information("singleplayer")))

[rollerozxa]

The PR has been changed to only expose the version string field now.

[appgurueu]

I'm fine with the PR in the current version (pun intended)

[sfan5]

I think what we should do to further discourage doing version comparisons is to actually provide documentation on which protocol version maps to which engine version (or perhaps just link to networkprotocol.h?).
As it is now the docs tell you to use the protocol version but there isn't a single word on how.

[sfan5]

IRC:

17:21 		<luatic> "I think what we should do to further discourage doing version comparisons is to actually provide documentation on which protocol version maps to which engine version"
17:24 		<luatic> I recall the original suggestion also being the addition of APIs. Could a simple table that maps engine version names to protocol versions suffice? So modders could do something like client_proto_ver >= core.protocol_version["5.9"].
17:55 	sfan5 	@luatic: sure

[appgurueu]

I took the liberty to implement this. Is it fine now?

One minor question perhaps: We want core.protocol_versions to not contain patch versions, even if they entailed a protocol version bump, correct?

[y5nw]

Makes sense.

[grorp]

One minor question perhaps: We want core.protocol_versions to not contain patch versions, even if they entailed a protocol version bump, correct?

Hmm, I feel like it's inconsistent to leave a protocol version out just because it corresponds to a patch version. The reason to bump the protocol version for a patch version would be to make it detectable on the server, so leaving it out of the mechanism for making detection easier doesn't make sense. In builtin, one of the two cases where the protocol version is used for client feature detection is actually the 45/5.9.1 case.

[sfan5]

We want core.protocol_versions to not contain patch versions, even if they entailed a protocol version bump, correct?

I think we want one entry in that table for every changed protocol version. No matter if it's minor or not.

Or to be exact we should define the table contents as follows:

{
    [version string] = protocol version at time of release
    -- every major and minor version has an entry
    -- patch versions only for the first release whose protocol version is not already present in the table
}

(in theory we could bump the protocol version twice during dev, this should only result in one entry)

Also: we could add a simple devtest unittest to check that the client protocol version is present in the table

FWIW we can merge this now and improve docs and tests later.

[appgurueu]

Hmm, I feel like it's inconsistent to leave a protocol version out just because it corresponds to a patch version.

I am slightly on the fence about this, but there is a reason.

The reason to bump the protocol version for a patch version would be to make it detectable on the server, so leaving it out of the mechanism for making detection easier doesn't make sense.

Detectable for whom? A patch version should not introduce any new features for API users; therefore, why would an API user be interested in detecting it (well, workarounds maybe I suppose, but that would also apply to patch versions which don't get their own protocol version)? The API feature set should not have changed.

In builtin, one of the two cases where the protocol version is used for client feature detection is actually the 45/5.9.1 case.

Yes, I remember, but (1) this is in builtin, not in mod code; (2) 5.9.1 did in fact add a very minor feature to enable us to fix the minimap bug as I recall. Maybe 5.9.1 should, in hindsight, never have been a "patch" release, but rather a "minor" release.

It's still the exception in that all other patch releases don't have their own protocol version, which raises a nasty question of: When does something get an entry? sfan has phrased this precisely, but I don't think it's very intuitive for an API user.

---

In fact, I'd probably even go one step further and consider: No more patch releases. Only minor releases. Problem with that is that package maintainers love patch releases and won't prioritize minor releases (even if they mostly fix bugs) the same way.

[sfan5]

In fact, I'd probably even go one step further and consider: No more patch releases. Only minor releases. Problem with that is that package maintainers love patch releases and won't prioritize minor releases (even if they mostly fix bugs) the same way.

If we keep on a quick release schedule, then in general sure. But we'll need to do patch releases if big security issues show up.

[appgurueu]

But we'll need to do patch releases if big security issues show up.

Fair, but will those need separate protocol versions?

[appgurueu]

Alright, I have now added all 5.x protocol versions to core.protocol_versions, including 5.9.1 and using sfan's definition of the table: 1fade59.

Should you change your mind, just revert that commit.

[sfan5]

Fair, but will those need separate protocol versions?

No.

[appgurueu]

updated test seems okay