GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-127527: Improve error handling in time_strftime1 by hoodmane · Pull Request #127528 · python/cpython

Conversation

hoodmane commented Dec 2, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

This fixes python -m test test_datetime -mtest_strftime_trailing_percent on Emscripten. On other musl platforms it will:

if HAVE_WCSFTIME is true, change strftime("format string ending in %") from returning empty string to raising ValueError("Invalid format string")
if HAVE_WCSFTIME is false, change strftime("format string ending in %") from raising ValueError("embedded null byte") to ValueError("invalid format string").

It also should change the behavior for other unsupported behaviors from returning empty string to raising Invalid format string. Also, if the result is actually empty, the exit condition (*outbuf)[buflen] == '\0' is always true the first time and allows us to avoid resizing the buffer and calling format_time again.

Issue: Musl libc strftime for format strings ending in % #127527

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          pythongh-127527: Improve error handling in time_strftime1

4615f6d

hoodmane requested review from abalkin and pganssle as code owners

December 2, 2024 18:52

bedevere-app Bot mentioned this pull request

Dec 2, 2024

Musl libc strftime for format strings ending in % #127527

Open

bedevere-app Bot added the awaiting review label

Dec 2, 2024

nineteendo commented Dec 2, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Can you add tests and a news entry?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hoodmane commented Dec 2, 2024

Copy link

Copy Markdown

Contributor Author

I can add a news entry. I'll have to think about tests. There's already coverage for this in any job that uses Musl such as the wasi job. Hopefully if I read the glibc implementation of strftime / wcsftime I can find a format string that will hit the new codepath on glibc as well as musl.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nineteendo commented Dec 2, 2024

Copy link

Copy Markdown

Contributor

Well, in any case these tests in test.test_strptime.StrptimeTests need to be updated:

test_date_locale
test_date_locale2
test_date_time_locale
test_date_time_locale2
test_month_locale
test_time_locale
test_weekday_locale

Details

======================================================================
ERROR: test_date_locale (test.test_strptime.StrptimeTests.test_date_locale) (locale='fr_FR')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/cpython/cpython/Lib/test/support/__init__.py", line 968, in wrapper
    func(self, *args, **kwargs)
    ~~~~^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 542, in test_date_locale
    self.roundtrip('%x', slice(0, 3), time.localtime(now))
    ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 306, in roundtrip
    strp_output = _strptime._strptime_time(strf_output, fmt)
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 670, in _strptime_time
    tt = _strptime(data_string, format)[0]
         ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 431, in _strptime
    _TimeRE_cache = TimeRE()
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 285, in __init__
    self.locale_time = LocaleTime()
                       ~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 86, in __init__
    self.__calc_am_pm()
    ~~~~~~~~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 119, in __calc_am_pm
    am_pm.append(time.strftime("%p", time_tuple).lower().strip())
                 ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
ValueError: Invalid format string
======================================================================
ERROR: test_date_locale2 (test.test_strptime.StrptimeTests.test_date_locale2) (locale='fr_FR')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/cpython/cpython/Lib/test/support/__init__.py", line 968, in wrapper
    func(self, *args, **kwargs)
    ~~~~^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 565, in test_date_locale2
    self.roundtrip('%x', slice(0, 3), (1900, 1, 1, 0, 0, 0, 0, 1, 0))
    ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 306, in roundtrip
    strp_output = _strptime._strptime_time(strf_output, fmt)
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 670, in _strptime_time
    tt = _strptime(data_string, format)[0]
         ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 431, in _strptime
    _TimeRE_cache = TimeRE()
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 285, in __init__
    self.locale_time = LocaleTime()
                       ~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 86, in __init__
    self.__calc_am_pm()
    ~~~~~~~~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 119, in __calc_am_pm
    am_pm.append(time.strftime("%p", time_tuple).lower().strip())
                 ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
ValueError: Invalid format string

======================================================================
ERROR: test_date_time_locale (test.test_strptime.StrptimeTests.test_date_time_locale) (locale='fr_FR')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/cpython/cpython/Lib/test/support/__init__.py", line 968, in wrapper
    func(self, *args, **kwargs)
    ~~~~^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 502, in test_date_time_locale
    self.roundtrip('%c', slice(0, 6), time.localtime(now))
    ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 306, in roundtrip
    strp_output = _strptime._strptime_time(strf_output, fmt)
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 670, in _strptime_time
    tt = _strptime(data_string, format)[0]
         ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 431, in _strptime
    _TimeRE_cache = TimeRE()
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 285, in __init__
    self.locale_time = LocaleTime()
                       ~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 86, in __init__
    self.__calc_am_pm()
    ~~~~~~~~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 119, in __calc_am_pm
    am_pm.append(time.strftime("%p", time_tuple).lower().strip())
                 ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
