[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1786597-test-suite into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug-1786597-test-suite into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug-1786597-test-suite/+merge/352928 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1786597-test-suite. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1786597-test-suite into lp:widelands
@bunnybot merge force -- https://code.launchpad.net/~widelands-dev/widelands/bug-1786597-test-suite/+merge/352928 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1786597-test-suite. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1786597-test-suite into lp:widelands
Continuous integration builds have changed state: Travis build 3780. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/415308939. Appveyor build 3579. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1786597_test_suite-3579. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1786597-test-suite/+merge/352928 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1786597-test-suite. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1786597-test-suite into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug-1786597-test-suite into lp:widelands has been updated. Commit message changed to: Fix the testsuite for carriers For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug-1786597-test-suite/+merge/352928 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1786597-test-suite. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1786597-test-suite into lp:widelands
Transient error on Travis @bunnybot merge force -- https://code.launchpad.net/~widelands-dev/widelands/bug-1786597-test-suite/+merge/352928 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1786597-test-suite. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1786597-test-suite into lp:widelands
Thanks for the review! :) @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/bug-1786597-test-suite/+merge/352928 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1786597-test-suite. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1786597-test-suite into lp:widelands
Review: Approve As far as it concerns me, this looks ok ;-P -- https://code.launchpad.net/~widelands-dev/widelands/bug-1786597-test-suite/+merge/352928 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1786597-test-suite. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1786597-test-suite into lp:widelands
No, it will fix the test suite. See the changes to the binary file on he bottom ;) -- https://code.launchpad.net/~widelands-dev/widelands/bug-1786597-test-suite/+merge/352928 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1786597-test-suite. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1786597-test-suite into lp:widelands
Review: Needs Information Does this end this branch' reason of existence? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1786597-test-suite/+merge/352928 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1786597-test-suite. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1786597-test-suite into lp:widelands
I tried to add https://bugs.launchpad.net/widelands/+bug/1535115/comments/33 into the compatibility code but it broke the test suite again, so I used the branch to update the mapobjectpacket in the problematic test scenario and removed the savegame compatibility. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1786597-test-suite/+merge/352928 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1786597-test-suite. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1786597-test-suite into lp:widelands
Continuous integration builds have changed state: Travis build 3770. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/414860025. Appveyor build 3569. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1786597_test_suite-3569. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1786597-test-suite/+merge/352928 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1786597-test-suite. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1786597-test-suite into lp:widelands
In current problematic design, a carrier in an invalid state can easily get stuck, so we will need to test some actual savegames to answer that. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1786597-test-suite/+merge/352928 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1786597-test-suite. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1786597-test-suite into lp:widelands
Oops... fixed! If this code sufficient or will it barf it the carrier is in init rather than wait? I don't care much about 100% correctness for the saveloading at this point. I am working on a follow-up branch and use an enum class for the states. Working on that has flagged up lots of hard-coded and fragile stuff from old code. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1786597-test-suite/+merge/352928 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1786597-test-suite. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1786597-test-suite into lp:widelands
Inline comment.
Also, if you haven't done it, check this comment:
https://bugs.launchpad.net/widelands/+bug/1535115/comments/33
Diff comments:
> === modified file 'src/logic/map_objects/tribes/carrier.cc'
> --- src/logic/map_objects/tribes/carrier.cc 2018-08-09 11:11:15 +
> +++ src/logic/map_objects/tribes/carrier.cc 2018-08-11 11:58:45 +
> @@ -433,9 +435,14 @@
> try {
>
> uint8_t packet_version = fr.unsigned_8();
> - if (packet_version == kCurrentPacketVersion) {
> + if (packet_version >= 1 && packet_version <=
> kCurrentPacketVersion) {
> Carrier& carrier = get();
> carrier.operation_ = fr.signed_32();
> + if (packet_version == 1) {
> + if (carrier.operation_ == NO_OPERATION) {
> + carrier.operation_ == INIT;
This looks like it should be =
> + }
> + }
> } else {
> throw UnhandledVersionError("Carrier", packet_version,
> kCurrentPacketVersion);
> }
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1786597-test-suite/+merge/352928
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1786597-test-suite into lp:widelands.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1786597-test-suite into lp:widelands
GunChleoc has proposed merging
lp:~widelands-dev/widelands/bug-1786597-test-suite into lp:widelands.
Commit message:
Added savegame compatibility for carriers
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1786597 in widelands: "Congestion branch broke the test suite"
https://bugs.launchpad.net/widelands/+bug/1786597
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1786597-test-suite/+merge/352928
This should unbreak the test suite
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1786597-test-suite into lp:widelands.
=== modified file 'src/logic/map_objects/tribes/carrier.cc'
--- src/logic/map_objects/tribes/carrier.cc 2018-08-09 11:11:15 +
+++ src/logic/map_objects/tribes/carrier.cc 2018-08-11 11:58:45 +
@@ -421,7 +421,9 @@
==
*/
-
+// Increasing the version number will break the test suite,
+// so we need to provide savegame compatibility here.
+// Compatibility is not needed for map loading.
constexpr uint8_t kCurrentPacketVersion = 2;
Carrier::Loader::Loader() {
@@ -433,9 +435,14 @@
try {
uint8_t packet_version = fr.unsigned_8();
- if (packet_version == kCurrentPacketVersion) {
+ if (packet_version >= 1 && packet_version <= kCurrentPacketVersion) {
Carrier& carrier = get();
carrier.operation_ = fr.signed_32();
+ if (packet_version == 1) {
+if (carrier.operation_ == NO_OPERATION) {
+ carrier.operation_ == INIT;
+}
+ }
} else {
throw UnhandledVersionError("Carrier", packet_version, kCurrentPacketVersion);
}
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
