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

Update dependency constraint to allow for tzinfo v2.0.4 #8516

Closed
jekyllbot opened this issue Dec 17, 2020 · 8 comments · Fixed by #8880
Closed

Update dependency constraint to allow for tzinfo v2.0.4 #8516

jekyllbot opened this issue Dec 17, 2020 · 8 comments · Fixed by #8880
Labels
dependency frozen-due-to-age has-pull-request Somebody suggested a solution to fix this issue pinned

Comments

@jekyllbot
Copy link
Contributor

Hey there! 👋

I noticed that the constraint you have for tzinfo doesn't allow for the latest version to be used.

The constraint I found was ~> 1.2, and the latest version available is 2.0.4.

Can you look into updating that constraint so our users can use the latest and greatest version? Thanks! 💞

@penguoir
Copy link

In #7562, this is the explanation given:

Lock use of tzinfo gem (Windows n JRuby only) to v1.x to avoid conflicts with activesupport gem https://rubygems.org/gems/activesupport which is typically present in a site's gemset via some plugins and also bundled with the github-pages gem https://rubygems.org/gems/github-pages

However, ActiveSupport doesn't use v1.x anymore (since rails/rails@e9425ab)

So there shouldn't be an issue with upgrading to v2.

@penguoir
Copy link

From the tzinfo changelog

Local times are now returned using the correct UTC offset (instead of using UTC). tzinfo/tzinfo#49 and tzinfo/tzinfo#52.

There's a big API change from v1-v2 so it could be tricky to implement.

@ashmaroli
Copy link
Member

@penguoir Yes. The change in API is a significant issue.
It is the very reason why the version constraint hasn't been relaxed insofar.
Unfortunately, our bot buddy isn't smart enough.. so 🤷‍♂️

@penguoir
Copy link

Is this an important thing to work on? I could give it a go...

@ashmaroli
Copy link
Member

Is this an important thing to work on?

It is not at all important as of this posting.

@philr
Copy link
Contributor

philr commented Nov 11, 2021

Hi,

I'm the author of tzinfo.

I'm not too familiar with Jekyll. It seems that tzinfo is just being used in the WinTZ.calculate method? That can be made compatible with all versions of tzinfo by using TZInfo::Timezone#current_period instead of TZinfo::Timezone#now:

tz = TZInfo::Timezone.get(timezone)
difference = -tz.current_period.utc_total_offset

The current WinTZ.calculate implementation will vary depending on whether daylight savings time is in use. I'm not sure if that's intentional. #utc_total_offset (as above) will give you the current offset from UTC, so replicates the same behaviour. If it was intended to just be the (usually) unvarying base offset from UTC then #utc_offset can be used instead:

tz = TZInfo::Timezone.get(timezone)
difference = -tz.current_period.utc_offset

Note that #utc_total_offset and #utc_offset have been renamed as #observed_utc_offset and #base_utc_offset respectively in tzinfo v2. The old names have been retained as aliases for backwards compatibility.

@ashmaroli
Copy link
Member

Hello @philr
Thank you for your insightful comment.

Your deduction that TZInfo is used only in the WinTZ module is correct.
Will you be able to submit a pull request to implement your proposed changes..?

The whole intention behind the WinTZ module was to provide parity between timezone assigned to Jekyll site-builds across Unix-based OSs and Windows (the latter not having built-in posix-based TZ data).

In Jekyll, the timezone is used to maintain consistency of generated dates across different site-builds irrespective of local time.
For example, if a site has multiple owners with one owner based in California using a Linux-based machine and another owner based in India on a Windows based machine, then with a timezone: America/Los_Angeles configuration set, the base time used as internal reference is always going to reflect the current time under America/Los_Angeles irrespective of whether the build was initiated by the owner in US or the owner in India (who may be around 12hours ahead in time of the former).

IMO, aligning the UTC offset based on DST is recommended so as to maintain backwards-compatibility and reduce confusion.
And there hasn't been a request filed to support disabling DST variation in all these years.

philr added a commit to philr/jekyll that referenced this issue Nov 14, 2021
Use the defined offset (in a v1 and v2 compatible way) instead of
determining the offset from the difference between local time and UTC.

Change the minutes calculation to allow for time zones that don't use
hour or half hour offsets (e.g. Australia/Eucla's UTC+08:45).

Resolves jekyll#8516.
philr added a commit to philr/jekyll that referenced this issue Nov 14, 2021
Use the defined offset (in a v1 and v2 compatible way) instead of
determining the offset from the difference between local time and UTC.

Change the minutes calculation to allow for time zones that don't use
hour or half hour offsets (e.g. Australia/Eucla's UTC+08:45).

Resolves jekyll#8516.
@jekyllbot jekyllbot added the has-pull-request Somebody suggested a solution to fix this issue label Nov 14, 2021
@philr
Copy link
Contributor

philr commented Nov 14, 2021

@ashmaroli I've submitted pull request #8880 with the proposed change.

I've actually used TZInfo::Timezone#period_for_utc instead of #current_period in order to add some unit tests that don't rely on the current time. This is still compatible with all versions of tzinfo.

@jekyll jekyll locked and limited conversation to collaborators Dec 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dependency frozen-due-to-age has-pull-request Somebody suggested a solution to fix this issue pinned
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants