bpo-37552 strptime/strftime return invalid results with UCRT version 17763.615 #14460

paulmon · 2019-06-28T22:41:17Z

Skip tests that fail because of bug in MSVC UCRT failure when the default code page is cp65001.

https://bugs.python.org/issue36511

zooba · 2019-06-28T22:58:10Z

These issues ought to be fixed in the next Windows update (end of year, about the time that 3.8 releases), though we may get a redistributable ucrtbase.dll sooner.

pganssle · 2019-06-29T16:44:22Z

Lib/test/test_strptime.py

@@ -135,6 +135,8 @@ def test_pattern_escaping(self):
                      "%s does not have re characters escaped properly" %
                      pattern_string)

+    @unittest.skipIf(sys.platform == 'win32' and locale.getdefaultlocale()[1] == 'cp65001',


Can you add a comment referencing bpo-36511 here, so people in the future will know when it is safe to stop skipping this?

It might even be worth opening a separate BPO issue for tracking this specific thing, because issue 36511 seems like more of an omnibus issue.

@zooba You probably know more about the lifetime of Windows support here - what is the best way to communicate that it will be safe to remove this skipIf?

Agreed, a specific issue for this particular failure would be good. There was one already for the same thing (to do with passing de_DE instead of de-DE IIRC, which triggered the experimental/broken UTF-8 support in the UCRT) that might be okay to reuse.

I don't know how best to communicate a future time for this, especially since the UCRT may get updated down-level (in earlier releases) and certain fixes may release out of band. Probably best to just leave a comment with the current UCRT version (@paulmon?) and a reminder to verify as new updates are made.

@zooba, Is this the same issue? https://bugs.python.org/issue36792
These tests aren't crashing as indicated in bpo-36792, but they do cause the Windows ARM32 buildbot to fail

I created a new issue: https://bugs.python.org/issue37552 and added comments.

bedevere-bot · 2019-06-29T16:44:30Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

paulmon · 2019-07-10T22:02:02Z

I have made the requested changes; please review again.

bedevere-bot · 2019-07-10T22:02:05Z

Thanks for making the requested changes!

@pganssle: please review the changes made to this pull request.

zooba · 2019-07-11T06:46:02Z

I believe this has been sufficiently handled, but I'll let @pganssle retract his review when he's ready. (And I'll poke him in person today to make sure he's ready ;) )

zware · 2019-07-16T20:24:38Z

Lib/test/test_strptime.py

@@ -135,6 +135,9 @@ def test_pattern_escaping(self):
                      "%s does not have re characters escaped properly" %
                      pattern_string)

+    # bpo-37552 [Windows] strptime/strftime return invalid results with UCRT version 17763.615
+    @unittest.skipIf(sys.platform == 'win32' and locale.getdefaultlocale()[1] == 'cp65001',


Rather than repeating the whole condition and comment several times, I'd rather see it factored out to a simple @skip_if_buggy_ucrt decorator.

Separately, I would suggest that that decorator should actually check for one of the known bugs in the UCRT, and only skip if that bug is still present; we will forget to remove these skips later otherwise :)

Those are both great ideas :)

paulmon · 2019-07-17T02:10:55Z

If the Azure Pipelines PR check passes, I think this is ready for additional feedback and/or merging.

pganssle · 2019-07-17T14:17:35Z

Lib/test/support/__init__.py

+    See bpo-37552 [Windows] strptime/strftime return invalid
+    results with UCRT version 17763.615
+    """
+    global _buggy_ucrt


This is not a particularly expensive check, right? Why cache it instead of checking each time?

The caching is simple and easy to follow; I'd rather keep it than assume it's not worthwhile :)

Comments addressed.

zware

LGTM.

zware · 2019-07-17T15:06:00Z

Lib/test/support/__init__.py

+    See bpo-37552 [Windows] strptime/strftime return invalid
+    results with UCRT version 17763.615
+    """
+    global _buggy_ucrt


The caching is simple and easy to follow; I'd rather keep it than assume it's not worthwhile :)

zware · 2019-07-17T15:06:35Z

Lib/test/support/__init__.py

@@ -2501,6 +2502,27 @@ def skip_unless_symlink(test):
    msg = "Requires functional symlink implementation"
    return test if ok else unittest.skip(msg)(test)

+_buggy_ucrt = None
+def skip_if_buggy_ucrt(test):


Since this now lives in test.support, it might be worthwhile to mention 'time' in the name (skip_if_buggy_ucrt_strfptime or similar).

I added strftime/strptime to the skip message and skip decorator name

miss-islington · 2019-07-18T13:57:02Z

Thanks @paulmon for the PR, and @pganssle for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

bedevere-bot · 2019-07-18T13:57:02Z

@pganssle: Please replace # with GH- in the commit message next time. Thanks!

…17763.615 (pythonGH-14460) A bug in MSVC UCRT version 17763.615 (which has been fixed in newer versions) is causing test failures in some strptime/strftime tests when the default code page is c65001. This change selectively skips the tests affected by this. (cherry picked from commit 9cd39b1) Co-authored-by: Paul Monson <paulmon@users.noreply.github.com>

