[tz] Package the source database by default #21671

samuel-emrys · 2023-12-07T02:48:17Z

This replaces the iana binary tzdb with the source tzdb as the default. This is because this is what's supported most comprehensively by the date package. This still retains the option to build the binary database if required.

For additional context:

particularly HowardHinnant/date#611 (comment):

For Windows platforms I strongly encourage people to migrate to C++20 .

Indicating that the upstream project has no desire to support parsing a binary database on windows.

This PR is an initial step to adding support to the date recipe to use the IANA TZDB as a conan package. This is not a feature currently supported by the date recipe, but I've raised HowardHinnant/date#788 upstream detailing the requirement. My plan is to submit a patch upstream (which I don't anticipate will be accepted, details are in the linked issue), and use this as the basis for a patch to the date recipe to allow the path to the source IANA TZDB to be specified as an environment variable to facilitate conan compatibility.

Closes #21670

Specify library name and version: tz/2023c

I've read the contributing guidelines.
I've used a recent Conan client version close to the currently deployed.
I've tried at least one configuration locally with the conan-center hook activated.

* This replaces the iana binary tzdb with the source tzdb as the default. This is because this is what's supported most comprehensively by the date package. This still retains the option to build the binary database if required. Closes conan-io#21670

samuel-emrys · 2023-12-07T14:40:56Z

@uilianries @RubenRBS I've re-kicked off the CI to see if the above issue was transient, but I thought you'd be interested in the error given that this PR is clearly only modifying the tz package, and not nanoflann:

Changes not allowed in build 3:

[nanoflann, tz]

Only one library can be changed in the same PR.

Maybe something to do with the parallelization of jobs?

uilianries · 2023-12-11T08:43:55Z

@samuel-emrys Yes, well observed. Indeed with a problem with concurrent jobs, it happens from time to time. I'll restart the build, thank you for reporting.

* Add tz recipe as a dependency to provide a way of easily consuming/maintaining and upgrading the timezone database * Apply patch from HowardHinnant/date#807 to add the ability to specify the tz database by environment variable * Deprecate `use_system_tz_db` in favour of `with_tzdb` option to handle all mutually exclusive options * Add `with_db_format` option to provide flexibility in how the tz package is consumed. This implementation superesedes conan-io#16285 and conan-io#21719. In the case of conan-io#21719, it turned out that HowardHinnant/date#611 was not mature enough to provide the ability for the date library to read zic-compiled binary variants of the tzdb on Windows. To take an iterative approach, this PR aims to extract the elements that worked from conan-io#21719, which was the use of an environment variable to inject the conan packaged tz database. Use of a binary database has been marked as an invalid configuration on Windows. This functionality can be added at a later date. This pull request is blocked on conan-io#21671, which packages the source database in the tz recipe and makes this the default configuration since this is the only database that can be parsed consistently on all major platforms. Closes conan-io#16204

samuel-emrys · 2024-01-13T12:24:25Z

@uilianries I'm a bit confused about the hook errors here - doesn't seem to apply well to this library at all. Are you able to shed any light? This PR is blocking #22294 so I'm keen to get this through ASAP.

valgur · 2024-01-13T12:28:11Z

Setting package_type = "application" should fix that linter error. The package contains only runtime data, I think, so it should be a reasonably good fit.

samuel-emrys · 2024-01-13T13:13:11Z

Setting package_type = "application" should fix that linter error. The package contains only runtime data, I think, so it should be a reasonably good fit.

Didn't seem to work!

* Swap the quotation characters in the python string to read the source database to enable the command to be parsed correctly on Windows

This file is parsed by windows to convert local timezone mappings to that of the IANA database. It's necessary to use the source version of the database on windows.

…data

* Options can't be queried in the source() method, so remove this and just always download windowsZones.xml

recipes/tz/all/test_package/conanfile.py

valgur · 2024-01-13T13:48:09Z

Opened a PR for the invalid linter error: conan-io/hooks#525

Conan 1.x is not able to parse self.dependencies in the test() method due to differences in the lifetime of objects. The solution presented here is to save the necessary options to file and load them back again in the test() method to be used as appropriate. Additionally, utilise the conan `save` and `load` methods for readability and consistency with other recipes. Co-authored-by: Martin Valgur <martin.valgur@gmail.com>

danimtb · 2024-01-17T11:31:25Z

