Change jerry_port interface to allow for a correct implementation of timezones. #2540

crazy2be · 2018-09-25T22:05:41Z

The previous jerry_port interface did not allow timezones to be handled correctly,
even if the host system was up to the task. This PR changes the jerry_port interface
to allow a completely correct implementation to exist, and provides one (for Linux/BSD
systems) in default-date.c.

There is some additional background in my comment on the original issue #1661 (comment), but essentially, this solution resolves the original ticket by allowing the jerry_port interface to from the time-invariant

jerry_port_get_time_zone(utc_offset_out*, is_dst_out*)

to the time-dependent

jerry_port_get_local_tza(time, is_utc)

As dst state and timezone rules in general change over time, it's necessary to give the time as an argument to correctly determine the local time offset at that time.

This interface also allows the platform to figure out the local time adjustment for a given date in whichever way it pleases. On most Linux / BSD systems, the tm_gmtoff field of struct tm can be used. On windows, GetTimeZoneInformationForYear can be used. On most embedded platforms, UTC can always be assumed, or some other local timezone.

The alternative is to have jerryscript link in some external timezone library, with all of the logic for handling timezones. However, I can't see any big advantage to this approach, and the libraries are often quite large. Most systems that run JerryScript simply don't need the complexity (they can just run UTC), and the ones that do need the complexity probably already have some other system which will be fairly easy to just link into the provided jerry_port interface (Linux and Windows are both quite easy, about 10 lines, to hook into this interface).

Fixes #1661

JerryScript-DCO-1.0-Signed-off-by: crazy2be [email protected]

crazy2be · 2018-10-01T15:41:18Z

Can one of the maintainers take a look? Maybe @zherczeg?

zherczeg · 2018-10-02T04:03:50Z

I don't know much about times but the patch looks reasonable. Does somebody knows more about time zones here?

LaszloLango · 2018-10-02T06:19:46Z

@crazy2be Sorry for the late response. I'll take a look on this patch later this week.

rerobika

@crazy2be Hence this PR is solution for the daylight saving issue, please enable these tests to ensure the correctness of the patch.

crazy2be · 2018-10-02T20:31:58Z

Those tests do pass for me, but it took some doing. Initially, they did not pass, and after investigating a bit, they always appeared to be off by 3 hours. After quite some digging, I discovered this is because the author lives in California, and the version of the tests we run assumes the California timezone. This was fixed in [1], but the es5-tests branch that run-test-suite-test262.sh checks out doesn't have that change. However, if I change my system timezone to California time, all of the tests pass:

=== Summary ===
 - Ran 11552 tests
 - All tests succeeded

ch15/15.9/15.9.3/S15.9.3.1_A5_T1 passed in non-strict mode
ch15/15.9/15.9.3/S15.9.3.1_A5_T2 passed in non-strict mode
ch15/15.9/15.9.3/S15.9.3.1_A5_T3 passed in non-strict mode
ch15/15.9/15.9.3/S15.9.3.1_A5_T4 passed in non-strict mode
ch15/15.9/15.9.3/S15.9.3.1_A5_T5 passed in non-strict mode
ch15/15.9/15.9.3/S15.9.3.1_A5_T6 passed in non-strict mode

Given this caveat, do we still want these tests enabled by default? Or should I just change the TODO? Ideally, it seems like [1] should be cherry-picked onto the es5-tests branch, but that branch seems fairly dead - last commit was more than 3 years ago, in July 2015.

[1] https://github.com/tc39/test262/pull/573/files

akosthekiss

First, mostly stylistic observations. More to follow.

akosthekiss · 2018-10-03T16:29:31Z

jerry-port/default/default-date.c

