Re: [PR] MINIFICPP-2326 Link lua statically [nifi-minifi-cpp]
szaszm closed pull request #1756: MINIFICPP-2326 Link lua statically URL: https://github.com/apache/nifi-minifi-cpp/pull/1756 -- 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
Re: [PR] MINIFICPP-2326 Link lua statically [nifi-minifi-cpp]
szaszm commented on code in PR #1756:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1756#discussion_r1566008345
##
libminifi/test/unit/CronTests.cpp:
##
@@ -421,7 +421,7 @@ TEST_CASE("Cron::calculateNextTrigger with timezones",
"[cron]") {
date::set_install(TZ_DATA_DIR);
#endif
- const std::vector time_zones{ "Europe/Berlin", "Asia/Seoul",
"America/Los_Angeles", "Asia/Singapore", "UCT" };
+ const std::vector time_zones{ "Europe/Berlin", "Asia/Seoul",
"America/Los_Angeles", "Asia/Singapore", "UTC" };
Review Comment:
ok, then let's leave it changed :)
--
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
Re: [PR] MINIFICPP-2326 Link lua statically [nifi-minifi-cpp]
martinzink commented on code in PR #1756:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1756#discussion_r1565982904
##
libminifi/test/unit/CronTests.cpp:
##
@@ -421,7 +421,7 @@ TEST_CASE("Cron::calculateNextTrigger with timezones",
"[cron]") {
date::set_install(TZ_DATA_DIR);
#endif
- const std::vector time_zones{ "Europe/Berlin", "Asia/Seoul",
"America/Los_Angeles", "Asia/Singapore", "UCT" };
+ const std::vector time_zones{ "Europe/Berlin", "Asia/Seoul",
"America/Los_Angeles", "Asia/Singapore", "UTC" };
Review Comment:
I didnt have a strong opinion on this when I wrote it to ne honest didnt
know about this whole situation before this thread and now that I looked it up.
Im good with changing it
--
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
Re: [PR] MINIFICPP-2326 Link lua statically [nifi-minifi-cpp]
szaszm commented on code in PR #1756:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1756#discussion_r1565961703
##
libminifi/test/unit/CronTests.cpp:
##
@@ -421,7 +421,7 @@ TEST_CASE("Cron::calculateNextTrigger with timezones",
"[cron]") {
date::set_install(TZ_DATA_DIR);
#endif
- const std::vector time_zones{ "Europe/Berlin", "Asia/Seoul",
"America/Los_Angeles", "Asia/Singapore", "UCT" };
+ const std::vector time_zones{ "Europe/Berlin", "Asia/Seoul",
"America/Los_Angeles", "Asia/Singapore", "UTC" };
Review Comment:
If it was just a typo, I'm OK with dropping it. My distro is quite 'new', an
Arch Linux updated last week, and it still has "UCT" under
`/usr/share/zoneinfo` (and also under `Etc`), which is not a symlink, but
identical to "UTC". So I'm guessing it's a debian/ubuntu thing. Either way, the
test should work on those systems.
Updating the date library or moving to the C++20 would be nice, but I'd keep
it separate from this PR. If you're investigating, please share your results,
but I don't want to hold up this PR with the additional change, if it's not
necessary.
--
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
Re: [PR] MINIFICPP-2326 Link lua statically [nifi-minifi-cpp]
fgerlits commented on code in PR #1756:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1756#discussion_r1565902422
##
libminifi/test/unit/CronTests.cpp:
##
@@ -421,7 +421,7 @@ TEST_CASE("Cron::calculateNextTrigger with timezones",
"[cron]") {
date::set_install(TZ_DATA_DIR);
#endif
- const std::vector time_zones{ "Europe/Berlin", "Asia/Seoul",
"America/Los_Angeles", "Asia/Singapore", "UCT" };
+ const std::vector time_zones{ "Europe/Berlin", "Asia/Seoul",
"America/Los_Angeles", "Asia/Singapore", "UTC" };
Review Comment:
Continuing to support UCT would be a bigger piece of work, if we want to do
it.
Starting with Ubuntu mantic and Debian trixie (and probably other newer
distros), UCT is no longer a top level time zone in `/usr/share/zoneinfo`, but
an alias to UTC under `/usr/share/zoneinfo/Etc`. The `date` library does not
like aliases (when compiled with `USE_OS_TZDB=1`, as we do on Linux/Mac): in
[tzdb::locate_zone](https://github.com/HowardHinnant/date/blob/cd3c57932f7f3906c5876b8f04e2ff6b20d2070b/src/tz.cpp#L3730),
it finds that `tz_name` (= "UCT") is different from the name of the time zone
it points to (= "UTC") and it fails.
EDIT: maybe this can be fixed with an update of the date library.
EDIT2: or maybe we can start using the date utilities from the C++20
standard library?
--
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
Re: [PR] MINIFICPP-2326 Link lua statically [nifi-minifi-cpp]
fgerlits commented on code in PR #1756:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1756#discussion_r1565902422
##
libminifi/test/unit/CronTests.cpp:
##
@@ -421,7 +421,7 @@ TEST_CASE("Cron::calculateNextTrigger with timezones",
"[cron]") {
date::set_install(TZ_DATA_DIR);
#endif
- const std::vector time_zones{ "Europe/Berlin", "Asia/Seoul",
"America/Los_Angeles", "Asia/Singapore", "UCT" };
+ const std::vector time_zones{ "Europe/Berlin", "Asia/Seoul",
"America/Los_Angeles", "Asia/Singapore", "UTC" };
Review Comment:
Continuing to support UCT would be a bigger piece of work, if we want to do
it.
Starting with Ubuntu mantic and Debian trixie (and probably other newer
distros), UCT is no longer a top level time zone in `/usr/share/zoneinfo`, but
an alias to UTC under `/usr/share/zoneinfo/Etc`. The `date` library does not
like aliases: in
[tzdb::locate_zone](https://github.com/HowardHinnant/date/blob/cd3c57932f7f3906c5876b8f04e2ff6b20d2070b/src/tz.cpp#L3730),
it finds that `tz_name` (= "UCT") is different from the name of the time zone
it points to (= "UTC") and it fails.
EDIT: maybe this can be fixed with an update of the date library.
EDIT2: or maybe we can start using the date utilities from the C++20
standard library?
--
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
Re: [PR] MINIFICPP-2326 Link lua statically [nifi-minifi-cpp]
fgerlits commented on code in PR #1756:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1756#discussion_r1565902422
##
libminifi/test/unit/CronTests.cpp:
##
@@ -421,7 +421,7 @@ TEST_CASE("Cron::calculateNextTrigger with timezones",
"[cron]") {
date::set_install(TZ_DATA_DIR);
#endif
- const std::vector time_zones{ "Europe/Berlin", "Asia/Seoul",
"America/Los_Angeles", "Asia/Singapore", "UCT" };
+ const std::vector time_zones{ "Europe/Berlin", "Asia/Seoul",
"America/Los_Angeles", "Asia/Singapore", "UTC" };
Review Comment:
Continuing to support UCT would be a bigger piece of work, if we want to do
it.
Starting with Ubuntu mantic and Debian trixie (and probably other newer
distros), UCT is no longer a top level time zone in `/usr/share/zoneinfo`, but
an alias to UTC under `/usr/share/zoneinfo/Etc`. The `date` library does not
like aliases: in
[tzdb::locate_zone](https://github.com/HowardHinnant/date/blob/cd3c57932f7f3906c5876b8f04e2ff6b20d2070b/src/tz.cpp#L3730),
it finds that `tz_name` (= "UCT") is different from the name of the time zone
it points to (= "UTC") and it fails.
EDIT: maybe this can be fixed with an update of the date library; let me
give it a go.
--
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
Re: [PR] MINIFICPP-2326 Link lua statically [nifi-minifi-cpp]
fgerlits commented on code in PR #1756:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1756#discussion_r1565902422
##
libminifi/test/unit/CronTests.cpp:
##
@@ -421,7 +421,7 @@ TEST_CASE("Cron::calculateNextTrigger with timezones",
"[cron]") {
date::set_install(TZ_DATA_DIR);
#endif
- const std::vector time_zones{ "Europe/Berlin", "Asia/Seoul",
"America/Los_Angeles", "Asia/Singapore", "UCT" };
+ const std::vector time_zones{ "Europe/Berlin", "Asia/Seoul",
"America/Los_Angeles", "Asia/Singapore", "UTC" };
Review Comment:
Continuing to support UCT would be a bigger piece of work, if we want to do
it.
Starting with Ubuntu mantic and Debian trixie (and probably other newer
distros), UCT is no longer a top level time zone in `/usr/share/zoneinfo`, but
an alias to UTC under `/usr/share/zoneinfo/Etc`. The `date` library does not
like aliases: in
[tzdb::locate_zone](https://github.com/HowardHinnant/date/blob/cd3c57932f7f3906c5876b8f04e2ff6b20d2070b/src/tz.cpp#L3730),
it finds that `tz_name` (= "UCT") is different from the name of the time zone
it points to (= "UTC") and it fails.
--
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
Re: [PR] MINIFICPP-2326 Link lua statically [nifi-minifi-cpp]
szaszm commented on code in PR #1756:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1756#discussion_r1565727195
##
libminifi/test/unit/CronTests.cpp:
##
@@ -421,7 +421,7 @@ TEST_CASE("Cron::calculateNextTrigger with timezones",
"[cron]") {
date::set_install(TZ_DATA_DIR);
#endif
- const std::vector time_zones{ "Europe/Berlin", "Asia/Seoul",
"America/Los_Angeles", "Asia/Singapore", "UCT" };
+ const std::vector time_zones{ "Europe/Berlin", "Asia/Seoul",
"America/Los_Angeles", "Asia/Singapore", "UTC" };
Review Comment:
`UTC` and `UCT` are both valid timezones, and they mean the same thing, at
least on my system. `UTC` is generally used to refer to the timezone, but I
want to make sure there is no other reason to use `UCT`. @martinzink can you
comment?
--
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
Re: [PR] MINIFICPP-2326 Link lua statically [nifi-minifi-cpp]
fgerlits commented on PR #1756: URL: https://github.com/apache/nifi-minifi-cpp/pull/1756#issuecomment-2049275853 > I think some other files need to be updated as well Yes, good point, thanks! Fixed in 450f397604263c1faafdfffdd3aab7158556720e. -- 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
Re: [PR] MINIFICPP-2326 Link lua statically [nifi-minifi-cpp]
lordgamez commented on PR #1756: URL: https://github.com/apache/nifi-minifi-cpp/pull/1756#issuecomment-2047939844 I think some other files need to be updated as well: - Remove Lua from build time dependencies in README.md - Remove lua package installation from bootstrap shell files if lua extension is enabled (aptitude.sh, arch.sh, centos.sh, darwin.sh, debian.sh, fedora.sh, rheldistro.sh, suse.sh) - Remove lua package installation from python bootstrap if lua extension is enabled (package_manager.py) - Remove lua development packages from dockerfiles -- 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
[PR] MINIFICPP-2326 Link lua statically [nifi-minifi-cpp]
fgerlits opened a new pull request, #1756: URL: https://github.com/apache/nifi-minifi-cpp/pull/1756 Link liblua statically into the lua script extension instead of using the lua library installed on the system. https://issues.apache.org/jira/browse/MINIFICPP-2326 --- Thank you for submitting a contribution to Apache NiFi - MiNiFi C++. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [x] Does your PR title start with MINIFICPP- where is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character. - [x] Has your PR been rebased against the latest commit within the target branch (typically main)? - [x] Is your initial contribution a single, squashed commit? ### For code changes: - [x] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? - [x] If applicable, have you updated the LICENSE file? - [x] If applicable, have you updated the NOTICE file? ### For documentation related changes: - [ ] Have you ensured that format looks appropriate for the output in which it is rendered? ### Note: Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible. -- 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
