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>]

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