recipes/tz/all/conandata.yml

+    "windows_zones":
+      url: "https://raw.githubusercontent.com/unicode-org/cldr/fd8ca965e03ca3cf07a0487678e0a1e002646ec0/common/supplemental/windowsZones.xml"
+      sha256: "e5720d0d82a0a62687184bf0d9472f3cf0f7ef8b930ad40f3ae7386d8cb57213"


Hi @samuel-emrys,

Could you please explain why this xml is needed in windows? How does it work to make the date library work with the source database?

I am a bit concerned about adding an external file to the tz package that at first has "nothing to do" with the original source database. I think we need some more context regarding this.

This is something that the date library expects to be present in the source database on windows. It contains the mapping between the windows time zone names and IANA time zones names (description)

You can see the dependency here:

https://github.com/HowardHinnant/date/blob/88a3b1512661decb78c60841f819a14eb200423c/src/tz.cpp#L661-L668

https://github.com/HowardHinnant/date/blob/88a3b1512661decb78c60841f819a14eb200423c/src/tz.cpp#L3686-L3688

If this file isn't in the same directory, it throws having failed to locate it.

Note: CLDR is the defacto authority for these kinds of files - see more about the project here: https://cldr.unicode.org/index

Thank you for your answer. The problem I see, you are trying to modeling this package to work with date, which is not inclined to support it. Packages in ConanCenterIndex, usually, should be independent and follow the upstream to avoid divergent behavior. I do not see that file as part of tz. I would prefer a separated package for it, in case really needed.

It's a "practically required" file to make the IANA tzdb useful on windows - without it, my understanding is that there isn't a way to convert between the windows timezones and IANA timezones. In that context, it could be viewed as a compatibility patch to enable windows compatibility for the package.

Having said that, if you'd prefer it in a different package I can attempt to do that. Please confirm if that's the way you would like me to go. My thought was that they fit together quite naturally in that a user who wants one would invariably want the other.

With respect to your comments about date: I didn't get the feeling that these changes wouldn't be supported out of principle, more time/maintenance constraints. The only modification required to date was to support reading the location of the database from an environment variable, which was a trivial modification.

Because the date library wasn't constructed with package management use in mind, external database provision wasn't a consideration and so the library makes some assumptions about what files are where, and also attempts to download its own files. Understandably this requires some modification to extricate these dependencies from each other.

Your comment gives me some doubt that this approach will be accepted on CCI though. If that's the case - please let me know so that I don't waste any more time pursuing this.

For what it's worth, this solution works really well for me locally. I can't imagine there's too much additional work in getting this functional, but I haven't been able to thoroughly test without the CI service whilst I'm blocked on the hook in conan-io/hooks#525

Another reason to consider bundling windowsZones.xml with the tz database is that it's often updated explicitly to support changes in tzdb - they are tightly coupled: https://github.com/unicode-org/cldr/commits/main/common/supplemental/windowsZones.xml

@samuel-emrys thanks a lot for the useful links and clear explanation. This has helped me to understand how tz database from IANA, the CLDR, and the "date" library work together.

While I understand that having the windowsZones.xml file inside this tz database package is quite practical and convenient, I believe that we would create a "mix" package from CLDR and IANA sources which do not follow the original sources of the packages and could cause different expectations in the user of the recipe.

As described at https://howardhinnant.github.io/date/tz.html#Installation in "windows specfic", this WindowsZones.xml (although tightly coupled with tz database) is something that the date library expects to be installed externally at a certain location. It is not assumed as part of the tz database and not provided automatically by the date library AFAIK.

From my point of view, there should be a separate CLDR recipe that provides the needed file and that should be a require in the date recipe, leaving this tz recipe uncoupled with the WindowsZones.xml file. The downside of this is that the version of both tz recipe and the new CLDR (windowsZones.xml) will have to be compatible according to the historic of changes you pointed out here https://github.com/unicode-org/cldr/commits/main/common/supplemental/windowsZones.xml, however, I believe this dependency represents how the different pieces work together and would be closer to user expectations when using these libraries.

I hope that this reasoning makes sense. I would be happy to collaborate on this if needed. Thanks a lot for the hard work @samuel-emrys and thanks again for taking the time to explain this situation 😄