ValueError: Invalid format string

======================================================================
ERROR: test_date_time_locale2 (test.test_strptime.StrptimeTests.test_date_time_locale2) (locale='fr_FR')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/cpython/cpython/Lib/test/support/__init__.py", line 968, in wrapper
    func(self, *args, **kwargs)
    ~~~~^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 529, in test_date_time_locale2
    self.roundtrip('%c', slice(0, 6), (1900, 1, 1, 0, 0, 0, 0, 1, 0))
    ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 306, in roundtrip
    strp_output = _strptime._strptime_time(strf_output, fmt)
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 670, in _strptime_time
    tt = _strptime(data_string, format)[0]
         ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 431, in _strptime
    _TimeRE_cache = TimeRE()
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 285, in __init__
    self.locale_time = LocaleTime()
                       ~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 86, in __init__
    self.__calc_am_pm()
    ~~~~~~~~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 119, in __calc_am_pm
    am_pm.append(time.strftime("%p", time_tuple).lower().strip())
                 ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
ValueError: Invalid format string

======================================================================
ERROR: test_month_locale (test.test_strptime.StrptimeTests.test_month_locale) (locale='fr_FR')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/cpython/cpython/Lib/test/support/__init__.py", line 968, in wrapper
    func(self, *args, **kwargs)
    ~~~~^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 335, in test_month_locale
    self.roundtrip('%B', 1)
    ~~~~~~~~~~~~~~^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 306, in roundtrip
    strp_output = _strptime._strptime_time(strf_output, fmt)
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 670, in _strptime_time
    tt = _strptime(data_string, format)[0]
         ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 431, in _strptime
    _TimeRE_cache = TimeRE()
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 285, in __init__
    self.locale_time = LocaleTime()
                       ~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 86, in __init__
    self.__calc_am_pm()
    ~~~~~~~~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 119, in __calc_am_pm
    am_pm.append(time.strftime("%p", time_tuple).lower().strip())
                 ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
ValueError: Invalid format string

======================================================================
ERROR: test_time_locale (test.test_strptime.StrptimeTests.test_time_locale) (locale='fr_FR')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/cpython/cpython/Lib/test/support/__init__.py", line 968, in wrapper
    func(self, *args, **kwargs)
    ~~~~^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 590, in test_time_locale
    self.roundtrip('%X', pos, time.localtime(now))
    ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 306, in roundtrip
    strp_output = _strptime._strptime_time(strf_output, fmt)
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 670, in _strptime_time
    tt = _strptime(data_string, format)[0]
         ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 431, in _strptime
    _TimeRE_cache = TimeRE()
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 285, in __init__
    self.locale_time = LocaleTime()
                       ~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 86, in __init__
    self.__calc_am_pm()
    ~~~~~~~~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 119, in __calc_am_pm
    am_pm.append(time.strftime("%p", time_tuple).lower().strip())
                 ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
ValueError: Invalid format string

======================================================================
ERROR: test_weekday_locale (test.test_strptime.StrptimeTests.test_weekday_locale) (locale='fr_FR')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/cpython/cpython/Lib/test/support/__init__.py", line 968, in wrapper
    func(self, *args, **kwargs)
    ~~~~^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 378, in test_weekday_locale
    self.roundtrip('%A', 6)
    ~~~~~~~~~~~~~~^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/test/test_strptime.py", line 306, in roundtrip
    strp_output = _strptime._strptime_time(strf_output, fmt)
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 670, in _strptime_time
    tt = _strptime(data_string, format)[0]
         ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 431, in _strptime
    _TimeRE_cache = TimeRE()
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 285, in __init__
    self.locale_time = LocaleTime()
                       ~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 86, in __init__
    self.__calc_am_pm()
    ~~~~~~~~~~~~~~~~~^^
  File "/Users/runner/work/cpython/cpython/Lib/_strptime.py", line 119, in __calc_am_pm
    am_pm.append(time.strftime("%p", time_tuple).lower().strip())
                 ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
ValueError: Invalid format string

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hoodmane commented Dec 2, 2024

Copy link

Copy Markdown

Contributor Author

Or maybe it's a sign that the patch is wrong...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hoodmane commented Dec 2, 2024

Copy link

Copy Markdown

Contributor Author

Hmm so the failures are all of the French locale?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hoodmane added 3 commits

December 3, 2024 09:59


          Debug

a7cf1b3


          Double buffer size

