Skip to content

Commit

Permalink
add tzdata to tox dependencies
Browse files Browse the repository at this point in the history 
GH actions on windows seem to lack this package, see if adding
it fixes things.

Change-Id: I9b830c583bbf541d965d6e76e08b68a88f645c54
  • Loading branch information
@zzzeek
zzzeek committed Nov 22, 2023
1 parent cc4c721 commit a8048a1
Showing 1 changed file with 1 addition and 0 deletions.
1 change: 1 addition & 0 deletions tox.ini
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ deps=pytest>4.6
sqlalchemy: sqlalchemy>=1.3.0
mako
backports.zoneinfo;python_version<"3.9"
tzdata

This comment has been minimized.

Sign in to view

Copy link
@CaselIT

CaselIT Nov 22, 2023

Member

how about adding it also to the tz install extra?

This comment has been minimized.

Sign in to view

Copy link
@zzzeek

zzzeek Nov 22, 2023

Author Member

that would interfere with systems that already have tzinfo set up at the OS level, I dont want to get involved with people building containers etc. if zoneinfo itself does not have this dependency then we should not either.

This comment has been minimized.

Sign in to view

Copy link
@CaselIT

CaselIT Nov 22, 2023

Member

ok!

I don't think it would interfere with os level time zones since it's used as fallback only

By default, zoneinfo uses the system’s time zone data if available; if no system time zone data is available, the library will fall back to using the first-party tzdata package available on PyPI.

This comment has been minimized.

Sign in to view

Copy link
@zzzeek

zzzeek Nov 22, 2023

Author Member

OK is this a bug in backports.zoneinfo and/or cpython zoneinfo? why do they have a package missing a dependency ?

This comment has been minimized.

Sign in to view

Copy link
@zzzeek

zzzeek Nov 22, 2023

Author Member

like why is it our dependency and not theirs

This comment has been minimized.

Sign in to view

Copy link
@CaselIT

CaselIT Nov 22, 2023

Member

no sorry it's not clear what I wrote:
tzdata is optional. zoneinfo will look in the os timezone and use that one if it finds it. if not and tzdata is installed it will look into it. if it still does not find it then an error is raised.

my guess is that the ci windows install don't have many time zones available, so it errors.

Not sure if it makes sense adding it to the template comment, but we can do that later if we get issues here

This comment has been minimized.

Sign in to view

Copy link
@CaselIT

CaselIT Nov 22, 2023

Member

to clarify, it's fine not to have it as requirement in the tz extra.

if we get issues we may document that tzdata may be required to find additional time zones

This comment has been minimized.

Sign in to view

Copy link
@zzzeek

zzzeek Nov 22, 2023

Author Member

sure. but I think this is a cpython bug anyway. "batteries included"

This comment has been minimized.

Sign in to view

Copy link
@CaselIT

CaselIT Nov 22, 2023

Member

apparently they are leaking. https://news.ycombinator.com/item?id=19948642
zimports
black==23.3.0
greenlet>=1
Expand Down

0 comments on commit a8048a1

Please sign in to comment.