Thanks for the clarification @danimtb, I'll get started on a windowszones package as well. For awareness because I don't think this is front of mind at the moment, this will mean additional changes to the date library, as it currently assumes that windowsZones.xml is present in the tzdata directory - it places it alongside the timezone database. This obviously won't be possible with the conan architecture as packages are immutable, so modifications will be required to decouple these.

thanks for your understanding. Let's see if we can point the date library to pick the windows zones from a different location. Please ping me if you need any help on this.

recipes/tz/all/conanfile.py

uilianries · 2024-01-17T12:20:47Z

recipes/tz/all/conandata.yml

+    "windows_zones":
+      url: "https://raw.githubusercontent.com/unicode-org/cldr/fd8ca965e03ca3cf07a0487678e0a1e002646ec0/common/supplemental/windowsZones.xml"
+      sha256: "e5720d0d82a0a62687184bf0d9472f3cf0f7ef8b930ad40f3ae7386d8cb57213"


Thank you for your answer. The problem I see, you are trying to modeling this package to work with date, which is not inclined to support it. Packages in ConanCenterIndex, usually, should be independent and follow the upstream to avoid divergent behavior. I do not see that file as part of tz. I would prefer a separated package for it, in case really needed.

uilianries · 2024-01-17T12:24:53Z

recipes/tz/all/conanfile.py

@@ -15,6 +15,9 @@ class TzConan(ConanFile):
    description = "The Time Zone Database contains data that represent the history of local time for many representative locations around the globe."
    topics = ("tz", "tzdb", "time", "zone", "date")
    settings = "os", "build_type", "arch", "compiler"
+    options = {"with_binary_db": [True, False]}
+    default_options = {"with_binary_db": False}


About distributing sources, instead of the binary database, do have a reference from the upstream, like a documentation how to do it, or a tar file with only sources distributed? Reading the https://github.com/eggert/tz I only found using command make to generate the binary, like, the default is with the binary DB, not the opposite.

That's a good question. It looks like date by default downloads a source-only version of the database: https://data.iana.org/time-zones/releases/tzdata2023c.tar.gz. On that page tzdata refers to data files only. So, perhaps I should just download this for the source only version and package everything there? One problem is that the source() method doesn't support conditional downloading based on options.

Having said that, I also can't see any explicit discussion recommending or talking about parsing the source database directly. The first issue raised on date suggested using the binary form HowardHinnant/date#1, which wasn't received particularly positively.

date currently only fully supports the source form of the database. It has limited support for the binary form on linux and macos. It doesn't support it on windows, but there are a couple of pull requests that may form the basis for a potential patch to add that functionality to the CCI package. I made an initial attempt at porting these, but decided that the first step was to take your recommendation and adhere as closely to upstream usage as possible to get something functional.

* Explain where file listing for tzdata has come from, including windowsZones.xml, which is managed by CLDR.

This reverts commit a89ec20.

conan-center-bot · 2024-01-19T06:08:11Z

Conan v1 pipeline ❌

Failure in build 14 (027549f63e8524b5e886631335a175af4361cc85):

tz/2023c:
CI failed to create some packages (All logs)

Logs for packageID 5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9:

[settings]
arch=x86_64
build_type=Release
compiler=gcc
compiler.libcxx=libstdc++11
compiler.version=5
os=Linux

