libical / vzic

A program to convert the IANA (formerly Olson) timezone database files into VTIMEZONE files compatible with the iCalendar (RFC5445) and TZDIST (RFC7808) specifications
Other
16 stars 14 forks source link

Generate VTIMEZONEs for Link zones #16

Closed rsto closed 2 years ago

rsto commented 2 years ago

As discussed with @ksmurchison, this adds the --gen-links option to vzic which instructs vzic to generate a VTIMEZONE file for each zone alias defined in a tzdata Link line. The generated VTIMEZONE has the TZID of the link zone and set its TZID-ALIAS-OF property to the authoritative zone for that link. In addition it aligns the result of symbolic link generation with the result of the new VTIMEZONE output. Before, symbolic links were generated for any Link zone, but VTIMEZONEs of Links only were generated if the zone name was not a to-level zone.

winterz commented 2 years ago

@ksmurchison I'm happy if you would give the final say so on this one.

Should libical use the new --gen-links option when generating the internal zoneinfo files?

ksmurchison commented 2 years ago

@winterz Yes, I believe that we should. Its appears that with the current code, the linked zones aren't added to zone.tab which means that even though a VTIMEZONE for the linked zone can be read from a file on disk, libical doesn't think it exists because it uses zone.tab as its index of what exists. And as more zone aliases get moved in backzone as links, this will become a bigger problem. IIRC, this was found in tzdata 2022c because Europe/Amsterdam was made a link rather than an actual zone.

@rsto can correct me or clarify the problem that this PR fixes

rsto commented 2 years ago

I just updated the PR and did away with the --gen-links argument, because I think it might introduce a regression. I updated the code to allow setting the CREATE_SYMLINK pre-processor define during make. That way, it still is up to how vzic was built whether symbolic links are generated or not. Otherwise it would have been up to vzic users to specify each time how they want to generate links.

rsto commented 2 years ago

assuming that what get generated without --gen-links matches what gets generated now, and what gets generated with --gen-links allows libical to fetch all aliases of zones.

The default output of vzic stays the same. If calling make with CREATE_SYMLINK=0 make ... then the output differs from previous versions as:

rsto commented 2 years ago

I did one additional change for backwards compatibility. Initially, my PR removed a conditional that instructed vzic to ignore timezone links if they do not contain a / character (such as EST5EDT or Japan). The symlink generation code already produced links for such timezone aliases, so I figured it might make sense to also generate their VTIMEZONEs. Now, I added an additional build option to let maintainers decide if the previous behavior should be preserved. For me, it would equally fine to just remove the conditional again, but I do not know the history of why it was added in the first place (it was introduced in 2008 at da263a03).

ksmurchison commented 2 years ago

@winterz I think this is safe to merge. It fixes a bug where some links weren't created because of the aliases being moved into the backzone file which is processed after we have processed (and disposed of) the zone to link to.

Otherwise, no existing data is changed.

The only remaining question is if we need/want the final commit. We're not sure why creating links like EST5EDT is optional.

winterz commented 2 years ago

@ksmurchison thanks for dealing with this.

I'm still confused as to what I need to change (if anything) when updating the builtin zoneinfo data for the next libical release.

I think I want to change the instructions at https://github.com/libical/libical/wiki/For-the-Maintainers:-Update-Zoneinfo#running-vzic to say Run ./vzic --gen-links, right? or you can edit those instructions for me.

also might be nice to document the --gen-links option in Readme.md

rsto commented 2 years ago

@winterz If you want to generate a VTIMEZONE for for each time zone alias, you need to build vzic with the Makefile variable CREATE_SYMLINK set to zero, e.g. this is how we build the timezone for the Cyrus project:

 $ TZID_PREFIX="" CREATE_SYMLINK=0 IGNORE_TOP_LEVEL_LINK=0 make

It's a build-time option because the CREATE_SYMLINK cpp variable already existed, we just now expose it to the build process.

winterz commented 2 years ago

done the same now for the libical builtin zoneinfo will be available in the upcoming libical 3.0.15 release