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

[date] Add tz recipe as dependency and add windows compatibility with binary zic-compiled tzdb #21719

Closed

Conversation

samuel-emrys
Copy link
Contributor

@samuel-emrys samuel-emrys commented Dec 11, 2023

  • Initial implementation adding tz recipe as a dependency
  • Apply patch from PR ADD windows support for USE_SYSTEM_TZ_DB HowardHinnant/date#611 to add ability to specify tz database by environment variable and also enable the binary tz database to be parsed on windows
  • 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

Specify library name and version: date/3.0.1

This implementation supersedes #16285. It makes the following improvements:

  • Allows date to locate the tz database using the TZDATA environment variable. This is an improvement over [date] Patch recipe to use tz recipe by default to obtain time zone database #16285, which hardcoded the path to the tz database within the binary, making it non-portable. Usage of environment variables here allows flexible runtime discovery of the tz database.
  • Adds windows compatibility to read the zic-compiled binary tz database, which is functionality that previously only existed for unix platforms.
  • No changes were required to the build system other than what has been modified in ADD windows support for USE_SYSTEM_TZ_DB HowardHinnant/date#611. This has pro's and cons:
    ** Is the most minimal change required to introduce the features we need to link the tz recipe with conan
    ** Sacrifices some semantic meaning/the build options that need to be enabled to provide desired features are not intuitive.

Closes #16204


* Initial implementation adding tz recipe as a dependency
* Apply patch from PR date#611 to add ability to specify tz database by
  environment variable and also enable the binary tz database to be
  parsed on windows
* 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 recipe
  is consumed
@samuel-emrys
Copy link
Contributor Author

For tracking, I've raised HowardHinnant/date#788 upstream for discussion of changes to the date library that might make integration with a package manager easier. Specifically, the inclusion of environment variables to specify the location of the tz database at runtime. The authors position was that they did not have time to provide an implementation or review any implementation that might be contributed by someone else.

The author was also of the opinion that this may inhibit his attempts to encourage migration to C++20 chrono. Unfortunately, migration to C++20 chrono is not a perfect solution as:

  • Some projects may be constrained to C++ standards older than C++20
  • There is still not good coverage of C++20 chrono implementations across compilers and operating systems

In these cases, it's still necessary for projects to be able to provide date features and use an appropriate tz database.

On further research into what my implementation might look like, I discovered HowardHinnant/date#611 which provides close to the fewest possible changes required to introduce the core features that I conan-center-index needs to couple the date package with the tz package whilst also improving windows compatibility.

For similar reasons above, my belief is that it's unlikely that these changes will be merged upstream (given that it was raised more than 3 years ago). My suggestion is that we incorporate these changes into the date package on conan center to improve its compatibility with the timezone database, and flexibility as to where this timezone database is obtained.

@uilianries uilianries self-assigned this Dec 12, 2023
@samuel-emrys
Copy link
Contributor Author

@uilianries I appreciate that there have been a number of CI related issues over the past week or two (and I suspect you're tracking the failure here as well) - I only have a macOS environment on which to test these changes for the moment (and docker for Linux), so I'm relying on the CI to provide feedback on Windows compatibility. Should I re-trigger the pipeline or is there something that you need to do on the backend?

@uilianries
Copy link
Member

Hello @samuel-emrys!! Sorry about the CI situation, indeed we are babysitting the situation.

Usually, we restart manually in CI (Jenkins), but also when marking a PR with Unexpected error label, the CI should re-start the PR automatically (it re-runs a failed PR every 10 min).

On the user side, it's possible to enforce it, but we ask to avoid doing it frequently to avoid overloading the CI. To do it, you just need to "rebase" you branch by clicking on the update branch button:

Screenshot 2023-12-13 at 10-32-44 date Add tz recipe as dependency and add windows compatibility with binary zic-compiled tzdb by samuel-emrys · Pull Request #21719 · conan-io_conan-center-index

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

* Add patches for v3.0.0 and v2.4.1 that hotfix the changes present in
  HowardHinnant/date#611 on to these older versions. This will allow all
  versions of date currently on CCI to utilise the tz package as a data
  source for the timezone database.

  Some additional modification of the build system was required in these
  patches to ensure a consistent interface across versions. Only the
  minimal changes necessary to introduce desired features were made,
  preserving deficiencies associated with older versions of date.
@conan-center-bot

This comment has been minimized.

* Fix issue in existing patch in which a string to replace '/' with '\\'
  in windows paths was attempting to operate on either a string_view or
  a const string, which was causing compilation errors. This was
  rectified by making a copy of the string before modification.
@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 6 (bf14b04ffffee4ae410dc882b96ea8e0c4b612bb):

  • date/3.0.0:
    All packages built successfully! (All logs)

  • date/2.4.1:
    All packages built successfully! (All logs)

  • date/3.0.1:
    All packages built successfully! (All logs)


Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 6 (bf14b04ffffee4ae410dc882b96ea8e0c4b612bb):

  • date/3.0.0:
    All packages built successfully! (All logs)

  • date/3.0.1:
    All packages built successfully! (All logs)

  • date/2.4.1:
    All packages built successfully! (All logs)

@samuel-emrys
Copy link
Contributor Author

This is not ready to be merged. The tests are failing to read from the binary database, but not erroring appropriately.

samuel-emrys added a commit to samuel-emrys/conan-center-index that referenced this pull request Jan 13, 2024
* 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
Copy link
Contributor Author

I'm closing this in favour of #22294. The Windows binary database parser implemented in HowardHinnant/date#611 turns out not to be mature enough to be functional at this stage. I've pared this patch back, and submitted my own PR with just the modifications necessary to use an environment variable to inject the path to a tz database in HowardHinnant/date#807, which I've added a conan implementation for in #22294.

The ability to read binary databases on Windows is an important consideration, but can be implemented in a future PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[package] date/3.0.1: Should package the tzdata database with the package
3 participants