Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[tz] Package the source database by default #21671

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
8 changes: 6 additions & 2 deletions recipes/tz/all/conandata.yml
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
sources:
"2023c":
url: "https://github.com/eggert/tz/archive/refs/tags/2023c.tar.gz"
sha256: "9aa20ef838183e58f09acca92098cf6aa6d8e229aecf24e098c3af2a38e596f8"
"sources":
url: "https://github.com/eggert/tz/archive/refs/tags/2023c.tar.gz"
sha256: "9aa20ef838183e58f09acca92098cf6aa6d8e229aecf24e098c3af2a38e596f8"
"windows_zones":
url: "https://raw.githubusercontent.com/unicode-org/cldr/fd8ca965e03ca3cf07a0487678e0a1e002646ec0/common/supplemental/windowsZones.xml"
sha256: "e5720d0d82a0a62687184bf0d9472f3cf0f7ef8b930ad40f3ae7386d8cb57213"
Comment on lines +6 to +8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@samuel-emrys samuel-emrys Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

86 changes: 66 additions & 20 deletions recipes/tz/all/conanfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

from conan import ConanFile
from conan.tools.gnu import Autotools, AutotoolsToolchain
from conan.tools.files import get, copy, rmdir, replace_in_file
from conan.tools.files import get, copy, rmdir, replace_in_file, download
from conan.tools.layout import basic_layout

required_conan_version = ">=1.53.0"
Expand All @@ -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}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@samuel-emrys samuel-emrys Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

package_type = "application" # This is not an application, but application has the correct traits to provide a runtime dependency on data

@property
def _settings_build(self):
Expand All @@ -29,16 +32,20 @@ def layout(self):

def package_id(self):
del self.info.settings.compiler
if not self.info.options.with_binary_db:
self.info.clear()

def build_requirements(self):
self.tool_requires("mawk/1.3.4-20230404")
if self._settings_build.os == "Windows":
self.win_bash = True
if not self.conf.get("tools.microsoft.bash:path", check_type=str):
self.tool_requires("msys2/cci.latest")
if self.options.with_binary_db:
self.tool_requires("mawk/1.3.4-20230404")
if self._settings_build.os == "Windows":
self.win_bash = True
if not self.conf.get("tools.microsoft.bash:path", check_type=str):
self.tool_requires("msys2/cci.latest")

def source(self):
get(self, **self.conan_data["sources"][self.version], strip_root=True)
get(self, **self.conan_data["sources"][self.version]["sources"], strip_root=True)
download(self, **self.conan_data["sources"][self.version]["windows_zones"], filename="windowsZones.xml")

def generate(self):
tc = AutotoolsToolchain(self)
Expand All @@ -50,24 +57,63 @@ def _patch_sources(self):
replace_in_file(self, os.path.join(self.source_folder, "Makefile"), "AWK= awk", f"AWK={awk_path}")

def build(self):
self._patch_sources()
autotools = Autotools(self)
autotools.make(args=["-C", self.source_folder.replace("\\", "/")])
if self.options.with_binary_db:
self._patch_sources()
autotools = Autotools(self)
ldlibs = ""
if self.settings.os == "Macos":
ldlibs = "-lintl"
samuel-emrys marked this conversation as resolved.
Show resolved Hide resolved
autotools.make(args=["-C", self.source_folder.replace("\\", "/"), f"LDLIBS={ldlibs}"])

def package(self):
copy(self, "LICENSE", dst=os.path.join(self.package_folder, "licenses"), src=self.source_folder)
autotools = Autotools(self)
destdir = self.package_folder.replace('\\', '/')
autotools.install(args=["-C", self.source_folder.replace("\\", "/"), f"DESTDIR={destdir}"])
rmdir(self, os.path.join(self.package_folder, "usr", "share", "man"))
# INFO: The library does not have a public API, it's used to build the zic and zdump tools
rmdir(self, os.path.join(self.package_folder, "usr", "lib"))
if self.options.with_binary_db:
autotools = Autotools(self)
destdir = self.package_folder.replace('\\', '/')
autotools.install(args=["-C", self.source_folder.replace("\\", "/"), f"DESTDIR={destdir}"])
rmdir(self, os.path.join(self.package_folder, "usr", "share", "man"))
# INFO: The library does not have a public API, it's used to build the zic and zdump tools
rmdir(self, os.path.join(self.package_folder, "usr", "lib"))
else:
tzdata = [
samuel-emrys marked this conversation as resolved.
Show resolved Hide resolved
"africa",
"antarctica",
"asia",
"australasia",
"backward",
"backzone",
"calendars",
"checklinks.awk",
"checktab.awk",
"etcetera",
"europe",
"factory",
"iso3166.tab",
"leap-seconds.list",
"leapseconds",
"leapseconds.awk",
"NEWS",
"northamerica",
"southamerica",
"version",
"ziguard.awk",
"zishrink.awk",
"zone.tab",
"zone1970.tab",
"windowsZones.xml",
]
for data in tzdata:
copy(self, data, dst=os.path.join(self.package_folder, "res", "tzdata"), src=self.source_folder)

