Skip to content

Conversation

@zanieb
Copy link
Contributor

@zaniebzanieb commented Dec 19, 2024

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 if strftime is not detected to be C99-compliant. If cross-compiling, we don't check if strftime 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.


@ghost
Copy link

ghost commented Dec 19, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

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 skip news label instead.

@zanieb

This comment was marked as outdated.

@bedevere-app
Copy link

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 skip news label instead.

@zaniebzanieb marked this pull request as ready for review December 20, 2024 00:16
@ZeroIntensity
Copy link
Member

I think this is probably blurb-worthy. We have a section for builds (but maybe this fits better under "library")

@zanieb
Copy link
ContributorAuthor

👍 I can add one. #123861 omitted the news entry but this change is definitely larger.

@zanieb
Copy link
ContributorAuthor

I believe that failure is a flake as those tests succeeded on the first commit.

@ZeroIntensity
Copy link
Member

Yeah, it's not related.

@picnixzpicnixz changed the title gh-128104: Remove Py_STRFTIME_C99_SUPPORT; require C99-compliant strftimegh-123681, gh-128104: Remove Py_STRFTIME_C99_SUPPORT; require C99-compliant strftimeDec 21, 2024
@picnixzpicnixz changed the title gh-123681, gh-128104: Remove Py_STRFTIME_C99_SUPPORT; require C99-compliant strftimegh-128104: Remove Py_STRFTIME_C99_SUPPORT; require C99-compliant strftimeDec 21, 2024
@picnixz
Copy link
Member

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.

@picnixz
Copy link
Member

This does not re-enable the strftime_y2k test for the JIT which was disabled in #124466

This file was removed in #127584 since aarch64 is now assumed to be supported. I don't know if the PGO builds were re-enabled for the JIT though and if this can be relevant.

@zanieb
Copy link
ContributorAuthor

@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 main?

@picnixz
Copy link
Member

picnixz commented Dec 22, 2024

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 main?

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 try to use the "update branch" button whenever I can. It merges main into my branch with a simple merge commit and that's fine.
  • If I can't do that, for instance because I work offline, I merge my main local branch into the branch so that I only have a single commit. Then I check the logs. If I have merged multiple commits because of --ff, then I rebase them. And then I wait until I can update my branch.
  • If I need to force-push something, I put my PR in draft (if I need time) or I just force-push just after. The cases where I need (and will) force-push are: when I really need to fixup a commit because I forgot something (which, without this additional change, the review for this specific commit would be wrong!!!), when I need to amend a commit message because it is wrong (i.e. it lies) or when I need to drop a commit I really don't want someone to see (e.g., it's an error on my side, or something should never have been done).

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.

@zanieb
Copy link
ContributorAuthor

Thanks @picnixz — I'll keep that all in mind.

@zanieb
Copy link
ContributorAuthor

@erlend-aasland gentle reminder on this one — let me know if you need anything.

@picnixz
Copy link
Member

cc @serhiy-storchaka (who re-opened my other issue to suggest a runtime check instead of a compile-time check)

@serhiy-storchaka
Copy link
Member

@blhsing, please remind, why was the %F and %C support was conditional at first place?

In any case, I proposed to make the Py_NORMALIZE_CENTURY check at runtime instead of a compile time. This is needed for cross-compilation. It would also be easy to check for %F and %C support at runtime.

@zanieb
Copy link
ContributorAuthor

zanieb commented Jan 2, 2025

I'm all for runtime checks, though if C11 is required I don't see why we need to check for C99 features? I only left the check here since it's low-maintenance and not complicating the runtime.

ref #123861 (comment) cc @zware

@erlend-aasland
Copy link
Contributor

Totally agree with @zanieb and Zach; there is no need to complicate the runtime with such obsolete checks.

@erlend-aasland

This comment was marked as resolved.

@zanieb

This comment was marked as resolved.

@zaniebzaniebforce-pushed the zb/py-strftime-c99 branch from 47a047f to 629af8dCompareJanuary 2, 2025 21:24
@erlend-aaslanderlend-aasland merged commit bb2dfad into python:mainJan 3, 2025
41 checks passed
@erlend-aasland
Copy link
Contributor

Thanks, Zanie! It's good to get rid of cruft like this; thanks for all the good work you're doing!

Sign up for freeto 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.

test_strftime_y2k fails while cross-compiling 3.14a3 for x86_64_v2 and x86_64_v3 on Linux test_strftime_y2k fails on embedded Linux

7 participants

@zanieb@ZeroIntensity@picnixz@erlend-aasland@serhiy-storchaka@zware@Eclips4