@@ -21,33 +24,32 @@
 #include "jerryscript-port-default.h"

 /**
- * Default implementation of jerry_port_get_time_zone. Uses 'gettimeofday' if
- * available on the system, does nothing otherwise.
+ * Default implementation of jerry_port_get_local_tza. Uses the `tm_gmtoff` field


Current documentation style doesn't use backticks (i.e., markdownish syntax) in doc comments but single quotes. It's not necessarily the best approach but at least it's consistent. (At least, I hope.) Please follow the style found throughout the code base.

akosthekiss · 2018-10-03T16:31:06Z

jerry-port/default/default-date.c

- * @return true - if 'gettimeofday' is available and executed successfully,
- *         false - otherwise.
+ * @return offset between utc and localtime at the given unix timestamp, if
+ *         available. 0 otherwise.


I'd suggest "UTC", "local time", and "available, 0 otherwise."

akosthekiss · 2018-10-03T16:32:29Z

jerry-port/default/default-date.c

-  if (gettimeofday (&tv, &tz) == 0)
+#ifdef HAVE_TM_GMTOFF
+  struct tm tm;
+  time_t now = (time_t)(unix_ms / 1000);


I'm not sure why our code style checker did not catch this but there should be a space after the type cast (i.e., (time_t) (unix_ms ... )

akosthekiss · 2018-10-03T16:36:03Z

jerry-core/include/jerryscript-port.h

- * @param[out] tz_p time zone structure to fill.
- * @return true  - if success
- *         false - otherwise
+ * @return milliseconds between local time and utc for the given timestamp.


The documentation in a public header, like this one, should be complete, i.e., the params must be documented.

Suggestion: "UTC".

akosthekiss · 2018-10-03T17:10:29Z

jerry-core/include/jerryscript-port.h

+ * Get local timezone adjustment, in milliseconds. This value, added to a timestamp in UTC
+ * time, should yield local time. Ideally, this function should satisfy the stipulations
+ * applied to LocalTZA in section 20.3.1.7 of ecmascript version 9.0 [1].
+ * [1] https://www.ecma-international.org/ecma-262/9.0/#sec-local-time-zone-adjustment


IIRC, the code base doesn't use url references to external documents, it just mentions the standard name, the version, and the section. Like:

* See also: * ECMA-262 v5, 15.9.1.9

(Taken from another code block, needs to be adapted.)

akosthekiss · 2018-10-04T09:31:33Z

re: the time zone issue during test262#es5-tests testing
The Travis CI job already ensures the proper time zone by explicitly setting TZ=America/Los_Angeles when running tools/run-tests.py $OPTS and OPTS="--test262". However, this doesn't ensure the proper TZ during local testing, so it might be a better way to incorporate the environment fixup in tools/run-tests.py.

akosthekiss · 2018-10-04T13:22:50Z

I've tried to address the test262 time zone issue in #2551 .

akosthekiss

Some more comments.

akosthekiss · 2018-10-04T17:31:20Z

jerry-port/default/default-date.c

+#ifdef HAVE_TM_GMTOFF
+  struct tm tm;
+  time_t now = (time_t) (js_time_ms / 1000);
+  localtime_r (&now, &tm);


A little bit of thinking aloud: localtime_r is not C99 but an extension defined by the POSIX standard. Which means that this code will not necessarily compile on all targets. Unless the existence of the tm.tm_gmtoff GNU extension implies the POSIX extension as well.

Another approach could be to use timegm(&tm) - t instead of tm.tm_gmtoff but timegm is also a GNU/BSD extension. (Which could be emulated if not present on the system, but I'm not sure it would be worth the effort.)

So, it seems to me that we need to rely on at least two extensions to get this work. I'm not sure yet which way to go. (That's why I was thinking aloud. I'm open to inputs.)

localtime_r seems ubiquitous, even on embedded systems. I find it hard to imagine a system without it but with tm.tm_gmtoff. tm.tm_gmtoff is not specified in any standard, but appears to exist as a GNU extension on linux (as you said) [0], and also in FreeBSD [1], OpenBSD [2], and OSX [3]. Windows has localtime_s [4], which seems to be the same as localtime_r, but with a different name. However, it doesn't have tm_gmtoff, so we couldn't support it this way anyway. Windows also doesn't support timegm, and the mkgmtime it supports doesn't support dates prior to 1970 [5], so you are likely going to need some non-standard solution there anyway (v8 resorts to calling WIN32API functions on windows).

[0] https://linux.die.net/man/3/localtime_r
[1] https://www.freebsd.org/cgi/man.cgi?query=localtime_r
[2] https://man.openbsd.org/ctime.3
[3] https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/localtime_r.3.html
[4] https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/localtime-s-localtime32-s-localtime64-s?view=vs-2017
[5] https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/mkgmtime-mkgmtime32-mkgmtime64?view=vs-2017

Please note that we are targeting embedded systems way below the embedded-Linux world as well. And there, it is easy to find missing bits and pieces. E.g., it's a bit hard to dig for info, but it's probable that neither TI's MSP430 C compiler nor the IAR Embedded Workbench have support for localtime_r or tm.tm_gmtoff. And the ARM Compiler (version 6) does have a _localtime_r but no info on struct tm.

That being said, the platforms these compilers compile for will most probably have their own implementation for all/most jerry port functions instead of these default implementations.

Still, it's good to keep those ideals (i.e., C99 conformance) in mind that drive the project. For the core engine library, we must be strict, I think. For libraries around it, we might be less strict, provided that the extensions are properly guarded. Here that seems to be the case.

Anyway, I'd like to hear the opinion of other reviewers. I summon @zherczeg @LaszloLango

My point was mainly that if a given platform has tm.tm_gmtoff then it most probably has localtime_r as well. But maybe there is a better way that has support on more platforms, I'm open to suggestions. We can always change / improve this later, however.

If I understand correctly this is the default implementation. Low-end targets must have their own implementation (or return 0 if not supported).

Btw what about windows? Does gcc on windows support this?

Correct, this is the default implementation. Most embedded targets won't even use this, or they will just use the return 0; default implementation, which should have the same "assume UTC" behaviour that they always had.

Windows does not support tm.tm_gmtoff. It's also not supported by mingw. However, it's fairly straightforward to shim it by using GetTimeZoneInformationForYear if someone wants to do this at some point. I'd rather not do it in this PR, as it's already gotten quite big (and it's a PITA for me to test on windows...)

akosthekiss · 2018-10-04T17:33:07Z

jerry-port/default/default-date.c

 */