ca016d7


          Restore backwards compatibility

d3b0056

hoodmane force-pushed the strftime-error-handling branch from 81e12cd to d3b0056 Compare

December 3, 2024 09:56

hoodmane commented Dec 3, 2024

Copy link

Copy Markdown

Contributor Author

If something is wrong with the format string, on Windows it raises a ValueError and on all other platforms it returns an empty string. I think ideally it we should always raise a ValueError but it is a backwards incompatible change...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nineteendo commented Dec 3, 2024

Copy link

Copy Markdown

Contributor

Please don't force push (https://devguide.python.org/getting-started/pull-request-lifecycle):

In order to keep the commit history intact, please avoid squashing or amending history and then force-pushing to the PR. Reviewers often want to look at individual commits.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hoodmane added 6 commits

December 3, 2024 11:36


          Update comment

95c7213


          Update documentation

d0bd247


          Whitespace

a2daaf4


          Add news entry

c3955c5


          More docs updates

5e5e243


          Fix lint

4d67df4

hoodmane requested a review from vstinner

December 3, 2024 11:25

hoodmane mentioned this pull request

Dec 3, 2024

Handle trailing % in strftime emscripten-core/emscripten#23045

Merged

hoodmane requested a review from encukou

December 3, 2024 12:15

encukou reviewed

Dec 10, 2024

View reviewed changes

Comment thread

Doc/library/datetime.rst

Comment on lines +2645 to +2649

		Handling of format strings containing Unicode code points that can't be
		represented in the charset of the current locale is also platform-dependent. On
		some platforms such code points are preserved intact in the output, while on
		others ``strftime`` may raise :exc:`UnicodeError` or return an empty string
		instead.

encukou Dec 10, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Is this still true? According to test_strftime_special, these should be always preserved.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

encukou commented Dec 10, 2024

Copy link

Copy Markdown

Member

Could you add tests for the newly documented behaviour?
There is test_strftime_format_check that checks there is no crash, but it would be good to check the results match the platform -- if only to verify the docs.

(Let me know if you'd like me to continue this.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hoodmane commented Dec 11, 2024

Copy link

Copy Markdown

Contributor Author

What platforms with musl or BSD libc are supported? MacOS uses BSD and wasi uses musl and they both have buildbots. Then we could also use windows and glibc linux and add tests for the edge cases for each of these?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

encukou commented Dec 11, 2024

Copy link

Copy Markdown

Member

Think of the tests as a tool for maintainers of platforms that CPython doesn't support. If they fail, they'll know that they need a patch -- even if it's just for the docs.
And we can merge patches for unsupported platforms, if they aren't invasive.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hoodmane commented Dec 11, 2024

Copy link

Copy Markdown

Contributor Author

tool for maintainers of platforms that CPython doesn't support

Well we already have divergent behavior in the platforms that we do support. One reasonable approach would be to classify unsupported platforms as glibc-like, musl-like, bsd-like, or windows-like and assert that they agree in behavior with the most similar supported platform...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

encukou commented Dec 11, 2024

Copy link

Copy Markdown

Member

Maybe assert that unknown platforms behave like any known platform?
The list of behaviours in the docs looks exclusive.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hoodmane commented Dec 11, 2024

Copy link

Copy Markdown

Contributor Author

Okay. I will look into that.

encukou reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot commented Apr 20, 2026

Copy link

Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot added the stale Stale PR or inactive for long period of time. label

Apr 20, 2026

Reviewers

encukou

encukou left review comments

pganssle

Awaiting requested review from pganssle

pganssle is a code owner

abalkin

Awaiting requested review from abalkin

vstinner

Awaiting requested review from vstinner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review stale

Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

hoodmane commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nineteendo commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoodmane commented Dec 2, 2024

Uh oh!

nineteendo commented Dec 2, 2024

Uh oh!

hoodmane commented Dec 2, 2024

Uh oh!

hoodmane commented Dec 2, 2024

Uh oh!

hoodmane commented Dec 3, 2024

Uh oh!

nineteendo commented Dec 3, 2024

Uh oh!

encukou Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

encukou commented Dec 10, 2024

Uh oh!

hoodmane commented Dec 11, 2024

Uh oh!

encukou commented Dec 11, 2024

Uh oh!

hoodmane commented Dec 11, 2024

Uh oh!

encukou commented Dec 11, 2024

Uh oh!

hoodmane commented Dec 11, 2024

Uh oh!

github-actions Bot commented Apr 20, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hoodmane commented Dec 2, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

nineteendo commented Dec 2, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.