Skip to content

Handle trailing % in strftime#23045

Merged
sbc100 merged 2 commits into
emscripten-core:mainfrom
hoodmane:strftime-trailing-percent
Dec 4, 2024
Merged

Handle trailing % in strftime#23045
sbc100 merged 2 commits into
emscripten-core:mainfrom
hoodmane:strftime-trailing-percent

Conversation

@hoodmane

@hoodmane hoodmane commented Dec 2, 2024

Copy link
Copy Markdown
Collaborator

"In glibc a trailing % in strftime() acts like printf, ie it's a literal %". This is breaking some Python tests. I've applied the patch suggested here: https://www.openwall.com/lists/musl/2022/12/19/2

"In glibc a trailing % in strftime() acts like printf, ie it's a literal %".
This is breaking some Python tests. I've applied the patch suggested here:
https://www.openwall.com/lists/musl/2022/12/19/2
@hoodmane

hoodmane commented Dec 2, 2024

Copy link
Copy Markdown
Collaborator Author

I guess this would have to be addressed upstream with musl though.

@sbc100 sbc100 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm % comment

Comment thread system/lib/libc/musl/src/time/strftime.c
@sbc100

sbc100 commented Dec 3, 2024

Copy link
Copy Markdown
Collaborator

Perhaps the python tests themselves should be updated? Otherwise they cannot run on any musl-based platform (e.g. alpine linux).

@hoodmane

hoodmane commented Dec 3, 2024

Copy link
Copy Markdown
Collaborator Author

Generally I try to fix both sides =)
python/cpython#127528

@hoodmane

hoodmane commented Dec 3, 2024

Copy link
Copy Markdown
Collaborator Author

I think it works on other musl systems as long as they support wide characters. The problem affects systems that use musl or bsd libc and where HAVE_WCSFTIME is false.

@sbc100

sbc100 commented Dec 3, 2024

Copy link
Copy Markdown
Collaborator

Ah.. In that case we can/should also be including wcsftime.. i don't know why we don't already include that.

@hoodmane

hoodmane commented Dec 3, 2024

Copy link
Copy Markdown
Collaborator Author

I looked into turning that on too but I couldn't figure out how it was determined. But I think this change is an improvement in either case.

sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 3, 2024
@sbc100

sbc100 commented Dec 3, 2024

Copy link
Copy Markdown
Collaborator

This change lgtm with comments addressed.

@sbc100

sbc100 commented Dec 3, 2024

Copy link
Copy Markdown
Collaborator

wcsftime being added in #23061

sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 3, 2024
sbc100 added a commit that referenced this pull request Dec 3, 2024
@hoodmane

hoodmane commented Dec 4, 2024

Copy link
Copy Markdown
Collaborator Author

Okay I addressed the comments I think.

@sbc100
sbc100 merged commit f526386 into emscripten-core:main Dec 4, 2024
@hoodmane
hoodmane deleted the strftime-trailing-percent branch December 5, 2024 09:04
hedwigz pushed a commit to hedwigz/emscripten that referenced this pull request Dec 18, 2024
hedwigz pushed a commit to hedwigz/emscripten that referenced this pull request Dec 18, 2024
"In glibc a trailing % in strftime() acts like printf, ie it's a literal
%". This is breaking some Python tests. I've applied the patch suggested
here: https://www.openwall.com/lists/musl/2022/12/19/2
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.

2 participants