-bool jerry_port_get_time_zone (jerry_time_zone_t *tz_p) /**< [out] time zone structure to fill */
+double jerry_port_get_local_tza (double js_time_ms,  /**< ms since unix epoch */


I'm not sure about the js_ prefix for this param. It's time in millisecs, which is quite well expressed by time_ms. There is not much JS-specific in that concept.

I'd only done this because it was a double, but I agree, I think unix_ms is probably a better name. (or do you prefer time_ms? Both seem better than js_time_ms).

unix_ms is fine by me

akosthekiss · 2018-10-04T17:47:57Z

jerry-core/include/jerryscript-port.h

- * Get timezone and daylight saving data
+ * Get local timezone adjustment, in milliseconds. This value, added to a timestamp in UTC
+ * time, should yield local time. Ideally, this function should satisfy the stipulations
+ * applied to LocalTZA in section 20.3.1.7 of ecmascript version 9.0.


Timezone might better be time zone, but I'm not completely sure, even the spec uses timezone at least once (although it uses time zone more often). About ECMAScript instead of ecmascript, I'm positive.

And I also think that the documentation of all other port functions are kept in sync in the docs/05.PORT-API.md and in jerry-core/include/jerryscript-port.h. This should also happen here. Perhaps the two texts should be merged into a single description and used at both places. (Especially as the current text here is a little bit vague about the case when is_utc == false.)

akosthekiss · 2018-10-04T17:48:31Z

jerry-core/include/jerryscript-port.h

- *      CONFIG_DISABLE_DATE_BUILTIN is _not_ defined. Otherwise this function is
- *      not used.
+ *      CONFIG_DISABLE_DATE_BUILTIN is _not_ defined. Otherwise this function
+ *      is not used.


Nitpicking: moving the "is" back to the end of the previous line would give a bit less churn.

akosthekiss · 2018-10-04T17:52:43Z

jerry-core/ecma/builtin-objects/ecma-builtin-helpers-date.c

-   */
-  return tz->daylight_saving_time * ECMA_DATE_MS_PER_HOUR;
-} /* ecma_date_daylight_saving_ta */
-
 /**
 * Helper function to get local time from UTC.


The comment of this function (ecma_date_local_time_zone) became (was?) misleading. Especially when considered together with the other changed function, ecma_date_utc. That latter one is documented as "get utc from local time" which is aligned to the code, but this current one has an inverse doc ("get local time from UTC") although it does not implement the inverse operation. Could be updated in this patch.

I noticed that, but I wasn't sure if you guys would want it fixed. I renamed it to ecma_date_local_time_zone_adjustment and updated the docs as well.

akosthekiss

The test262 runner has been changed too much.

akosthekiss · 2018-10-05T21:31:03Z

tools/runners/run-test-suite-test262.sh

+  done
+  (>&2 echo)
+  (>&2 echo)
+}


Now, this is something that I'd like to see in a different PR. This is a feature/improvement that has nothing to do with the goals of this current PR. This change could work no matter what port API is in place.

akosthekiss · 2018-10-05T21:32:18Z

tools/runners/run-test-suite-test262.sh


-echo "Starting test262 testing for ${ENGINE}. Running test262 may take a several minutes."
+function progress_monitor() {
+  NUM_LINES_EXPECTED=11558


This is too hard-wired. How did you get this number? Why can't it be computed here by this script? (BTW, summary prints me 11552 executed tests.)

This is number of lines, not number of tests, so it is slightly more than the number of tests. Previously it just ran for several minutes without giving any feedback, slightly wrong feedback (like if the test suite changes) seems a lot better than no feedback to me. I think the right place to put this is probably down a level in test262.py, but that is a considerably more involved change that I'm not about to attempt.

I suppose you could always just print number of tests executed, and not the percentage... at least then you can see that it's progressing. It still seems worse than giving the percentage, even if it's slightly off, but maybe it's an approach that will age better.

akosthekiss · 2018-10-05T21:40:59Z

tools/runners/run-test-suite-test262.sh

+  (>&2 echo)
+  while read line; do
+    FRAC=$((NUM_LINES_GOTTEN * 100 / NUM_LINES_EXPECTED))
+    (>&2 echo -ne "\rEstimated ${FRAC}% complete (${NUM_LINES_GOTTEN}/${NUM_LINES_EXPECTED})...")


This will print 10k lines. On a live console, that's no problem as lines end in CR only without LF. However, it still blows up logs if output is redirected (see Travis CI raw logs). Printing progress (the same way, with CRs) only when the percentage increases, that would be the proper solution.

akosthekiss · 2018-10-05T21:52:01Z

jerry-core/ecma/builtin-objects/ecma-builtin-helpers-date.c


 /**
- * Helper function to get utc from local time.
+ * Helper function to get utc time from local time.


Suggestion: UTC

akosthekiss · 2018-10-05T22:05:10Z

jerry-core/include/jerryscript-port.h

 */
