Enable servers and clients to declare compatibility with public servers. #830
Conversation
rfortier
force-pushed
the
feat/Enable-public-server-compatibility
branch
4 times, most recently
from
8669b3f to
b896c4a
Compare
|
Set back to draft while I fix a UI glitch. |
rfortier
force-pushed
the
feat/Enable-public-server-compatibility
branch
from
b896c4a to
86f5ffc
Compare
| -- Comment out next line if incompatible with current release, or, fix compatibility version. | ||
| contents = contents .. "#if !IS_MASTER\n" .. " #define COMPATIBLE_WITH_BUILD_COMMIT \"v1.8.0\"\n" .. " #endif" |
There was a problem hiding this comment.
I'm still not sure about this:
- A developer has to be aware of this when releasing a new version of the mod. And has to find and bump the version contained within this string.
- "Comment out... if..." I don't like this. Maybe consider integrating this right into the build pipeline. Like via a build flag, or a custom build mode or something
From my experience, things like this are almost always end up abandoned, especially when new maintainers come
There was a problem hiding this comment.
You have made it plain you don’t like the PR, pointing out potential problems while ignoring the benefits to the community. You have not offered any constructive criticism that is also feasible to implement without magic.
I object to the fears you are voicing, as the disasters you predict require incompetence on the part of the author for the disaster to come about.
Present an improvement that is feasible. Otherwise, I stand by the design and the code.
There was a problem hiding this comment.
Not a big fan, just because the protocol might be compatible between versions does not mean that other behaviour(s) weren't changed between versions.
And since dev releases don't increment the overall version there is no point in providing workarounds 😅
There was a problem hiding this comment.
Your first point, "does not mean other behaviour(s) weren't changed" is the point I've been trying to make all along too.
The back-and-forth with Miredirex started over this point, that we should be able to compute if the bits in the protocol have changed. Might be technically possible but that is an inadequate solution because it may be that there is no change in the wire protocol at all, but just a semantic / interpretation / behavior change that renders a new version compatible or not. That requires human review to decide.
Even then it can be ambiguous, or at least a decision as to which way to go. The current Vampire Lord animation fix is a case in point that can be used for a discussion example.
At first, M. said it was incompatible because it was a server-side change, implying a wire-protocol change also. But reviewing the final version showed it was only a behavior change with no wire protocol change. If you had the server-side fix, the Vampire Lord transformation fix would be seen by clients, even older clients. If you used even a newer client with the older server, it would work just fine, you just wouldn't have the animation fix.
So, is it compatible with 1.8.0 servers or not?
I'll argue it is compatible, because that is better for the community. People on a fork/branch with the fix can interoperate with 1.8.0 clients and servers with no issues other than a cosmetic one. When a release comes out with the fix, it will still interoperate with the public servers like PlayTogether. Those providers can update at their leisure, or even skip an upgrade cycle if we think another maintenance release will be "soon."
Everyone wins, IMHO.
Now, it's not quite true with the current state of the PR. Taking into account past discussions with the team, there is a clear bias against dealing with version compatibility. So, the current PR ensures that an IS_MASTER build will never make a compatibility declaration.
But, we could choose to change that; the example I just cited above makes a pretty good case for doing so. Plus, the makeup of the team maintaining the mod and approving PRs has changed. The new guard may come to a different conclusion than the old guard; I'm providing that option.
One last point: I have to do this in the fork anyway. It is a better and more supportable solution than having the fork "lie" that it is v1.8.0 just so it will work with the public servers (which is what the fork used to do, and I just think that is dangerous).
This way it can have a distinct version number while still maintaining compatibility, which is far better for hunting down bugs.
If the PR is rejected, the code can just live on in the fork as a "best we can do" solution, as @miredirex suggested. But there are benefits to other branches or forks if it goes in. Like fixing the fact Dev branches can't have distinct version numbers for each new build without breaking compatibility.
Because then you can’t make weekly dev builds compatible with current release either. It is a tool for building any other branch, not just forks. |
|
Not a big fan, just because the protocol might be compatible between versions does not mean that other behaviour(s) weren't changed between versions. And since dev changes don't bump the tag, there is also no point in providing workarounds 😬 |
That is, enable servers and clients to declare compatibility with current-release public servers. Declare which public server version your new build can support in top-level xmake.lua. MASTER builds will never declare support for a different version. You can also comment out the COMPATIBLE_WITH_BUILD_COMMIT line in xmake.lua to turn it off in other branches. Needed a few changes to "lie" to the public server list and send the protocol-compatible build commit instead of the actual build commit. Removed pnpm-lock.yaml; review comment.
rfortier
force-pushed
the
feat/Enable-public-server-compatibility
branch
from
86f5ffc to
fd69731
Compare
3 participants
Declare which public server version your new build can support in top-level xmake.lua. MASTER builds will never declare support for a different version.
You can also comment out the COMPATIBLE_WITH_BUILD_COMMIT line in xmake.lua to turn it off in other branches.