-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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
gh-128104: Remove Py_STRFTIME_C99_SUPPORT
; require C99-compliant strftime
#128106
base: main
Are you sure you want to change the base?
Conversation
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
This comment was marked as outdated.
This comment was marked as outdated.
f82b179
to
68fea4b
Compare
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
[ac_cv_strftime_c99_support=no])]) | ||
if test "$ac_cv_strftime_c99_support" = yes | ||
[ac_cv_strftime_c99_support=yes])]) | ||
if test "$ac_cv_strftime_c99_support" = no | ||
then | ||
AC_DEFINE([Py_STRFTIME_C99_SUPPORT], [1], | ||
[Define if C99-specific strftime specifiers are supported.]) | ||
AC_MSG_ERROR([Python requires C99-compatible strftime specifiers]) | ||
fi |
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.
I tried a couple different approaches to avoid ac_cv_strftime_c99_support
entirely or avoid using =yes
when cross-compiling but don't really know how to use autoconf yet. If a different approach is desired, lmk.
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.
This seems fine to me; if the check fails, we can't build.
However, we should also be fine with just removing this check. As you already pointed out, we require C99 support.
(I'm not sure what I meant about the libm
reference in the other PR 😆)
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.
Agree — I don't think it's a big deal either way. It seems easy to remove if it's causing a problem.
I think this is probably blurb-worthy. We have a section for builds (but maybe this fits better under "library") |
👍 I can add one. #123861 omitted the news entry but this change is definitely larger. |
I believe that failure is a flake as those tests succeeded on the first commit. |
Yeah, it's not related. |
Misc/NEWS.d/next/Build/2024-12-20-09-03-22.gh-issue-128104.m_SoVx.rst
Outdated
Show resolved
Hide resolved
Py_STRFTIME_C99_SUPPORT
; require C99-compliant strftimePy_STRFTIME_C99_SUPPORT
; require C99-compliant strftime
Py_STRFTIME_C99_SUPPORT
; require C99-compliant strftimePy_STRFTIME_C99_SUPPORT
; require C99-compliant strftime
Ok, I eventually decided that we can keep your issue opened instead of mine because there's a bit more details. It'd be better not to have two issues in the title as well (sorry for the title double edit). I'll close mine. |
66135e2
to
0a9e225
Compare
Just a heads-up, @zanieb: please use |
@erlend-aasland can do. It seems like the devguide suggests that you shouldn't ever amend commits? Is it not a priority to have discrete commits in the pull request? Are you just referring to merging |
We don't really care about having merge commits since we anyway squash everything together at the end and change the commit message. However, we allow (at least I) to amend commits in some situations. My personal rules (which are not the official ones, but that I use) are:
I avoid force-pushing when I can, but sometimes the situation requires you to force-push. Considering the time and the window between two commits I usually have, my force-pushes are fine because we can still follow from the last reviewed commit but if I wait before force-pushing, someone could already have reviewed something and everything's lost! Now, this is something I've been doing less and less because we live in different timezones and people might be very reactive! (or already reviewing the PR, then they see the "Refresh" button, hit it, and then the review becomes messed up because of my force-push). So, I always try to first clean my commits locally before pushing them in one go. A final note: unless I've marked my PR as ready for review, I assume I can do whatever I want. Victor told me that he doesn't review draft PRs until they are ready and I assume that it's because we can have huge commit activity. Obviously, this is assuming the PR was opened as a draft. If I mark it as a draft during the review process, I still try to reduce commit activity and force-pushing just for the sake of reviewers. Overall, force-pushing is a matter of taste. I personally prefer having a force-push rather a wrong commit that reviewers will view. However, if the "wrong" in the commit is something like a typo or a compilation failure, I don't bother because reviewers don't review commits locally but on a web UI. So I just add a commit "fix compilation" or "fix typo" for instance. Most of the time, force-pushing for having one single commit at the end is not recommended (and should be avoided) but force-pushing for cleaning something that was dirty and would make reviews incorrect or incorrectly lead reviewers is probably preferred, at least to me. |
Thanks @picnixz — I'll keep that all in mind. |
Supersedes #123861
Closes #123681
Closes #128104
Since C11 is required now, there's no need to toggle behavior based on C99-compliant behavior in
strftime
. Instead, we fail at configure time ifstrftime
is not detected to be C99-compliant. If cross-compiling, we don't check ifstrftime
is compliant.We could remove the check entirely, but it seems nice to check when we can. I loosely modeled this after the
libm
check.This does not re-enable the
strftime_y2k
test for the JIT which was disabled in #124466 — I don't see that file :) I did a JIT test run anyway.test_strftime_y2k
fails while cross-compiling 3.14a3 forx86_64_v2
andx86_64_v3
on Linux #128104test_strftime_y2k
fails on embedded Linux #123681