-bool jerry_port_get_time_zone (jerry_time_zone_t *tz_p);
+double jerry_port_get_local_time_zone_adjustment (double unix_ms, bool is_utc);


Question to other reviewers (again, ping @zherczeg @LaszloLango ): do we want this long identifiers / port API identifiers? jerry_port_get_local_time_zone_adjustment or jerry_port_get_local_tza or ...?

I prefer full words, so I like the current one.

akosthekiss · 2018-10-05T22:09:04Z

docs/05.PORT-API.md

 */
-bool jerry_port_get_time_zone (jerry_time_zone_t *tz_p)
+double jerry_port_get_local_time_zone_adjustment (double unix_ms, /**< ms since unix epoch */
+                                                  bool is_utc)    /**< is the time above in utc? */


Suggestion: UTC

akosthekiss · 2018-10-09T10:08:21Z

Now that #2551 has landed, it might be worth rebasing this PR onto latest master.

crazy2be · 2018-10-09T17:24:41Z

I think I addressed all the outstanding concerns, can you guys take another look?

crazy2be · 2018-10-10T02:08:32Z

Just looked into the build failures - they seem to be due to my CMake changes not having the desired effect (on Ubuntu, the target is getting built as though there is no tm_gmtoff support, when there most certainly is). The rest of the changes should be reviewable, however.

