bpo-40503: Add compile-time configuration to PEP 615 #20034

pganssle · 2020-05-11T16:57:32Z

This is the implementation for PEP 615's support for compile-time configuration of the time zone search path.

The main implementation is in #19909 — at the moment it's not merged so unfortunately the commits are also present in this PR until that one gets merged. I couldn't come up with a good way to both have a neat diff and run the CI, so I went with running the CI.

The relevant changes are the ones starting with the commit "Add --with-tzpath to autoconf"

https://bugs.python.org/issue40503

This reverts commit b5de778.

This reverts commit 45146f1.

Makefile.pre.in

Yhg1s · 2020-05-11T19:15:13Z

configure.ac

+validate_tzpath() {
+    # Check that each element of the tzpath is an absolute path
+    all_paths=$1
+    while test "${all_paths#*:}" != "$1"; do


The comparison to "$1" confuses me. $all_paths starts out as $1, and you're only removing things, so surely this loop would never end? It also feels like you could do this with grep instead:

if ( echo $1 | grep -qE '(^|:)([^/]|$)' ); then
AC_MSG_ERROR(...);
return 1;
fi

(There are already other uses of grep -E in configure.ac, so I think it's safe to rely on.)

I like it, though it was annoyingly tricky to pull off. It took me forever to realize that autoconf was stripping out the [] characters from the grep string.

configure.ac

Yhg1s · 2020-05-11T19:16:52Z

configure.ac

+        ;;
+esac
+],
+[AC_MSG_RESULT(default)])


Add $TZPATH in the message here, so you can see what that default is?

Good catch. I originally had it this way, but then I played around with the possibility of using something closer to the previous logic, where if TZPATH is not explicitly set, at runtime the module will order the path based on a hard-coded string where the order is determined by which directories exist at import time. In that case, "default" made more sense than any hard-coded string.

I abandoned that approach as YAGNI — if we want to do something like that, we should probably just do it at build time anyway.

setup.py

The import machinery can be somewhat fragile, and the "seamlessly falls back to pure Python" nature of this module makes it so that a problem building the C extension or a failure to import the pure Python version might easily go unnoticed. This adds some "smoke tests" that rely on implementation details of each module to ensure that we're building the ones we think we are.

This should improve support on Windows, including actually building the C extension.

This reverts commit 874dca0.

Yhg1s

LG for the configure changes (for UNIX; not sure if more should be done for Windows).

configure.ac

Yhg1s · 2020-05-12T15:25:55Z

configure.ac

