Re: [Tarantool-patches] [PATCH v5 1/8] build: add Christian Hansen c-dt to the build

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

18.08.2021 02:24, Safin Timur пишет:
> Thanks, for quick review, please see my notes below...
>
> On 17.08.2021 15:15, Serge Petrenko wrote:
>>
>> Hi! Thanks for the patch!
>>
>> Please, find a couple of style comments below.
>>
>> Also I think you may squash all the commits regarding c-dt cmake 
>> integration
>> into one. And push all the commits from your branch to tarantool/c-dt 
>> master.
>> It's not good that they live on a separate branch
>
> I wanted to keep original Christian Hansen' master intact, that's why 
> I've pushed all our changes to the separate branch.  Agreed though 
> they make no much sense separately, and squashed them together. Also 
> renamed branch `cmake` to `tarantool-master` and has made it's default 
> in repository.

Yep, that's fine.

>
>>
>>> ---
>>>   .gitmodules               |   3 +
>>>   CMakeLists.txt            |   8 +
>>>   cmake/BuildCDT.cmake      |   8 +
>>>   src/CMakeLists.txt        |   3 +-
>>>   test/unit/CMakeLists.txt  |   3 +-
>>>   test/unit/datetime.c      | 223 ++++++++++++++++++++++++
>>>   test/unit/datetime.result | 358 
>>> ++++++++++++++++++++++++++++++++++++++
>>>   third_party/c-dt          |   1 +
>>>   8 files changed, 605 insertions(+), 2 deletions(-)
>>>   create mode 100644 cmake/BuildCDT.cmake
>>>   create mode 100644 test/unit/datetime.c
>>>   create mode 100644 test/unit/datetime.result
>>>   create mode 160000 third_party/c-dt
>>>
>>> diff --git a/.gitmodules b/.gitmodules
>>> index f2f91ee72..aa3fbae4e 100644
>>> --- a/.gitmodules
>>> +++ b/.gitmodules
>>> @@ -43,3 +43,6 @@
>>>   [submodule "third_party/xxHash"]
>>>       path = third_party/xxHash
>>>       url =https://github.com/tarantool/xxHash
>>> +[submodule "third_party/c-dt"]
>>> +    path = third_party/c-dt
>>> +    url =https://github.com/tarantool/c-dt.git
>>> diff --git a/CMakeLists.txt b/CMakeLists.txt
>>> index e25b81eac..53c86f2a5 100644
>>> --- a/CMakeLists.txt
>>> +++ b/CMakeLists.txt
>>> @@ -571,6 +571,14 @@ endif()
>>>   # zstd
>>>   #
>>> +#
>>> +# Chritian Hanson c-dt
>
> BTW, I've spelled his name wrongly. Fixed and push-forced updated branch.
>
>>> +#
>>> +
>>> +include(BuildCDT)
>>> +libccdt_build()
>>> +add_dependencies(build_bundled_libs cdt)
>>> +
>>>   #
>>>   # Third-Party misc
>>>   #
>>> diff --git a/cmake/BuildCDT.cmake b/cmake/BuildCDT.cmake
>>> new file mode 100644
>>> index 000000000..343fb1b99
>>> --- /dev/null
>>> +++ b/cmake/BuildCDT.cmake
>>> @@ -0,0 +1,8 @@
>>> +macro(libccdt_build)
>>> +    set(LIBCDT_INCLUDE_DIRS ${PROJECT_SOURCE_DIR}/third_party/c-dt/)
>>> +    set(LIBCDT_LIBRARIES cdt)
>>> +
>>> +    file(MAKE_DIRECTORY 
>>> ${CMAKE_CURRENT_BINARY_DIR}/third_party/c-dt/build/)
>>> +    add_subdirectory(${PROJECT_SOURCE_DIR}/third_party/c-dt
>>> + ${CMAKE_CURRENT_BINARY_DIR}/third_party/c-dt/build/)
>>> +endmacro()
>>> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
>>> index adb03b3f4..97b0cb326 100644
>>> --- a/src/CMakeLists.txt
>>> +++ b/src/CMakeLists.txt
>>> @@ -193,7 +193,8 @@ target_link_libraries(server core coll 
>>> http_parser bit uri uuid swim swim_udp
>>>   # Rule of thumb: if exporting a symbol from a static library, list 
>>> the
>>>   # library here.
>>>   set (reexport_libraries server core misc bitset csv swim swim_udp 
>>> swim_ev
>>> -     shutdown ${LUAJIT_LIBRARIES} ${MSGPUCK_LIBRARIES} 
>>> ${ICU_LIBRARIES} ${CURL_LIBRARIES} ${XXHASH_LIBRARIES})
>>> +     shutdown ${LUAJIT_LIBRARIES} ${MSGPUCK_LIBRARIES} 
>>> ${ICU_LIBRARIES}
>>> +     ${CURL_LIBRARIES} ${XXHASH_LIBRARIES} ${LIBCDT_LIBRARIES})
>>>   set (common_libraries
>>>       ${reexport_libraries}
>>> diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
>>> index 5bb7cd6e7..31b183a8f 100644
>>> --- a/test/unit/CMakeLists.txt
>>> +++ b/test/unit/CMakeLists.txt
>>> @@ -56,7 +56,8 @@ add_executable(uuid.test uuid.c core_test_utils.c)
>>>   target_link_libraries(uuid.test uuid unit)
>>>   add_executable(random.test random.c core_test_utils.c)
>>>   target_link_libraries(random.test core unit)
>>> -
>>> +add_executable(datetime.test datetime.c)
>>> +target_link_libraries(datetime.test cdt unit)
>>>   add_executable(bps_tree.test bps_tree.cc)
>>>   target_link_libraries(bps_tree.test small misc)
>>>   add_executable(bps_tree_iterator.test bps_tree_iterator.cc)
>>> diff --git a/test/unit/datetime.c b/test/unit/datetime.c
>>> new file mode 100644
>>> index 000000000..64c19dac4
>>> --- /dev/null
>>> +++ b/test/unit/datetime.c
>>
>> I see that you only test datetime parsing in this test.
>> Not the datetime module itself.
>> Maybe worth renaming the test to datetime_parse, or c-dt,
>> or any other name you find suitable?
>>
>> P.S. never mind, I see more tests are added to this file later on.
>
> Yes.
>
>>
>>
>> <stripped>
>>> +
>>> +/* avoid introducing external datetime.h dependency -
>>> +   just copy paste it for today
>>> +*/
>>
>> Please, fix comment formatting:
>>
>> /* Something you have to say. */
>>
>> /*
>>   * Something you have to say
>>   * spanning a couple of lines.
>>   */
>>
>
> I've deleted this block of code with comment, after I've introduced 
> datetime.h. So I'd rather delete this comment.
>
>
>>> +#define SECS_PER_DAY      86400
>>> +#define DT_EPOCH_1970_OFFSET 719163
>>> +
>>> +struct datetime {
>>> +    double secs;
>>> +    int32_t nsec;
>>> +    int32_t offset;
>>> +};
>>> +
>>> +static int
>>> +local_rd(const struct datetime *dt)
>>> +{
>>> +    return (int)((int64_t)dt->secs / SECS_PER_DAY) + 
>>> DT_EPOCH_1970_OFFSET;
>>> +}
>>> +
>>> +static int
>>> +local_dt(const struct datetime *dt)
>>> +{
>>> +    return dt_from_rdn(local_rd(dt));
>>> +}
>>> +
>>> +struct tm *
>>> +datetime_to_tm(struct datetime *dt)
>>> +{
>>> +    static struct tm tm;
>>> +
>>> +    memset(&tm, 0, sizeof(tm));
>>> +    dt_to_struct_tm(local_dt(dt), &tm);
>>> +
>>> +    int seconds_of_day = (int64_t)dt->secs % 86400;
>>> +    tm.tm_hour = (seconds_of_day / 3600) % 24;
>>> +    tm.tm_min = (seconds_of_day / 60) % 60;
>>> +    tm.tm_sec = seconds_of_day % 60;
>>> +
>>> +    return &tm;
>>> +}
>>> +
>>> +static void datetime_test(void)
>>> +{
>>> +    size_t index;
>>> +    int64_t secs_expected;
>>> +    int32_t nanosecs;
>>> +    int32_t ofs;
>>> +
>>> +    plan(355);
>>> +    parse_datetime(sample, sizeof(sample) - 1,
>>> +               &secs_expected, &nanosecs, &ofs);
>>> +
>>> +    for (index = 0; index < DIM(tests); index++) {
>>> +        int64_t secs;
>>> +        int rc = parse_datetime(tests[index].sz, tests[index].len,
>>> +                        &secs, &nanosecs, &ofs);
>>
>>
>> Please, fix argument alignment here.
>
> Fixed.
>
>>
>>
>>> +        is(rc, 0, "correct parse_datetime return value for '%s'",
>>> +           tests[index].sz);
>>> +        is(secs, secs_expected, "correct parse_datetime output "
>>> +           "seconds for '%s", tests[index].sz);
>>> +
>>> +        /* check that stringized literal produces the same date */
>>> +        /* time fields */
>>
>>
>> Same as above, please fix comment formatting.
>>
>
> Ditto.
>
>>
>>> +        static char buff[40];
>>> +        struct datetime dt = {secs, nanosecs, ofs};
>>> +        /* datetime_to_tm returns time in GMT zone */
>>> +        struct tm * p_tm = datetime_to_tm(&dt);
>>> +        size_t len = strftime(buff, sizeof buff, "%F %T", p_tm);
>>> +        ok(len > 0, "strftime");
>>> +        int64_t parsed_secs;
>>> +        int32_t parsed_nsecs, parsed_ofs;
>>> +        rc = parse_datetime(buff, len, &parsed_secs, &parsed_nsecs, 
>>> &parsed_ofs);
>>> +        is(rc, 0, "correct parse_datetime return value for '%s'", 
>>> buff);
>>> +        is(secs, parsed_secs,
>>> +           "reversible seconds via strftime for '%s", buff);
>>> +    }
>>> +}
>>> +
>>> +int
>>> +main(void)
>>> +{
>>> +    plan(1);
>>> +    datetime_test();
>>> +
>>> +    return check_plan();
>>> +}
>>> diff --git a/test/unit/datetime.result b/test/unit/datetime.result
>>> new file mode 100644
>>> index 000000000..33997d9df
>>> --- /dev/null
>>> +++ b/test/unit/datetime.result
>>
>> <stripped>
>>
>>> diff --git a/third_party/c-dt b/third_party/c-dt
>>> new file mode 160000
>>> index 000000000..5b1398ca8
>>> --- /dev/null
>>> +++ b/third_party/c-dt
>>> @@ -0,0 +1 @@
>>> +Subproject commit 5b1398ca8f53513985670a2e832b5f6e253addf7
>>> -- 
>>> Serge Petrenko
>>
>
> Here is incremental update for this patch so far (applied to branch, 
> but not yet pushed - to accumulate all changes after Vova feedback):
> --------------------------------------------------------------------
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 53c86f2a5..8037c30a7 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -572,7 +572,7 @@ endif()
>  #
>
>  #
> -# Chritian Hanson c-dt
> +# Christian Hansen c-dt
>  #
>
>  include(BuildCDT)
> diff --git a/test/unit/datetime.c b/test/unit/datetime.c
> index 64c19dac4..827930fc4 100644
> --- a/test/unit/datetime.c
> +++ b/test/unit/datetime.c
> @@ -136,16 +136,13 @@ exit:
>         return 0;
>  }
>
> -/* avoid introducing external datetime.h dependency -
> -   just copy paste it for today
> -*/
> -#define SECS_PER_DAY      86400
> +#define SECS_PER_DAY         86400
>  #define DT_EPOCH_1970_OFFSET 719163
>
>  struct datetime {
> -       double secs;
> -       int32_t nsec;
> -       int32_t offset;
> +       double   secs;
> +       uint32_t nsec;
> +       int32_t  offset;
>  };
>
>  static int
> @@ -190,14 +187,16 @@ static void datetime_test(void)
>         for (index = 0; index < DIM(tests); index++) {
>                 int64_t secs;
>                 int rc = parse_datetime(tests[index].sz, 
> tests[index].len,
> -                                               &secs, &nanosecs, &ofs);
> +                                       &secs, &nanosecs, &ofs);
>                 is(rc, 0, "correct parse_datetime return value for '%s'",
>                    tests[index].sz);
>                 is(secs, secs_expected, "correct parse_datetime output "
>                    "seconds for '%s", tests[index].sz);
>
> -               /* check that stringized literal produces the same 
> date */
> -               /* time fields */
> +               /*
> +                * check that stringized literal produces the same date
> +                * time fields
> +                */
>                 static char buff[40];
>                 struct datetime dt = {secs, nanosecs, ofs};
>                 /* datetime_to_tm returns time in GMT zone */
> diff --git a/third_party/c-dt b/third_party/c-dt
> index 5b1398ca8..3cbbbc7f0 160000
> --- a/third_party/c-dt
> +++ b/third_party/c-dt
> @@ -1 +1 @@
> -Subproject commit 5b1398ca8f53513985670a2e832b5f6e253addf7
> +Subproject commit 3cbbbc7f032cfa67a8df9f81101403249825d7f3
> --------------------------------------------------------------------
>
> Thanks,
> Timur

-- 
Serge Petrenko