zherczeg

The change makes sense but I have some questions.

zherczeg · 2018-10-10T04:19:35Z

docs/05.PORT-API.md

+ * The timestamp can be specified in either UTC or local time, depending on
+ * the value of is_utc. Adding the value returned from this function to
+ * a timestamp in UTC time should result in local time for the current time
+ * zone.


What about the reverse? Subtracting from local gives an utc?

Correct, assuming you specify the timestamp in local time (is_utc is false). Should I make this explicit in the docs?

zherczeg · 2018-10-10T04:22:34Z

docs/05.PORT-API.md

- *         false - otherwise
+ * @param unix_ms The unix timestamp we want an offset for, given in
+ *                millisecond precision (could be now, in the future,
+ *                or in the past)


What is the base? 1970.01.01? How leap seconds are handled? Do they need to be handled?

Correct, the base is 1970.01.01. The given unix_ms is assumed to be unix time - i.e. every day is exactly 86400 seconds, leap seconds cause the same second to occur twice. This seems to be the same set of assumptions made by the date object according to MDN [1]

The JavaScript date is based on a time value that is milliseconds since midnight January 1, 1970, UTC. A day holds 86,400,000 milliseconds. The JavaScript Date object range is -100,000,000 days to 100,000,000 days relative to January 1, 1970 UTC.

This API only cares about handling them correctly to the extent that it allows it to most correctly determine if dst is in effect, other than that, they should not affect the returned local_time_zone_adjustment, since leap seconds occur at the same time around the world, regardless of local timezone.

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date

zherczeg · 2018-10-10T04:24:03Z

jerry-core/include/jerryscript-port.h

 */
-bool jerry_port_get_time_zone (jerry_time_zone_t *tz_p);
+double jerry_port_get_local_time_zone_adjustment (double unix_ms, bool is_utc);


I prefer full words, so I like the current one.

zherczeg · 2018-10-10T04:24:45Z

jerry-port/default/CMakeLists.txt

+CHECK_STRUCT_HAS_MEMBER ("struct tm" tm_gmtoff time.h HAVE_TM_GMTOFF)
+if(HAVE_TM_GMTOFF)
+  set(DEFINES_PORT_DEFAULT ${DEFINES_PORT_DEFAULT} HAVE_TM_GMTOFF)
+endif()


Is this only affects the default implementation?

Correct. It also doesn't seem to work, and I haven't been able to figure out why yet. My cmake-fu is not great.

I haven't investigated all the details of the problem but my guess is that the root of the problem is that tm_gmtoff is non-standard. See, for example, in glibc:

tm_gmtoff is only declared if __USE_MISC is defined: https://sourceware.org/git/?p=glibc.git;a=blob;f=time/bits/types/struct_tm.h;h=b13b631228d0ec36691b25db2e1f9b1d66b54bb0;hb=HEAD#l19

and it only defines __USE_MISC if _DEFAULT_SOURCE is defined: https://sourceware.org/git/?p=glibc.git;a=blob;f=include/features.h;h=5bed0a499605a3a26d55443f3c8b7e67de152f74;hb=HEAD#l368

but when a compiler is instructed to compile in strict C99 mode then it will not pass extra macros to the preprocessor and thus will not "enable" such non-standard libc parts.