+TZPATH="/usr/share/zoneinfo:/usr/lib/zoneinfo:/usr/share/lib/zoneinfo:/etc/zoneinfo"
+AC_MSG_CHECKING(for --with-tzpath)
+AC_ARG_WITH(tzpath,
+            AS_HELP_STRING([--with-tzpath=<list of relative paths separated by pathsep>]


Absolute paths, yes?

Hah, oops! Good catch.

Suggestion by Petr Viktorin.

zooba

Windows build changes look fine, thanks for getting the installer!

zooba · 2020-05-12T20:42:34Z

.github/workflows/build.yml

+    - name: Install test dependencies
+      run: |
+        .\python.bat -m ensurepip --user
+        .\python.bat -m pip install --user -r Misc/requirements-test.txt


As we discussed, I don't like relying on pip to install into the source tree before we've run the test suite.

In general, system-wide dependencies are installed using the system package manager, or downloaded using specialised scripts and an existing Python installation (see PCbuild/get-externals.py). This would be a new network dependency, and I'd rather keep us to things that are checked in or directly grabbed from GitHub.

zooba · 2020-05-12T20:43:41Z

.github/workflows/coverage.yml

@@ -48,6 +48,7 @@ jobs:
        ./python -m venv .venv
        source ./.venv/bin/activate
        python -m pip install -U coverage
+        python -m pip install -r Misc/requirements-test.txt


Also as discussed, coverage is the one exception, but it's also not a core part of the PR workflow. We already switch CI providers for PRs every time Victor gets annoyed at network instability, so let's try and keep PR clean. We can leave it in the post-merge workflow if you really want.

zooba · 2020-05-12T20:44:46Z

Lib/sysconfig.py

@@ -546,6 +546,7 @@ def get_config_vars(*args):

        if os.name == 'nt':
            _init_non_posix(_CONFIG_VARS)
+            _CONFIG_VARS['TZPATH'] = ''


Shouldn't we just .get() it rather than assuming it'll be set?

I think we should be guaranteeing that it's set, and it should be an error condition otherwise. The "reasonable default" varies by platform, and can be configured at build time.

On Windows, the reasonable default is actually an empty string (no tzdata), but also I'd like to add it as a flag at build time.

zooba · 2020-05-12T20:45:09Z

Lib/test/test_zoneinfo/_support.py

+TZPATH_TEST_LOCK = threading.Lock()
+
+
+def call_once(f):


Lib/test/test_zoneinfo/test_zoneinfo.py

zooba · 2020-05-12T20:53:06Z

Lib/zoneinfo/__init__.py

+from ._common import ZoneInfoNotFoundError
+
+try:
+    from _czoneinfo import ZoneInfo


We normally just use the underscore prefix to indicate an accelerator module. The c prefix hasn't really been used since 3.0

This is a slightly more complicated situation, because _czoneinfo is actually parallel to zoneinfo._zoneinfo. In the reference implementation, the two are zoneinfo._zoneinfo and zoneinfo._czoneinfo, but the limitations of our current build system prevent me from doing that here.

Lib/zoneinfo/_tzpath.py

zooba · 2020-05-12T21:03:14Z

Lib/zoneinfo/_zoneinfo.py

+            + day
+        )
+
+    # TODO: These are not actually epoch dates as they are expressed in local time


Does this TODO need to be fixed?

At some point, but this is a method on a private class and the only issue is that the name is slightly misleading, so it's not in the critical path to an MVP.

PCbuild/_czoneinfo.vcxproj

PCbuild/pcbuild.sln

This is configurable only on POSIX systems at the moment and TZPATH is initialized to an empty string on Windows.

pganssle · 2020-05-14T14:05:43Z

OK, I expected this to take longer than it actually did, but it looks like this is resolved, so I've gone ahead and cherry-picked the commit back into #19909.

Thanks @zooba and @Yhg1s

pganssle added 12 commits May 8, 2020 16:27

Add tests and implementation for ZoneInfo

35dfa40

Add tzdata to Travis requirements

93c03aa

Deliberately break TZData tests to see who is skipping them

45146f1

Add test requirements to Github Actions builds

ea7afea

Add test dependencies to windows AP builds

b5de778

Revert "Add test dependencies to windows AP builds"

6802178

This reverts commit b5de778.

Revert "Deliberately break TZData tests to see who is skipping them"

9292b3c

This reverts commit 45146f1.

fixup! Add tests and implementation for ZoneInfo

0cad151

fixup! Add tests and implementation for ZoneInfo

c12439b

fixup! Add tests and implementation for ZoneInfo

7fa32f2

fixup! Add tests and implementation for ZoneInfo

3e4d81d

fixup! fixup! Add tests and implementation for ZoneInfo

87d8c51

pganssle requested review from zooba and Yhg1s May 11, 2020 16:57

the-knights-who-say-ni added the CLA signed label May 11, 2020

bedevere-bot added the awaiting core review label May 11, 2020

pganssle added the skip news label May 11, 2020

pganssle mentioned this pull request May 11, 2020

bpo-40503: Add tests and implementation for ZoneInfo #19909

Merged

Yhg1s reviewed May 11, 2020

View reviewed changes

pganssle added 6 commits May 11, 2020 15:45

Fix member def for key

2324a30

Fix refleak in error code

3c8427b

Fix refleak on cache miss

4f12629

fixup! Add tests and implementation for ZoneInfo

88ae102

Fix memory leak in zoneinfo_dealloc

9cc8073

Fix refleak in fromutc

3d30dcb

pganssle force-pushed the zoneinfo_compile_config branch from 91cb1dd to e3a52c6 Compare May 12, 2020 13:46

pganssle added the DO-NOT-MERGE label May 12, 2020

pganssle added 3 commits May 12, 2020 10:09

Add Windows build support

507f5c2

This should improve support on Windows, including actually building the C extension.

fixup! Add Windows build support

874dca0

pganssle added 2 commits May 12, 2020 11:15

Revert "fixup! Add Windows build support"

44b3135

This reverts commit 874dca0.

fixup! Add Windows build support

d490586

Yhg1s approved these changes May 12, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels May 12, 2020

pganssle added 2 commits May 12, 2020 12:42

Remove __version__ from __dir__

d9fc16f

Include all globals in __dir__

07fc66d

Suggestion by Petr Viktorin.

pganssle force-pushed the zoneinfo_compile_config branch from e3a52c6 to cb54267 Compare May 12, 2020 16:52

pganssle requested a review from a team as a code owner May 12, 2020 16:52

zooba reviewed May 12, 2020

View reviewed changes

pganssle added 3 commits May 12, 2020 18:03

fixup! Add Windows build support

d55872c

Reduce indentation in _parse_python_tzpath

d7996a6

Add --with-tzpath to autoconf

ddb8244

This is configurable only on POSIX systems at the moment and TZPATH is initialized to an empty string on Windows.

pganssle force-pushed the zoneinfo_compile_config branch from cb54267 to ddb8244 Compare May 12, 2020 22:08

pganssle closed this May 14, 2020

pganssle mentioned this pull request Apr 10, 2022

PEP 615: Add zoneinfo module #84683

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-40503: Add compile-time configuration to PEP 615 #20034

bpo-40503: Add compile-time configuration to PEP 615 #20034

pganssle commented May 11, 2020 •

edited

Loading

Yhg1s May 11, 2020

pganssle May 12, 2020

Yhg1s May 11, 2020

pganssle May 12, 2020

Yhg1s left a comment

Yhg1s May 12, 2020

pganssle May 12, 2020

zooba left a comment

zooba May 12, 2020

zooba May 12, 2020

zooba May 12, 2020

pganssle May 12, 2020

zooba May 12, 2020

zooba May 12, 2020

pganssle May 12, 2020

zooba May 12, 2020

pganssle May 12, 2020

pganssle commented May 14, 2020

bpo-40503: Add compile-time configuration to PEP 615 #20034

bpo-40503: Add compile-time configuration to PEP 615 #20034

Conversation

pganssle commented May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yhg1s left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zooba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pganssle commented May 14, 2020

pganssle commented May 11, 2020 •

edited

Loading