[...]
[HOOK - conan-center.py] pre_build(): [FPIC MANAGEMENT (KB-H007)] 'fPIC' option not found
[HOOK - conan-center.py] pre_build(): [FPIC MANAGEMENT (KB-H007)] OK
tz/2023c: Calling build()
tz/2023c: Package '5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9' built
tz/2023c: Build folder /home/conan/w/prod-v1/bsr/92238/edeac/.conan/data/tz/2023c/_/_/build/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/build-release
tz/2023c: Generated conaninfo.txt
tz/2023c: Generated conanbuildinfo.txt
tz/2023c: Generating the package
tz/2023c: Package folder /home/conan/w/prod-v1/bsr/92238/edeac/.conan/data/tz/2023c/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9
tz/2023c: Calling package()
tz/2023c: Copied 1 file: LICENSE
tz/2023c: Copied 1 file: africa
tz/2023c: Copied 1 file: antarctica
tz/2023c: Copied 1 file: asia
tz/2023c: Copied 1 file: australasia
tz/2023c: Copied 1 file: backward
tz/2023c: Copied 1 file: backzone
tz/2023c: Copied 1 file: calendars
tz/2023c: Copied 1 '.awk' file: checklinks.awk
tz/2023c: Copied 1 '.awk' file: checktab.awk
tz/2023c: Copied 1 file: etcetera
tz/2023c: Copied 1 file: europe
tz/2023c: Copied 1 file: factory
tz/2023c: Copied 1 '.tab' file: iso3166.tab
tz/2023c: Copied 1 '.list' file: leap-seconds.list
tz/2023c: Copied 1 '.awk' file: leapseconds.awk
tz/2023c: Copied 1 file: northamerica
tz/2023c: Copied 1 file: southamerica
tz/2023c: Copied 1 '.awk' file: ziguard.awk
tz/2023c: Copied 1 '.awk' file: zishrink.awk
tz/2023c: Copied 1 '.tab' file: zone.tab
tz/2023c: Copied 1 '.tab' file: zone1970.tab
tz/2023c: Copied 1 '.xml' file: windowsZones.xml
[HOOK - conan-center.py] post_package(): [PACKAGE LICENSE (KB-H012)] OK
[HOOK - conan-center.py] post_package(): [DEFAULT PACKAGE LAYOUT (KB-H013)] OK
[HOOK - conan-center.py] post_package(): [SHARED ARTIFACTS (KB-H015)] OK
[HOOK - conan-center.py] post_package(): [STATIC ARTIFACTS (KB-H074)] OK
[HOOK - conan-center.py] post_package(): [EITHER STATIC OR SHARED OF EACH LIB (KB-H076)] OK
[HOOK - conan-center.py] post_package(): [PC-FILES (KB-H020)] OK
[HOOK - conan-center.py] post_package(): [CMAKE-MODULES-CONFIG-FILES (KB-H016)] OK
[HOOK - conan-center.py] post_package(): [PDB FILES NOT ALLOWED (KB-H017)] OK
[HOOK - conan-center.py] post_package(): [LIBTOOL FILES PRESENCE (KB-H018)] OK
[HOOK - conan-center.py] post_package(): [MS RUNTIME FILES (KB-H021)] OK
[HOOK - conan-center.py] post_package(): [SHORT_PATHS USAGE (KB-H066)] OK
[HOOK - conan-center.py] post_package(): [MISSING SYSTEM LIBS (KB-H043)] OK
[HOOK - conan-center.py] post_package(): [APPLE RELOCATABLE SHARED LIBS (KB-H077)] OK
tz/2023c: WARN: Using the new toolchains and generators without specifying a build profile (e.g: -pr:b=default) is discouraged and might cause failures and unexpected behavior
[HOOK - conan-center.py] post_package(): ERROR: [MATCHING CONFIGURATION (KB-H014)] Package for Header Only does not contain artifacts with these extensions: ['h', 'h++', 'hh', 'hxx', 'hpp', 'cuh'] (https://github.com/conan-io/conan-center-index/blob/master/docs/error_knowledge_base.md#KB-H014-MATCHING-CONFIGURATION) 
[HOOK - conan-center.py] post_package(): ERROR: [MATCHING CONFIGURATION (KB-H014)] Packaged artifacts does not match the settings used: os=Linux, compiler=gcc (https://github.com/conan-io/conan-center-index/blob/master/docs/error_knowledge_base.md#KB-H014-MATCHING-CONFIGURATION) 
ERROR: [HOOK - conan-center.py] post_package(): Some checks failed running the hook, check the output

^{Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability.}

prince-chrismc mentioned this pull request Dec 7, 2023

Pull Requests Ready to be Reviewed prince-chrismc/conan-center-index-pending-review#1

Open

[tz] Fix binary database compilation on Macos

a89ec20

samuel-emrys force-pushed the recipe/21670-add-source-tzdb branch from 063f917 to a89ec20 Compare December 7, 2023 03:38

conan-center-bot added the Failed label Dec 7, 2023

This comment has been minimized.

Sign in to view

uilianries added the Unexpected Error label Dec 11, 2023

conan-center-bot added Service Under Maintenance and removed Unexpected Error Failed labels Dec 14, 2023

This comment has been minimized.

Sign in to view

conan-center-bot added Failed Unexpected Error and removed Service Under Maintenance labels Dec 16, 2023

This comment has been minimized.

Sign in to view

conan-center-bot removed the Unexpected Error label Jan 13, 2024

This comment has been minimized.

Sign in to view

samuel-emrys mentioned this pull request Jan 13, 2024

[date] Add tz recipe as a dependency #22294

Open

3 tasks

This comment has been minimized.

Sign in to view

samuel-emrys added 3 commits January 13, 2024 23:45

[tz] Fix test_package on Windows

1d819e4

* Swap the quotation characters in the python string to read the source database to enable the command to be parsed correctly on Windows

[tz] Add windowsZones.xml to source database

42d7a8f

This file is parsed by windows to convert local timezone mappings to that of the IANA database. It's necessary to use the source version of the database on windows.

[tz] Add package_type = application to reflect runtime dependency on …

a44b92e

…data

samuel-emrys added 2 commits January 13, 2024 23:45

[tz] Remove conditional download of windowsZones.xml

3cafb14

* Options can't be queried in the source() method, so remove this and just always download windowsZones.xml

[tz] Modify conandata.yml scheme to conform to CCI standards

b52af15

samuel-emrys force-pushed the recipe/21670-add-source-tzdb branch from 3f1d679 to b52af15 Compare January 13, 2024 13:16

This comment has been minimized.

Sign in to view

valgur reviewed Jan 13, 2024

View reviewed changes

recipes/tz/all/test_package/conanfile.py Outdated Show resolved Hide resolved

valgur reviewed Jan 13, 2024

View reviewed changes

recipes/tz/all/test_package/conanfile.py Outdated Show resolved Hide resolved

valgur mentioned this pull request Jan 13, 2024

conan-center.py: add an exception for tz conan-io/hooks#525

Merged

This comment has been minimized.

Sign in to view

danimtb reviewed Jan 17, 2024

View reviewed changes

recipes/tz/all/conanfile.py Show resolved Hide resolved

uilianries reviewed Jan 17, 2024

View reviewed changes

recipes/tz/all/conanfile.py Outdated Show resolved Hide resolved

uilianries reviewed Jan 17, 2024

View reviewed changes

samuel-emrys added 2 commits January 19, 2024 16:17

[tz] Add documentation to explain file listing

2a9c43b

* Explain where file listing for tzdata has come from, including windowsZones.xml, which is managed by CLDR.

Revert "[tz] Fix binary database compilation on Macos"

027549f

This reverts commit a89ec20.

jjerphan mentioned this pull request May 24, 2024

Support for timestamps man-group/sparrow#70

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tz] Package the source database by default #21671

[tz] Package the source database by default #21671

samuel-emrys commented Dec 7, 2023 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

samuel-emrys commented Dec 7, 2023

uilianries commented Dec 11, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

samuel-emrys commented Jan 13, 2024

valgur commented Jan 13, 2024

This comment has been minimized.

This comment has been minimized.

samuel-emrys commented Jan 13, 2024

This comment has been minimized.

valgur commented Jan 13, 2024

This comment has been minimized.

danimtb Jan 17, 2024

samuel-emrys Jan 17, 2024

samuel-emrys Jan 17, 2024

uilianries Jan 17, 2024

samuel-emrys Jan 17, 2024 •

edited

Loading

samuel-emrys Jan 17, 2024

danimtb Jan 22, 2024

samuel-emrys Jan 25, 2024

danimtb Jan 25, 2024

uilianries Jan 17, 2024

uilianries Jan 17, 2024

samuel-emrys Jan 17, 2024 •

edited

Loading

conan-center-bot commented Jan 19, 2024

[tz] Package the source database by default #21671

Are you sure you want to change the base?

[tz] Package the source database by default #21671

Conversation

samuel-emrys commented Dec 7, 2023 • edited Loading

This comment has been minimized.

This comment has been minimized.

samuel-emrys commented Dec 7, 2023

uilianries commented Dec 11, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

samuel-emrys commented Jan 13, 2024

valgur commented Jan 13, 2024

This comment has been minimized.

This comment has been minimized.

samuel-emrys commented Jan 13, 2024

This comment has been minimized.

valgur commented Jan 13, 2024

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuel-emrys Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuel-emrys Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

conan-center-bot commented Jan 19, 2024

Conan v1 pipeline ❌

samuel-emrys commented Dec 7, 2023 •

edited

Loading

samuel-emrys Jan 17, 2024 •

edited

Loading

samuel-emrys Jan 17, 2024 •

edited

Loading