So, if CHECK_STRUCT_HAS_MEMBER calls the compiler with default settings, i.e., with no extra defines, then the compilation during the check will not see tm_gmtoff. Perhaps setting CMAKE_REQUIRED_DEFINITIONS to _DEFAULT_SOURCE before CHECK_STRUCT_HAS_MEMBER may solve the problem (see: https://cmake.org/cmake/help/v3.0/module/CheckStructHasMember.html). But even if this works, it may be glibc-specific.

I think it is something to do with my defines not getting passed. If I put a message() inside the if, it gets printed (at least, if I run (cd jerry-port/default && cmake .)). And compiling outside of cmake or any build system compiles fine without errors:

#include <stdio.h> #include <time.h> #include <stdbool.h> double jerry_port_get_local_time_zone_adjustment (double unix_ms, /**< ms since unix epoch */ bool is_utc) /**< is the time above in UTC? */ { struct tm tm; time_t now = (time_t) (unix_ms / 1000); localtime_r (&now, &tm); if (!is_utc) { now -= tm.tm_gmtoff; localtime_r (&now, &tm); } return ((double) tm.tm_gmtoff) * 1000; } /* jerry_port_get_local_time_zone_adjustment */ int main() { double adj = jerry_port_get_local_time_zone_adjustment(0, 0); printf("%lf\n", adj); }

(the above compiles without any warnings or errors with just gcc test_gmtoff.c)

I verified that it isn't trying to use tm.tm_gmtoff (and getting, say, bogus values) by putting a printf in default-date.c before each return statement. The one we hit is the one immediately before return 0;, so it's not even attempting to use the tm.tm_gmtoff path.

Hmm... looking at CMakeCache.txt after a clean build, maybe I'm wrong, and for some reason, when run as part of the larger project, tm.tm_gmtoff isn't found.

Confirmed that when running cmake . in jerry-port/default, my CMakeCache.txt is generated with HAVE_TM_GMTOFF:INTERNAL=1, but when run as part of tools/build.py, the generated CMakeCache.txt (in build/tests/test262_tests/CMakeCache.txt) has HAVE_TM_GMTOFF:INTERNAL=, as above.

Looking at the two different CMakeCache.txt files, no glaring differences in build configuration stick out at me.

Ah, I see now what you were saying. I didn't realize jerryscript compiled in strict c99 mode (which, as you said, makes tm.tm_gmtoff invisible). gcc -std=c99 test_gmtoff.c does indeed fail as you predicted.

Huh, so the default source code that check_struct_has_member uses to check if the given struct has the given member actually fails to compile with the default jerryscript compiler options... Ugh.

Performing C SOURCE FILE Test HAVE_TM_GMTOFF failed with the following output: Change Dir: /home/fbuser/src/jerryscript/build/tests/test262_tests/CMakeFiles/CMakeTmp Run Build Command:"/usr/bin/make" "cmTC_a80df/fast" /usr/bin/make -f CMakeFiles/cmTC_a80df.dir/build.make CMakeFiles/cmTC_a80df.dir/build make[1]: Entering directory '/home/fbuser/src/jerryscript/build/tests/test262_tests/CMakeFiles/CMakeTmp' Building C object CMakeFiles/cmTC_a80df.dir/src.c.o /usr/bin/cc -D_DEFAULT_SOURCE -flto -fno-fat-lto-objects -std=c99 -pedantic -fno-builtin -fno-stack-protector -Wall -Werror=all -Wextra -Werror=extra -Wformat-nonliteral -Werror=format-nonliteral -Winit-self -Werror=init-self -Wconversion -Werror=conversion -Wsign-conversion -Werror=sign-conversion -Wformat-security -Werror=format-security -Wmissing-declarations -Werror=missing-declarations -Wshadow -Werror=shadow -Wstrict-prototypes -Werror=strict-prototypes -Wundef -Werror=undef -Wold-style-definition -Werror=old-style-definition -Wno-stack-protector -Wno-attributes -Werror -Wlogical-op -Werror=logical-op -DHAVE_TM_GMTOFF -o CMakeFiles/cmTC_a80df.dir/src.c.o -c /home/fbuser/src/jerryscript/build/tests/test262_tests/CMakeFiles/CMakeTmp/src.c /home/fbuser/src/jerryscript/build/tests/test262_tests/CMakeFiles/CMakeTmp/src.c:4:5: error: function declaration isn’t a prototype [-Werror=strict-prototypes] int main() ^ /home/fbuser/src/jerryscript/build/tests/test262_tests/CMakeFiles/CMakeTmp/src.c: In function ‘main’: /home/fbuser/src/jerryscript/build/tests/test262_tests/CMakeFiles/CMakeTmp/src.c:4:5: error: old-style function definition [-Werror=old-style-definition] cc1: all warnings being treated as errors CMakeFiles/cmTC_a80df.dir/build.make:65: recipe for target 'CMakeFiles/cmTC_a80df.dir/src.c.o' failed make[1]: *** [CMakeFiles/cmTC_a80df.dir/src.c.o] Error 1 make[1]: Leaving directory '/home/fbuser/src/jerryscript/build/tests/test262_tests/CMakeFiles/CMakeTmp' Makefile:126: recipe for target 'cmTC_a80df/fast' failed make: *** [cmTC_a80df/fast] Error 2 Source file was: #include <time.h> int main() { (void)sizeof(((struct tm *)0)->tm_gmtoff); return 0; }

Finally got it all to build. On the plus side, I'm a lot more familiar with cmake now :).

zherczeg · 2018-10-10T04:26:11Z

jerry-port/default/default-date.c

+#ifdef HAVE_TM_GMTOFF
+  struct tm tm;
+  time_t now = (time_t) (js_time_ms / 1000);
+  localtime_r (&now, &tm);


If I understand correctly this is the default implementation. Low-end targets must have their own implementation (or return 0 if not supported).

crazy2be · 2018-10-12T19:24:39Z

I think I addressed all the outstanding concerns, and the build passes now. Can you take another look?

akosthekiss

I'm almost OK with the PR

docs/05.PORT-API.md

crazy2be · 2018-10-16T19:39:00Z

@rerobika: Those tests have been enabled, and pass.
@akosthekiss: I addressed your nit.
@zherczeg: I updated the docs to make the answers to your questions more explicit.

The travis-ci build seems to have hung on the signed-off-by check, I re-triggered it which should make the build go green. Everything else is passing.

zherczeg

LGTM

rerobika

Getting close, just a few minor things.

rerobika · 2018-10-17T09:13:38Z

jerry-port/default/default-date.c

@@ -13,6 +13,9 @@
 * limitations under the License.
 */

+#ifdef HAVE_TM_GMTOFF
+#include <time.h>
+#endif


Please add a closing comment for #endif.

#endif /* HAVE_TM_GMTOFF */

rerobika · 2018-10-17T09:17:11Z

jerry-port/default/default-date.c

@@ -13,6 +13,9 @@
 * limitations under the License.
 */

+#ifdef HAVE_TM_GMTOFF
+#include <time.h>
+#endif
 #ifdef __GNUC__
 #include <sys/time.h>
 #endif


This patch does not affect this code path, but I would be great to unify the closing #endif comments as well.

rerobika · 2018-10-17T09:19:45Z

jerry-port/default/default-date.c

-  return false;
-} /* jerry_port_get_time_zone */
+  return ((double) tm.tm_gmtoff) * 1000;
+#else