bedevere-bot · 2019-07-18T13:57:24Z

GH-14839 is a backport of this pull request to the 3.8 branch.

pganssle · 2019-07-18T13:57:41Z

@pganssle: Please replace # with GH- in the commit message next time. Thanks!

Noooo, I forgot I left that in the PR title. Serves me right for not using automerge.

pganssle · 2019-07-18T13:58:21Z

Thanks for the PR and for promptly addressing comments, @paulmon!

…17763.615 (GH-14460) A bug in MSVC UCRT version 17763.615 (which has been fixed in newer versions) is causing test failures in some strptime/strftime tests when the default code page is c65001. This change selectively skips the tests affected by this. (cherry picked from commit 9cd39b1) Co-authored-by: Paul Monson <paulmon@users.noreply.github.com>

paulmon · 2019-07-19T01:02:58Z

Thanks! all of the buildbot tests for Windows ARM32 have passed passed with only warnings since this was merged.

…17763.615 (python#14460) A bug in MSVC UCRT version 17763.615 (which has been fixed in newer versions) is causing test failures in some strptime/strftime tests when the default code page is c65001. This change selectively skips the tests affected by this.

bpo-36511 skip strptime/strftime cp65001 failures

2e8cc0a

paulmon requested review from abalkin and pganssle as code owners June 28, 2019 22:41

the-knights-who-say-ni added the CLA signed label Jun 28, 2019

bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Jun 28, 2019

zooba added needs backport to 3.8 skip news labels Jun 28, 2019

pganssle previously requested changes Jun 29, 2019

View reviewed changes

bedevere-bot removed the awaiting review label Jun 29, 2019

bedevere-bot added the awaiting changes label Jun 29, 2019

paulmon changed the title ~~bpo-36511 skip strptime/strftime cp65001 failures~~ bpo-37552 strptime/strftime return invalid results with UCRT version 17763.615 Jul 10, 2019

add bpo-37552 comments

a805590

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 10, 2019

zware reviewed Jul 16, 2019

View reviewed changes

Paul Monson added 5 commits July 16, 2019 18:12

refactor skips

6cfcddb

merge upstream

323f283

fix bad merge

ce72404

fix bad fix of bad merge

7665b54

commas are important too

ccde057

pganssle reviewed Jul 17, 2019

View reviewed changes

zware approved these changes Jul 17, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jul 17, 2019

include strftime/strptime in skip

e8001cb

pganssle approved these changes Jul 18, 2019

View reviewed changes

pganssle merged commit 9cd39b1 into python:master Jul 18, 2019

bedevere-bot removed the awaiting merge label Jul 18, 2019

bedevere-bot removed the needs backport to 3.8 label Jul 18, 2019

paulmon deleted the buildbot_time branch August 12, 2019 20:06

paulmon mannequin mentioned this pull request May 20, 2022

Add Windows ARM32 buildbot #80692

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-37552 strptime/strftime return invalid results with UCRT version 17763.615 #14460

bpo-37552 strptime/strftime return invalid results with UCRT version 17763.615 #14460

paulmon commented Jun 28, 2019 •

edited by bedevere-bot

Loading

zooba commented Jun 28, 2019

pganssle Jun 29, 2019

zooba Jul 1, 2019

paulmon Jul 10, 2019 •

edited

Loading

paulmon Jul 10, 2019

bedevere-bot commented Jun 29, 2019

paulmon commented Jul 10, 2019

bedevere-bot commented Jul 10, 2019

zooba commented Jul 11, 2019

zware Jul 16, 2019

paulmon Jul 17, 2019

paulmon commented Jul 17, 2019

pganssle Jul 17, 2019

zware Jul 17, 2019

zware left a comment

zware Jul 17, 2019

zware Jul 17, 2019

paulmon Jul 17, 2019

miss-islington commented Jul 18, 2019

bedevere-bot commented Jul 18, 2019

bedevere-bot commented Jul 18, 2019

pganssle commented Jul 18, 2019

pganssle commented Jul 18, 2019

paulmon commented Jul 19, 2019

bpo-37552 strptime/strftime return invalid results with UCRT version 17763.615 #14460

bpo-37552 strptime/strftime return invalid results with UCRT version 17763.615 #14460

Conversation

paulmon commented Jun 28, 2019 • edited by bedevere-bot Loading

zooba commented Jun 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmon Jul 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Jun 29, 2019

paulmon commented Jul 10, 2019

bedevere-bot commented Jul 10, 2019

zooba commented Jul 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmon commented Jul 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zware 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

miss-islington commented Jul 18, 2019

bedevere-bot commented Jul 18, 2019

bedevere-bot commented Jul 18, 2019

pganssle commented Jul 18, 2019

pganssle commented Jul 18, 2019

paulmon commented Jul 19, 2019

paulmon commented Jun 28, 2019 •

edited by bedevere-bot

Loading

paulmon Jul 10, 2019 •

edited

Loading