[Tarantool-patches] [PATCH v5 1/8] build: add Christian Hansen c-dt to the build
Serge Petrenko
sergepetrenko at tarantool.org
Tue Aug 17 15:15:17 MSK 2021
16.08.2021 02:59, Timur Safin via Tarantool-patches пишет:
> * Integrated chansen/c-dt parser as 3rd party module to the
> Tarantool cmake build process;
> * Points to tarantool/c-dt instead of original chansen/c-dt to
> have easier build integration, because there are additional
> commits which have integrated cmake support and have established
> symbols renaming facilities (similar to those we see in xxhash
> or icu).
> We have to be able to rename externally public symbols to avoid
> name clashes with 3rd party modules. We prefix c-dt symbols
> in the Tarantool build with `tnt_` prefix;
> * took special care that generated build artifacts not override
> in-source files, but use different build/ directory.
>
> * added datetime parsing unit tests, for literals - with and
> without trailing timezones;
> * also we check that strftime is reversible and produce consistent
> results after roundtrip from/to strings;
> * discovered the harder way that on *BSD/MacOSX `strftime()` format
> `%z` outputs local time-zone if passed `tm_gmtoff` is 0.
> This behaviour is different to that we observed on Linux, thus we
> might have different execution results. Made test to not use `%z`
> and only operate with normalized date time formats `%F` and `%T`
>
> Part of #5941
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
> ---
> .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
> +#
> +
> +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.
<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.
*/
> +#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.
> + 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.
> + 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
More information about the Tarantool-patches
mailing list