Please add a comment here as well.

#else /* !HAVE_TM_GMTOFF */

rerobika · 2018-10-17T13:48:37Z

tests/jerry/date-setters.js

-//assert (d.getMinutes() == 1);
-//assert (d.getSeconds() == 1);
-//assert (d.getMilliseconds() == 1);
+d.setTime(0);


There are several FIXME:s in date-getters.js as well due to the timezone adjustment. Could you check them please?

Done, and they pass (with some changes, the original tests were slightly incorrect).

…timezones. The previous jerry_port interface did not allow timezones to be handled correctly, even if the host system was up to the task. This PR changes the jerry_port interface to allow a completely correct implementation to exist, and provides one (for Linux/BSD systems) in default-date.c. Fixes jerryscript-project#1661 JerryScript-DCO-1.0-Signed-off-by: crazy2be [email protected]

rerobika

LGTM (informal)

akosthekiss

LGTM. thanks for seeing it through.

crazy2be force-pushed the fix-jerry-port-timezones branch 3 times, most recently from b0821b5 to 76236da Compare September 26, 2018 17:02

crazy2be changed the title ~~Change jerry_port interface to allow for a correct implementation of timezones~~ Change jerry_port interface to allow for a correct implementation of timezones. Fixes #1661. Oct 1, 2018

