unix/modtime: Add mktime function. #5333
Conversation
|
I missed those issues. The argument seems to be that I've got existing stm32 code that uses mktime to convert between tuple based time and seconds based timestamps. Perhaps when the previous arguments were made unix didn't have |
Hmm, I don't think so, |
|
Sure, I don't really follow the counter-argument then, not sure what the issue was. |
ports/unix/modtime.c
Outdated
| @@ -161,6 +162,29 @@ STATIC mp_obj_t mod_time_localtime(size_t n_args, const mp_obj_t *args) { | |||
| } | |||
| STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(mod_time_localtime_obj, 0, 1, mod_time_localtime); | |||
|
|
|||
| /// \function mktime() | |||
There was a problem hiding this comment.
Minor, but probably better to use the same style as the rest of the code: no need to repeat function name, just 2 //. Likewise for the code, I don't feel it needs all those newlines or at least not the first one in the function body.
There was a problem hiding this comment.
Valid point, I updated the function name to match this files style but didn't pay attention to any of the other styling.
There was a problem hiding this comment.
This commenting style is really obsolete. I intend to delete it all from stm32 at some point...
There was a problem hiding this comment.
I think I've cleaned up the styling to match the rest of the file
|
I'm +1 to including this, main reason that if localtime is available, mktime should be as well I guess. However it feels a bit silly to just copy the function once more, couldn't all ports providing this use the same one? |
|
@stinos I tend to agree that consolidating the wrapper functionality here would be nice, though there's only some parts on this file that can easily be done, others are certainly port specific. The bulk of the code is already consolidated in |
|
Yeah maybe it's over-engineering in this case, but it's just that I looked up mktime throughout the entire code and saw that 'CPython uses 9 etc' logic 5 times or so, and I'm a fan of deduplication just because I've been screwed by duplication so many times in the past. But indeed I'm not sure if it's worth it here and it could be done separately. |
|
Hehe I'm all too often the same, but then I get tunnel vision and refactor everything as I go and end up with something like https://github.com/micropython/micropython/pull/5249/files with 37 files changed. Smaller self-contained changes are best, ideally with discussion leading to logical next steps. |
|
Ok, so there's probably a problem with this PR regarding epoch. In unix port, So for consistency, I probably need to update Alternatively and/or separately, we look at fixing epoch consistency across all the ports? |
Yes. It should be that
The uPy-specific 2000 epoch does lead to difficulties when using it, as people expect the 1970 epoch. But it's hard to solve this. Ideally all ports would use the 1970 epoch but I don't see how to do it. One option is to provide a constant like |
So I started adding a "seconds from (2000 - 1970)" offset in the unix port, it's still not really enough to satisfy this expectation: |
I see. So It's insightful to look at the result of So a |
|
Yeah true, |
|
Ok, new version using C mktime pushed. |
ports/windows/Makefile
Outdated
| @@ -29,6 +29,7 @@ endif | |||
| # source files | |||
| SRC_C = \ | |||
| lib/utils/printf.c \ | |||
| lib/timeutils/timeutils.c \ | |||
There was a problem hiding this comment.
Shouldn't need this anymore...?
ports/unix/modtime.c
Outdated
| // printf("sec: %d\n", (int)time.tm_sec); | ||
| // printf("wday: %d\n", (int)time.tm_wday); | ||
| // printf("yday: %d\n", (int)time.tm_yday); | ||
| // printf("isdst: %d\n", (int)time.tm_isdst); |
ports/unix/modtime.c
Outdated
| @@ -126,6 +130,8 @@ STATIC mp_obj_t mod_time_sleep(mp_obj_t arg) { | |||
| } | |||
| STATIC MP_DEFINE_CONST_FUN_OBJ_1(mod_time_sleep_obj, mod_time_sleep); | |||
|
|
|||
|
|
|||
|
|
|||
ports/unix/modtime.c
Outdated
| @@ -66,6 +67,9 @@ static inline int msec_sleep_tv(struct timeval *tv) { | |||
| #error Unsupported clock() implementation | |||
| #endif | |||
|
|
|||
| #define DAYS_PER_4Y (365*4 + 1) | |||
| #define SECONDS_1970_2000 ((DAYS_PER_4Y * (2000-1970) / 4) * 24 * 60 * 60) | |||
ports/unix/modtime.c
Outdated
|
|
||
| // localtime generates a tuple of len 8. CPython uses 9, so we accept both. | ||
| if (len < 8 || len > 9) { | ||
| nlr_raise(mp_obj_new_exception_msg_varg(&mp_type_TypeError, "mktime needs a tuple of length 8 or 9 (%d given)", len)); |
There was a problem hiding this comment.
Don't need to say how many items were given, just a simple error msg would be enough (using mp_raise_TypeError).
710b1c6 to
3de7362
Compare
|
Hmm, my test doesn't work on the unix nanbox build, not sure why. |
It looks like you ran into the Year 2038 problem: https://en.wikipedia.org/wiki/Year_2038_problem Nanbox builds are 32-bit, and |
|
Ah, that makes sense. It looks like cpython has quite a bit of extra parsing and error checking going on, some of which appears to be for this: |
|
Looks like OverflowError is the right thing to throw then. |
|
This will need to pass CI before it can be merged. |
|
Yes, there's the tests that break due to the date overflow issue and the exception being thrown. I haven't decided whether to try to handle that properly on ports where expected of just restrict the date rage to safe dates... |
Also applies to windows port.
|
I'm not sure why the AppVeyor buiild is failing, the log I looked at showed the build compiled fine and all the tests were either pass or skipped. |
I pushed this PR as a new PR in #5461 to try and fix this. |
Make `next_code_allocation` and `prev_traceback_allocation` movable
Add
utime.mktime()to match other ports.Function implementation was copied from stm32 version.