[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1560: MINIFICPP-2101 Compilation fix in PutSFTPTests (libc++)
martinzink commented on code in PR #1560:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1560#discussion_r1188203069
##
libminifi/test/unit/FileUtilsTests.cpp:
##
@@ -516,3 +516,38 @@ TEST_CASE("FileUtils::path_size", "[TestPathSize]") {
REQUIRE(FileUtils::path_size(dir) == 12);
}
+
+TEST_CASE("file_clock to system_clock conversion tests") {
+ using namespace std::chrono;
+
+ static_assert(system_clock::period::num == file_clock::period::num);
+ constexpr auto lowest_den = std::min(file_clock::period::den,
system_clock::period::den);
+ using LeastPreciseDurationType =
duration, std::ratio>;
+
+ {
+system_clock::time_point system_now = system_clock::now();
+file_clock::time_point converted_system_now =
FileUtils::from_sys(system_now);
+system_clock::time_point double_converted_system_now =
FileUtils::to_sys(converted_system_now);
+
+
CHECK(time_point_cast(system_now).time_since_epoch().count()
==
time_point_cast(double_converted_system_now).time_since_epoch().count());
+ }
+
+ {
+file_clock::time_point file_now = file_clock ::now();
+system_clock::time_point converted_file_now = FileUtils::to_sys(file_now);
+file_clock::time_point double_converted_file_now =
FileUtils::from_sys(converted_file_now);
+
+
CHECK(time_point_cast(file_now).time_since_epoch().count()
==
time_point_cast(double_converted_file_now).time_since_epoch().count());
+ }
+
+ {
+system_clock::time_point system_now = system_clock::now();
+file_clock::time_point file_now = file_clock ::now();
+
+file_clock::time_point converted_system_now =
FileUtils::from_sys(system_now);
+system_clock::time_point converted_file_now = FileUtils::to_sys(file_now);
+
+CHECK(system_now-converted_file_now < 10ms);
Review Comment:
You are right, thanks. I've refactored it a bit so its more clear which one
is earlier in
https://github.com/apache/nifi-minifi-cpp/pull/1560/commits/8139796ee615269c86d693942e6fc3b87d16a3bc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1560: MINIFICPP-2101 Compilation fix in PutSFTPTests (libc++)
martinzink commented on code in PR #1560:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1560#discussion_r1185826659
##
libminifi/src/utils/file/FileUtils.cpp:
##
@@ -66,21 +66,21 @@ bool contains(const std::filesystem::path& file_path,
std::string_view text_to_s
return std::search(view.begin(), view.end(), searcher) != view.end();
}
-time_t to_time_t(std::filesystem::file_time_type file_time) {
-#if defined(_LIBCPP_VERSION) && _LIBCPP_VERSION < 14000
- return std::chrono::file_clock::to_time_t(file_time);
+std::chrono::system_clock::time_point to_sys(std::filesystem::file_time_type
file_time) {
+ using namespace std::chrono; // NOLINT(build/namespaces)
+#if defined(WIN32) || (defined(_LIBCPP_VERSION) && (_LIBCPP_VERSION < 14000))
+ return system_clock::now() + duration_cast(file_time
- file_clock::now());
Review Comment:
To support older windows it seems we would have to use the double now method
or resort to hardcoded magic numbers like the file epoch is 1601.01.01. Whats
your opinion?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1560: MINIFICPP-2101 Compilation fix in PutSFTPTests (libc++)
martinzink commented on code in PR #1560:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1560#discussion_r1185767400
##
libminifi/src/utils/file/FileUtils.cpp:
##
@@ -66,21 +66,21 @@ bool contains(const std::filesystem::path& file_path,
std::string_view text_to_s
return std::search(view.begin(), view.end(), searcher) != view.end();
}
-time_t to_time_t(std::filesystem::file_time_type file_time) {
-#if defined(_LIBCPP_VERSION) && _LIBCPP_VERSION < 14000
- return std::chrono::file_clock::to_time_t(file_time);
+std::chrono::system_clock::time_point to_sys(std::filesystem::file_time_type
file_time) {
+ using namespace std::chrono; // NOLINT(build/namespaces)
+#if defined(WIN32) || (defined(_LIBCPP_VERSION) && (_LIBCPP_VERSION < 14000))
+ return system_clock::now() + duration_cast(file_time
- file_clock::now());
Review Comment:
More info on the windows issue: https://github.com/microsoft/STL/issues/2446
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1560: MINIFICPP-2101 Compilation fix in PutSFTPTests (libc++)
martinzink commented on code in PR #1560:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1560#discussion_r1185765588
##
libminifi/src/utils/file/FileUtils.cpp:
##
@@ -66,21 +66,21 @@ bool contains(const std::filesystem::path& file_path,
std::string_view text_to_s
return std::search(view.begin(), view.end(), searcher) != view.end();
}
-time_t to_time_t(std::filesystem::file_time_type file_time) {
-#if defined(_LIBCPP_VERSION) && _LIBCPP_VERSION < 14000
- return std::chrono::file_clock::to_time_t(file_time);
+std::chrono::system_clock::time_point to_sys(std::filesystem::file_time_type
file_time) {
+ using namespace std::chrono; // NOLINT(build/namespaces)
+#if defined(WIN32) || (defined(_LIBCPP_VERSION) && (_LIBCPP_VERSION < 14000))
+ return system_clock::now() + duration_cast(file_time
- file_clock::now());
Review Comment:
It seems this method produces the same error which I encountered when I was
using clock_cast (clock cast probably does this under the hood) (it loads
timezonedb which based on the comments fails on older windowses
https://github.com/microsoft/STL/issues/1911)
Also the macOS CI does seem to use libc++<14 (altough I might be able to
update that)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1560: MINIFICPP-2101 Compilation fix in PutSFTPTests (libc++)
martinzink commented on code in PR #1560:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1560#discussion_r1172250693
##
extensions/standard-processors/tests/unit/ListFileTests.cpp:
##
@@ -107,8 +104,12 @@ TEST_CASE_METHOD(ListFileTestFixture, "Input Directory is
empty", "[testListFile
REQUIRE_THROWS_AS(test_controller_.runSession(plan_, true),
minifi::Exception);
}
+std::string get_last_modified_time_formatted_string(const
std::filesystem::path& path) {
+ return date::format("%Y-%m-%dT%H:%M:%SZ",
std::chrono::time_point_cast(utils::file::to_sys(*utils::file::last_write_time(path;
Review Comment:
you are right, I've changed it in
https://github.com/apache/nifi-minifi-cpp/pull/1560/commits/41f3dd4ef1416a1bf5ab38d7052e08678adc2d88
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1560: MINIFICPP-2101 Compilation fix in PutSFTPTests (libc++)
martinzink commented on code in PR #1560:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1560#discussion_r1172250412
##
libminifi/test/unit/FileUtilsTests.cpp:
##
@@ -477,3 +477,27 @@ TEST_CASE("FileUtils::get_relative_path",
"[TestGetRelativePath]") {
REQUIRE(*FileUtils::get_relative_path(path, base_path / "") ==
std::filesystem::path("subdir") / "file.log");
REQUIRE(*FileUtils::get_relative_path(base_path, base_path) == ".");
}
+
+TEST_CASE("file_clock to system_clock conversion tests") {
+ using namespace std::chrono;
+
+ static_assert(system_clock::duration::period::num ==
file_clock::duration::period::num);
+ constexpr auto lowest_den = system_clock::period::den <
file_clock::period::den ? system_clock::period::den : file_clock::period::den;
+ using LeastPreciseDurationType =
duration, std::ratio>;
Review Comment:
good idea, changed it in
https://github.com/apache/nifi-minifi-cpp/pull/1560/commits/41f3dd4ef1416a1bf5ab38d7052e08678adc2d88
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1560: MINIFICPP-2101 Compilation fix in PutSFTPTests (libc++)
martinzink commented on code in PR #1560:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1560#discussion_r1172250106
##
libminifi/test/unit/FileUtilsTests.cpp:
##
@@ -477,3 +477,27 @@ TEST_CASE("FileUtils::get_relative_path",
"[TestGetRelativePath]") {
REQUIRE(*FileUtils::get_relative_path(path, base_path / "") ==
std::filesystem::path("subdir") / "file.log");
REQUIRE(*FileUtils::get_relative_path(base_path, base_path) == ".");
}
+
+TEST_CASE("file_clock to system_clock conversion tests") {
+ using namespace std::chrono;
+
+ static_assert(system_clock::duration::period::num ==
file_clock::duration::period::num);
+ constexpr auto lowest_den = system_clock::period::den <
file_clock::period::den ? system_clock::period::den : file_clock::period::den;
Review Comment:
sure thing, good idea, I've changed it in
https://github.com/apache/nifi-minifi-cpp/pull/1560/commits/41f3dd4ef1416a1bf5ab38d7052e08678adc2d88
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1560: MINIFICPP-2101 Compilation fix in PutSFTPTests (libc++)
martinzink commented on code in PR #1560:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1560#discussion_r1171319182
##
libminifi/src/utils/file/FileUtils.cpp:
##
@@ -74,13 +74,23 @@ time_t to_time_t(std::filesystem::file_time_type file_time)
{
#endif
}
-std::chrono::time_point
to_sys(std::filesystem::file_time_type file_time) {
-#if defined(WIN32) || defined(_LIBCPP_VERSION) && _LIBCPP_VERSION >= 14000
- return
std::chrono::time_point_cast(file_time -
std::filesystem::file_time_type::clock::now() +
std::chrono::system_clock::now());
-#elif defined(_LIBCPP_VERSION)
+std::chrono::system_clock::time_point to_sys(std::filesystem::file_time_type
file_time) {
+#if defined(WIN32)
+ return std::chrono::clock_cast(file_time);
+#elif defined(_LIBCPP_VERSION) && (_LIBCPP_VERSION < 14000)
return
std::chrono::system_clock::from_time_t(std::chrono::file_clock::to_time_t(file_time));
#else
- return std::chrono::file_clock::to_sys(file_time);
+ return
std::chrono::time_point_cast(std::chrono::file_clock::to_sys(file_time));
+#endif
+}
+
+std::filesystem::file_time_type from_sys(std::chrono::system_clock::time_point
sys_time) {
+#if defined(WIN32)
+ return std::chrono::clock_cast(sys_time);
+#elif defined(_LIBCPP_VERSION) && (_LIBCPP_VERSION < 14000)
+ return
std::chrono::file_clock::from_time_t(std::chrono::system_clock::to_time_t(sys_time));
+#else
+ return std::chrono::file_clock::from_sys(sys_time);
Review Comment:
It turns out we can't rely on clock_cast on windows either, because (for
some reason it needs timezone information which should be available after msi
install but not neccesarly there during tests of course we could load it like
CronTests do, but I think we can skip the hassle since the other platforms dont
have clock_cast)
Anyways, I've refactored these in
https://github.com/apache/nifi-minifi-cpp/pull/1560/commits/d8e7f065500dc282786918880057f7a23ca0b942
I've also removed the to_time_t functions from FileUtils since it was only
used in a single test (ListFileTests)
https://github.com/apache/nifi-minifi-cpp/pull/1560/commits/256d695e88751dbf5519546f53eebb72e4f3776f
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1560: MINIFICPP-2101 Compilation fix in PutSFTPTests (libc++)
martinzink commented on code in PR #1560: URL: https://github.com/apache/nifi-minifi-cpp/pull/1560#discussion_r1171317303 ## libminifi/src/utils/file/FileUtils.cpp: ## Review Comment: I've added some unit tests in https://github.com/apache/nifi-minifi-cpp/pull/1560/commits/d8e7f065500dc282786918880057f7a23ca0b942 I've only added back and forth tests since we can't parse file_clock on every platform and the file_clock epoch is unspecified, so its seems impossible to specify the same time on system_clock and file_clock independently. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1560: MINIFICPP-2101 Compilation fix in PutSFTPTests (libc++)
martinzink commented on code in PR #1560:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1560#discussion_r1169706602
##
libminifi/src/utils/file/FileUtils.cpp:
##
@@ -74,13 +74,23 @@ time_t to_time_t(std::filesystem::file_time_type file_time)
{
#endif
}
-std::chrono::time_point
to_sys(std::filesystem::file_time_type file_time) {
-#if defined(WIN32) || defined(_LIBCPP_VERSION) && _LIBCPP_VERSION >= 14000
- return
std::chrono::time_point_cast(file_time -
std::filesystem::file_time_type::clock::now() +
std::chrono::system_clock::now());
-#elif defined(_LIBCPP_VERSION)
+std::chrono::system_clock::time_point to_sys(std::filesystem::file_time_type
file_time) {
+#if defined(WIN32)
+ return std::chrono::clock_cast(file_time);
+#elif defined(_LIBCPP_VERSION) && (_LIBCPP_VERSION < 14000)
return
std::chrono::system_clock::from_time_t(std::chrono::file_clock::to_time_t(file_time));
#else
- return std::chrono::file_clock::to_sys(file_time);
+ return
std::chrono::time_point_cast(std::chrono::file_clock::to_sys(file_time));
+#endif
+}
+
+std::filesystem::file_time_type from_sys(std::chrono::system_clock::time_point
sys_time) {
+#if defined(WIN32)
+ return std::chrono::clock_cast(sys_time);
+#elif defined(_LIBCPP_VERSION) && (_LIBCPP_VERSION < 14000)
+ return
std::chrono::file_clock::from_time_t(std::chrono::system_clock::to_time_t(sys_time));
+#else
+ return std::chrono::file_clock::from_sys(sys_time);
Review Comment:
You only need to cast it where it can lose precision otherwise the cast is
implicit.
So from hours to minutes you dont need duration_cast while you would need it
in the other direction.
Of course we could add the explicit cast to other function aswell, if we
expect that on some platfrom system_clock will be more precise than file_clock
(or simply to improve readability) What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1560: MINIFICPP-2101 Compilation fix in PutSFTPTests (libc++)
martinzink commented on code in PR #1560:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1560#discussion_r1169706602
##
libminifi/src/utils/file/FileUtils.cpp:
##
@@ -74,13 +74,23 @@ time_t to_time_t(std::filesystem::file_time_type file_time)
{
#endif
}
-std::chrono::time_point
to_sys(std::filesystem::file_time_type file_time) {
-#if defined(WIN32) || defined(_LIBCPP_VERSION) && _LIBCPP_VERSION >= 14000
- return
std::chrono::time_point_cast(file_time -
std::filesystem::file_time_type::clock::now() +
std::chrono::system_clock::now());
-#elif defined(_LIBCPP_VERSION)
+std::chrono::system_clock::time_point to_sys(std::filesystem::file_time_type
file_time) {
+#if defined(WIN32)
+ return std::chrono::clock_cast(file_time);
+#elif defined(_LIBCPP_VERSION) && (_LIBCPP_VERSION < 14000)
return
std::chrono::system_clock::from_time_t(std::chrono::file_clock::to_time_t(file_time));
#else
- return std::chrono::file_clock::to_sys(file_time);
+ return
std::chrono::time_point_cast(std::chrono::file_clock::to_sys(file_time));
+#endif
+}
+
+std::filesystem::file_time_type from_sys(std::chrono::system_clock::time_point
sys_time) {
+#if defined(WIN32)
+ return std::chrono::clock_cast(sys_time);
+#elif defined(_LIBCPP_VERSION) && (_LIBCPP_VERSION < 14000)
+ return
std::chrono::file_clock::from_time_t(std::chrono::system_clock::to_time_t(sys_time));
+#else
+ return std::chrono::file_clock::from_sys(sys_time);
Review Comment:
You only need to cast it where it can lose precision otherwise the cast is
implicit.
So from hours to minutes you dont need duration_cast while you would need in
the other direction.
Of course we could add the explicit cast to other function aswell, if we
expect that on some platfrom system_clock will be more precise than file_clock
(or simply to improve readability) What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1560: MINIFICPP-2101 Compilation fix in PutSFTPTests (libc++)
martinzink commented on code in PR #1560:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1560#discussion_r1169655064
##
libminifi/src/utils/file/FileUtils.cpp:
##
@@ -74,13 +74,23 @@ time_t to_time_t(std::filesystem::file_time_type file_time)
{
#endif
}
-std::chrono::time_point
to_sys(std::filesystem::file_time_type file_time) {
-#if defined(WIN32) || defined(_LIBCPP_VERSION) && _LIBCPP_VERSION >= 14000
- return
std::chrono::time_point_cast(file_time -
std::filesystem::file_time_type::clock::now() +
std::chrono::system_clock::now());
-#elif defined(_LIBCPP_VERSION)
+std::chrono::system_clock::time_point to_sys(std::filesystem::file_time_type
file_time) {
+#if defined(WIN32)
+ return std::chrono::clock_cast(file_time);
+#elif defined(_LIBCPP_VERSION) && (_LIBCPP_VERSION < 14000)
return
std::chrono::system_clock::from_time_t(std::chrono::file_clock::to_time_t(file_time));
#else
- return std::chrono::file_clock::to_sys(file_time);
+ return
std::chrono::time_point_cast(std::chrono::file_clock::to_sys(file_time));
+#endif
+}
+
+std::filesystem::file_time_type from_sys(std::chrono::system_clock::time_point
sys_time) {
+#if defined(WIN32)
+ return std::chrono::clock_cast(sys_time);
+#elif defined(_LIBCPP_VERSION) && (_LIBCPP_VERSION < 14000)
+ return
std::chrono::file_clock::from_time_t(std::chrono::system_clock::to_time_t(sys_time));
+#else
+ return std::chrono::file_clock::from_sys(sys_time);
Review Comment:
On libc++ (if the _LIBCPP_HAS_NO_INT128 is not defined) file_clock uses a
128 bit resolution while the system_clock only uses 64 bit, due to this we only
need the cast in from_sys.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