rerobika requested changes Oct 2, 2018

View reviewed changes

akosthekiss requested changes Oct 3, 2018

View reviewed changes

crazy2be force-pushed the fix-jerry-port-timezones branch 2 times, most recently from 36b71f4 to b81a97f Compare October 3, 2018 20:33

akosthekiss changed the title ~~Change jerry_port interface to allow for a correct implementation of timezones. Fixes #1661.~~ Change jerry_port interface to allow for a correct implementation of timezones. Oct 3, 2018

akosthekiss reviewed Oct 4, 2018

View reviewed changes

akosthekiss added api Related to the public API jerry-port Related to the port API or the default port implementation labels Oct 4, 2018

crazy2be mentioned this pull request Oct 5, 2018

Ensure that tests are always executed in a proper time zone #2551

Merged

crazy2be force-pushed the fix-jerry-port-timezones branch 5 times, most recently from 96fa494 to 69726da Compare October 5, 2018 21:02

akosthekiss requested changes Oct 5, 2018

View reviewed changes

akosthekiss reviewed Oct 5, 2018

View reviewed changes

crazy2be force-pushed the fix-jerry-port-timezones branch 2 times, most recently from 2c97ff7 to a90db7b Compare October 9, 2018 17:22

zherczeg reviewed Oct 10, 2018

View reviewed changes

crazy2be force-pushed the fix-jerry-port-timezones branch from a90db7b to 09d6e76 Compare October 12, 2018 18:25

akosthekiss reviewed Oct 13, 2018

View reviewed changes

docs/05.PORT-API.md Show resolved Hide resolved

crazy2be force-pushed the fix-jerry-port-timezones branch 4 times, most recently from 01da250 to 49defba Compare October 16, 2018 19:37

zherczeg approved these changes Oct 17, 2018

View reviewed changes

rerobika requested changes Oct 17, 2018

View reviewed changes

crazy2be force-pushed the fix-jerry-port-timezones branch from 49defba to 645f2c0 Compare October 17, 2018 15:14

rerobika approved these changes Oct 17, 2018

View reviewed changes

akosthekiss approved these changes Oct 17, 2018

View reviewed changes

akosthekiss merged commit 3afc4b0 into jerryscript-project:master Oct 17, 2018

LaszloLango added this to the Release 2.0 milestone Oct 18, 2018

LaszloLango mentioned this pull request Oct 18, 2018

Migration Guide #2419

Merged

zherczeg mentioned this pull request Mar 19, 2019

Nominating Robert Fancsik (@rerobika) to become a JerryScript reviewer #2799

Closed

Change jerry_port interface to allow for a correct implementation of timezones. #2540

Change jerry_port interface to allow for a correct implementation of timezones. #2540

Conversation

crazy2be commented Sep 25, 2018 • edited Loading

crazy2be commented Oct 1, 2018

zherczeg commented Oct 2, 2018

LaszloLango commented Oct 2, 2018

rerobika left a comment

Choose a reason for hiding this comment

crazy2be commented Oct 2, 2018

akosthekiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosthekiss commented Oct 4, 2018

akosthekiss commented Oct 4, 2018

akosthekiss left a comment

Choose a reason for hiding this comment

akosthekiss Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosthekiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosthekiss commented Oct 9, 2018

crazy2be commented Oct 9, 2018

crazy2be commented Oct 10, 2018

zherczeg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazy2be commented Oct 12, 2018

akosthekiss left a comment

Choose a reason for hiding this comment

crazy2be commented Oct 16, 2018

zherczeg left a comment

Choose a reason for hiding this comment

rerobika left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rerobika left a comment

crazy2be commented Sep 25, 2018 •

edited

Loading

akosthekiss Oct 4, 2018 •

edited

Loading