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

OpenTTD 1.10 breaks compatibility of Admin port #8060

Closed
frosch123 opened this issue Apr 5, 2020 · 3 comments
Closed

OpenTTD 1.10 breaks compatibility of Admin port #8060

frosch123 opened this issue Apr 5, 2020 · 3 comments

Comments

@frosch123
Copy link
Member

@frosch123 frosch123 commented Apr 5, 2020

Commit 5880f14 breaks compatibility of the Admin Port API by inserting an item in the middle of an enum.

Since this breaks all external libraries which connect to OpenTTD, I suggest to break the API again in 1.10.1 and to revert to the old enum values (by moving the new enum value to the end).

Reasoning:

  • There are plenty of libraries written in various languages, which connect to the Admin Port. Most of them are unmaintained.
  • Noone noticed the breakage during the RC period, so likely most won't notice it in 1.10.1 either. (if we give a poke to those who made a custom work around)
  • In the long term, the API stays mostly compatible, so there will be no issues after 1.10.1.
  • I do not know if API users rely on receiving NETWORK_ACTION_LEAVE for every player leaving the game. So another mitigation may be to send both NETWORK_ACTION_KICKED and NETWORK_ACTION_LEAVE (in this order).

I am not sure what API client @bjarnithor99 used when testing. Maybe you have some input as well.

@glx22
Copy link
Contributor

@glx22 glx22 commented Apr 5, 2020

I agree, the best choice is to move the new enum value to the end. And I think @duckfullstop may have some inputs too.

frosch123 added a commit to frosch123/OpenTTD that referenced this issue Apr 5, 2020
@frosch123
Copy link
Member Author

@frosch123 frosch123 commented Apr 5, 2020

Possible fix in #8061.

My comment from above about sending both ACTION_LEAVE and ACTION_KICKED is void. That change only affects the regular client-server API (which is version specific anyway), it does not affect the admin network api.

@duckfullstop
Copy link
Contributor

@duckfullstop duckfullstop commented Apr 5, 2020

This one has affected us at Reddit OpenTTD: I’ve patched our ancient version of libottdadmin2 to shift everything to the right places, but I’m not sure changing protocol again in the next version is a sane choice: that is unless it’s a hotfix that can be deployed quickly.

That being said, moving things back means that everyone’s libraries don’t have to be updated. Yin/yang, I guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants