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

Add empty handler for new ruleset clause packet #180

Merged
merged 1 commit into from
Aug 30, 2018
Merged

Add empty handler for new ruleset clause packet #180

merged 1 commit into from
Aug 30, 2018

Conversation

zekoz
Copy link
Contributor

@zekoz zekoz commented Aug 10, 2018

No description provided.

@lonemadmax
Copy link
Contributor

I would expect this kind of change to be hand in hand with server updates. Right now this is dead code, not even an empty stub. Is anyone reviewing the changes since 8e2801d583e1aaaee2bfaf3229e2ba233b518ff3 (June) and how they might affect Freeciv-web?

@lonemadmax lonemadmax closed this Aug 26, 2018
@zekoz
Copy link
Contributor Author

zekoz commented Aug 26, 2018

I'm not sure why you closed, but a handler is definitely needed... https://github.com/freeciv/freeciv/blob/fa63c2a6b39d0d3ff63683fda2268aace248d1c3/server/ruleset.c#L7828

@lonemadmax
Copy link
Contributor

I thought it was clear in the other message: a handler is needed only with a server that sends that packet, which was introduced less than a month ago (freeciv/freeciv@fa63c2a#diff-a56f2ec537afebac742457d2ba401e47), and we are using a version from June:

FCREV=8e2801d583e1aaaee2bfaf3229e2ba233b518ff3

@zekoz
Copy link
Contributor Author

zekoz commented Aug 30, 2018

freecivweb.org uses a more recent version that actually sends this packet. Please just merge.

@lonemadmax
Copy link
Contributor

And I don't see that in the commit, or the fork.

This change should come with a change in version.txt to point at least to the one that introduces the packet, and that be just one of many commits moving from the current one to there. Do you know freeciv enough and have checked that the rest of changes don't impact freeciv-web in any way (I'm not attacking you or asking for a curriculum, I myself can't do it except for the most simple ones)?

That said, merging on the basis that it is dead code that will have to be included some day and doing it now seems it may help freecivweb.org a bit.

But it's probably my last of this kind. I won't disagree with others doing it, though; they'll have to step in anyway for any change in the freeciv tracked commit.

@lonemadmax lonemadmax reopened this Aug 30, 2018
@lonemadmax lonemadmax merged commit 3b5df57 into freeciv:develop Aug 30, 2018
@zekoz
Copy link
Contributor Author

zekoz commented Aug 30, 2018

freecivweb.org now runs based on d196b8a0131637ce5947c15d04231c493bff03fb. You should now be able to change version.txt to refer to that.

@kvilhaugsvik
Copy link
Member

I just started updating from the current revision. I'll hopefully be able to test and merge the update during the weekend. (Any new dependencies, configuration options or other stuff I'll have to change to make my bare metal install from June work?)

@mchenryc
Copy link
Contributor

@kvilhaugsvik

... or other stuff I'll have to change...

If you are not using a clean checkout, you may have old files in freeciv-web/src that need to be manually deleted (per changes in #186, see this comment). Git should mark them as untracked, but I suspect anyone used to the old build may expect them to be present.

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

Successfully merging this pull request may close these issues.

4 participants