As reported by leper in rP19801, the BSD build is broken because the timezone variant expected by our lobby timestamp parsing code was removed.
The most reliable and clean way to solve this issue I found was using ICU, which is used by SpiderMonkey and we ship with that.
This also allows removing the wposix inclusion which was only needed for this one strptime call and caused the first revert of this patch.
Details
Details
Join the a23 lobby with a chat client like pidgin to see the correct timestamps of the last chat messages.
Join with the unpatched lobby if you're not using BSD to see that the stamps match.
Apply the patch and see that we get the same timestamps.
Read the specs at http://icu-project.org/apiref/icu4c/classicu_1_1SimpleDateFormat.html#a4c6fb4cda13384e3536360d7aff75428 to verify that the SimpleDateFormat variable we create uses the default locale.
Read the Detailed Description part of that page to see that the formatting string is correct.
Notice that timezone is gone, so it should work on BSD.
Would be nice if someone can compile it on Windows to confirm that the nothing stupid was missed.
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 2671 Build 4554: Vulcan Build (Windows) Jenkins Build 4553: Vulcan Build Jenkins Build 4552: arc lint + arc unit
Event Timeline
Comment Actions
As reported by leper in rP19801, the BSD build is broken because the timezone variant expected by our lobby timestamp parsing code was removed.
I'm just relaying the relevant part of some email conversation I was added to between Itms and a BSD port maintainer.
The most reliable and clean way to solve this issue I found was using ICU, which is used by SpiderMonkey and we ship with that.
It's used by our i18n/l10n code, SpiderMonkey might use it too, but that is not necessarily the same one that you are using (at least certainly isn't based on the headers you pull in).
I suggest looking at what L10n.h provides, since this seems very much like it would solve that issue nicer. Or if it doesn't extend it so it does.
| source/lobby/XmppClient.cpp | ||
|---|---|---|
| 948 | Why not have this on the stack instead of leaking it like you do right now? | |
| 952 | Why does this convert to double first, then does a floating point division, then converts back to some integer type, instead of just dividing by an integer? | |
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jw:8080/job/phabricator/1783/ for more details.
Comment Actions
Executing section Default... Executing section Source... Executing section JS... Executing section XML GUI... Executing section Python... Executing section Perl...
http://jw:8080/job/phabricator_lint/344/ for more details.
| source/lobby/XmppClient.cpp | ||
|---|---|---|
| 952 | This parse returns UDate, which is double. Dividing a double by an integer converts the integer to double and then the ratio is casted to std::time_t afaik. So using 1000.0 avoids one step, no? | |
| source/lobby/XmppClient.cpp | ||
|---|---|---|
| 952 | Ah, I guess reading the docs does help. | |
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jw:8080/job/phabricator/1785/ for more details.
Comment Actions
Executing section Default... Executing section Source... Executing section JS... Executing section XML GUI... Executing section Python... Executing section Perl...
http://jw:8080/job/phabricator_lint/346/ for more details.
Comment Actions
Stan tested it on Windows, compiles and works as expected.
It must work on BSD too since the l10n.cpp file worked there before.
So inclined to commit it if there are no further comments.
At the end of october we will have it tested for non-daylight-saving-time as well.
Comment Actions
This looks very good, and I'd be happy to see someone test it on BSD but if it doesn't get any love I'm ok with seeing it committed.
Comment Actions
Committed in rP19947.
Forgot the Differential-Revision entry in the commit. Will have to abolish it in order to close it, unless someone wants to accept this.
Comment Actions
(Actually would be nicer to have it listed correctly and find a way to close something, even if not accepted prior.)
Comment Actions
Has anyone considered setting up a VM, or even asking downstream if that works (since downstream reported that failure)?
Comment Actions
If one isn't collecting VM images and has deduced that a one-line patch must be correct,
then spending that hour or two to install and configure BSD on a VM, installing the dependencies, checking out, patching, compiling and running 0ad is moderately unproportional.
or (anyone) even asking downstream if that works (since downstream reported that failure)?
The reviewer is at least one country away from his computer since the last comment.
If that question implies that the one who received the report also didn't respond,
then the answer is no.
I'm just relaying the relevant part of some email conversation I was added to between Itms and a BSD port maintainer.
In case you want anyone in the same country as their computer who has broken something to communicate,
then cc'ing or providing a contact would make it more feasible to find and communicate with busy developers on the other side.
Comment Actions
(Actually would be nicer to have it listed correctly and find a way to close something, even if not accepted prior.)
Well, that sounds different than that one quote above ;-)
then spending that hour or two to install and configure BSD on a VM, installing the dependencies, checking out, patching, compiling and running 0ad is moderately unproportional.
Indeed, then again one is actually able to test on that platform (that shares quite a few things with another where we cannot do so easily).
or (anyone) even asking downstream if that works (since downstream reported that failure)?
The reviewer is at least one country away from his computer since the last comment.
Last I heard that wasn't a different country and something about access and the whole thing not taking that long. Then again I do consider the previous post a friendly reminder and nothing else.
If that question implies that the one who received the report also didn't respond,
then the answer is no.
I was CC'd to that, and did point to the original commit, then again I do have other things to do than follow up on everything when other people are just as capable.
Comment Actions
Forgot to ref it in the commit message.
(Revision proposals can only be closed by a commit or if they were accepted . Mentioned it in the using phabricator thread recently).
(Also had to use close-revision D760 because the upstream bugfix isn't applied on this instance. But I'm so happy this thing is finally closed now after years of reverts, concerns and omnidirectional breakage xD)