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

Change to upstream tzdata #1

Merged

Conversation

mbargull
Copy link
Member

@mbargull mbargull commented Oct 6, 2020

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

This changes this repository to build the upstream Time Zone Database as the package named tzdata on conda-forge.
Up until now, this recipe built the Python package tzdata, which is now living at https://github.com/conda-forge/python-tzdata-feedstock and will be available under the python-tzdata name on the conda-forge channel.
Following up on: conda-forge/python-feedstock#392 and conda-forge/staged-recipes#12828

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

@@ -1,41 +1,72 @@
{% set version = "2020.1" %}
{% set version = "2020a" %}
Copy link
Member Author

Choose a reason for hiding this comment

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

Although conda also uses Python-like versioning, tzdata uses consistent versions for which the non-numeric suffix is always lexicographical increasing.
Meaning: It's fine for us to use the upstream version scheme.


requirements:
build:
- {{ compiler('c') }}
Copy link
Member Author

Choose a reason for hiding this comment

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

This uses a compiler because it builds its own tools. We add the ignore_run_exports above to get a noarch: generic package without any other dependencies.

recipe/build.sh Outdated
TOPDIR="${PREFIX}" \
USRDIR='' \
POSIXRULES='' \
ZFLAGS='-b slim' \
Copy link
Member

Choose a reason for hiding this comment

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

I would use -b fat (or whatever the equivalent is) for now. pytz and dateutil will have problems with the slim versions, unfortunately.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, thanks. I assume you intend to change the Python package to -b fat then too?

Copy link
Member

Choose a reason for hiding this comment

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

No, the Python package is not used by anything except zoneinfo at the moment, whereas dateutil will automatically use the system time zone information, and pytz has an option to use the system time zone data. Anyone who starts using the Python tzdata package should support slim files.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. Makes sense to assume everything that uses newly introduced functionality to support slim.

mbargull added a commit to mbargull/cf-mark-broken that referenced this pull request Oct 6, 2020
tzdata will now carry the upstream database as a noarch:generic package.
refs:
 - conda-forge/tzdata-feedstock#1
ZIC='$(ZIC)'

-INSTALL_DATA_DEPS = zic leapseconds yearistype tzdata.zi
+INSTALL_DATA_DEPS = zic leapseconds yearistype tzdata.zi $(TABDATA)
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to patch this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to avoid the manual copying of those extra files ;).
But if we don't need them, we can remove that whole commit. I just don't know if it makes sense to include them or not.
(ref: python/tzdata#28)

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why you have to patch anything to get those files. Upstream the way I unpack this is here. I do:

make DESTDIR={temporary_directory} POSIXRULES= ZFLAGS='-b slim' install

(You would use -b fat) and then move everything that's in temporary_directory/usr/share/zoneinfo over to the location I'm expecting. As far as I know no additional copying is necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just wanted a Make target that installed everything in TZDIR. But I'm also more than fine with the way you did it and added a commit to do just that!
The tests in this recipe will also catch when anything unexpected would get installed, so all good either way.

@ocefpaf
Copy link
Member

ocefpaf commented Oct 6, 2020

@mbargull thanks so much for doing this! I was planning to hack at it over the weekend but could not even get close to reading the build intructions.

I would like to say I approve and this is ready to go but you have the much better expertise of @pganssle here so I'll defer to him to tell us when this is OK to merge.

beckermr pushed a commit to conda-forge/admin-requests that referenced this pull request Oct 6, 2020
tzdata will now carry the upstream database as a noarch:generic package.
refs:
 - conda-forge/tzdata-feedstock#1
@mbargull
Copy link
Member Author

mbargull commented Oct 6, 2020

Thanks @ocefpaf. And yes, I'm also very happy that @pganssle is helping out here, much appreciated!

@ocefpaf
Copy link
Member

ocefpaf commented Oct 6, 2020

@pganssle any last comments or is this OK to be merged?

@mbargull
Copy link
Member Author

mbargull commented Oct 6, 2020

This is likely good to go now -- anything else we should change, @pganssle? Would you like to help maintaining this recipe, too? I took you off the maintainers list just to not enforce an additional recipe onto you -- but of course we'd be happy to have you!

Just to recap what's different from https://github.com/conda-forge/python-tzdata:

  • This is not a noarch: python package anymore, but just a noarch: generic with all files under share/zoneinfo.
  • This doesn't carry the zones files from the Python package.
  • The actual data files are the same as in the Python package, but not built with -b slim.
  • We can use this as a dependency for the python package and point its TZPATH to the directory from this package then.

@ocefpaf
Copy link
Member

ocefpaf commented Oct 6, 2020

Let's get this in. We can always add/remove stuff later. Thanks @mbargull for fixing this and @pganssle for the valuable insight!

@ocefpaf ocefpaf merged commit 2c0fd2b into conda-forge:master Oct 6, 2020
@pganssle
Copy link
Member

pganssle commented Oct 7, 2020

This is likely good to go now -- anything else we should change, @pganssle? Would you like to help maintaining this recipe, too? I took you off the maintainers list just to not enforce an additional recipe onto you -- but of course we'd be happy to have you!

Hey, sorry, this seemed low-priority since it was merged before I got back to it, but yeah I'm fine helping to maintain this feedstock.

Thanks for putting this together, I think it looks great!

(Now for the first test — tzdata 2020b was just released!)

ammattita6i added a commit to ammattita6i/admin-requests that referenced this pull request Aug 14, 2024
tzdata will now carry the upstream database as a noarch:generic package.
refs:
 - conda-forge/tzdata-feedstock#1
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.

5 participants