def package_info(self):
self.cpp_info.libdirs = []
self.cpp_info.includedirs = []
self.cpp_info.frameworkdirs = []
self.cpp_info.resdirs = [os.path.join("usr", "share")]
self.cpp_info.bindirs = [os.path.join("usr", "bin"), os.path.join("usr", "sbin")]
self.buildenv_info.define("TZDATA", os.path.join(self.package_folder, "usr", "share", "zoneinfo"))
self.runenv_info.define("TZDATA", os.path.join(self.package_folder, "usr", "share", "zoneinfo"))
self.cpp_info.resdirs = ["res"]
self.buildenv_info.define("TZDATA", os.path.join(self.package_folder, "res", "tzdata"))
self.runenv_info.define("TZDATA", os.path.join(self.package_folder, "res", "tzdata"))
if self.options.with_binary_db:
self.cpp_info.resdirs = [os.path.join("usr", "share")]
self.cpp_info.bindirs = [os.path.join("usr", "bin"), os.path.join("usr", "sbin")]
self.buildenv_info.define("TZDATA", os.path.join(self.package_folder, "usr", "share", "zoneinfo"))
self.runenv_info.define("TZDATA", os.path.join(self.package_folder, "usr", "share", "zoneinfo"))
30 changes: 22 additions & 8 deletions recipes/tz/all/test_package/conanfile.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
from conan import ConanFile
from conan.tools.build import can_run
from conan.tools.layout import basic_layout
from conan.tools.files import save, load
from conan.errors import ConanException

Check warning on line 5 in recipes/tz/all/test_package/conanfile.py

View workflow job for this annotation

GitHub Actions / Lint changed test_package/conanfile.py (v2 migration)

Unused ConanException imported from conan.errors
import os


Expand All @@ -8,28 +10,40 @@
settings = "os", "compiler", "build_type", "arch"
generators = "VirtualBuildEnv", "VirtualRunEnv"
test_type = "explicit"
tzdata = None

@property
def _settings_build(self):
return getattr(self, "settings_build", self.settings)

def layout(self):
basic_layout(self, src_folder="src")

def requirements(self):
self.requires(self.tested_reference_str)

def build_requirements(self):
self.tool_requires(self.tested_reference_str)
if self._settings_build.os == "Windows" and not self.conf.get("tools.microsoft.bash:path", check_type=str):
self.tool_requires("msys2/cci.latest")

def generate(self):
# INFO: zdump does not consume TZDATA, need to pass absolute path of the zoneinfo directory
self.tzdata = self.dependencies.build['tz'].buildenv_info.vars(self).get('TZDATA')
with open("tzdata.info", "w") as fd:
fd.write(self.tzdata)
tzdata = self.dependencies['tz'].runenv_info.vars(self).get('TZDATA')
with_binary_db = str(self.dependencies['tz'].options.with_binary_db)
save(self, "tzdata.info", tzdata)
save(self, "with_binary_db.option", with_binary_db)

def build(self):
pass

def test(self):
if can_run(self):
with open("tzdata.info", "r") as fd:
self.tzdata = fd.read()
self.run(f"zdump {os.path.join(self.tzdata, 'America', 'Los_Angeles')}")
with_binary_db = load(self, os.path.join(self.generators_folder, "with_binary_db.option")) == 'True'
if with_binary_db:
self.output.info("Test that binary tzdb is readable")
tzdata = load(self, os.path.join(self.generators_folder, "tzdata.info"))
la_tz = os.path.join(tzdata, 'America', 'Los_Angeles')
self.run(f"zdump {la_tz}", env="conanrun")
else:
self.output.info("Test that source tzdb is readable")
cmd = "python -c \"import os; tzdata = os.environ['TZDATA']; f=open(os.path.join(tzdata, 'factory'), 'r'); s = f.read(); f.close(); print(s)\""
self.run(cmd, env="conanrun")