Skip to content
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-123681: handle c99-specific specifiers for strftime when cross-compiling #123861

Closed

Conversation

blhsing
Copy link
Contributor

@blhsing blhsing commented Sep 9, 2024

@blhsing blhsing changed the title gh-123681: handle c99-specific specifiers when cross-compiling gh-123681: handle c99-specific specifiers for strftime when cross-compiling Sep 9, 2024
@blhsing blhsing marked this pull request as ready for review September 9, 2024 10:28
@blhsing
Copy link
Contributor Author

blhsing commented Sep 9, 2024

Hmm how do I trigger the JIT checks?

@picnixz
Copy link
Member

picnixz commented Sep 9, 2024

Hmm how do I trigger the JIT checks?

Change a line in any file matching Python/optimizer*.c (ok that's an ugly hack but I can't run it manually for you I think)

@blhsing blhsing requested a review from markshannon as a code owner September 9, 2024 13:05
@picnixz picnixz removed the request for review from markshannon September 9, 2024 13:07
@picnixz
Copy link
Member

picnixz commented Sep 9, 2024

@blhsing Ok, I think I know how to run the JIT manually:

  • Go to your own fork.
  • Open the Actions tab.
  • Expand the workflows list and locate the JIT one.
  • Manually trigger it on the branch you want.

@blhsing
Copy link
Contributor Author

blhsing commented Sep 9, 2024

@blhsing Ok, I think I know how to run the JIT manually:

  • Go to your own fork.
  • Open the Actions tab.
  • Expand the workflows list and locate the JIT one.
  • Manually trigger it on the branch you want.

Ah I see. Thanks a lot for the tips. Will definitely do it that way next time!

With the optimizer.c hack though the commit did pass the JIT checks so I'm reverting the hack now.

@serhiy-storchaka would you help review and possibly merge the PR? Thanks!

@zware
Copy link
Member

zware commented Sep 24, 2024

I'm not sure we can make just this change. AC_RUN_IFELSE takes input, action-if-true, action-if-false, and action-if-cross-compiling; this is changing action-if-cross-compiling to unconditionally set ac_cv_strftime_c99_support=yes. We can only do that if we can unconditionally set that in any case, so we might as well remove the whole check. I think we should be able to do that; IIUC we currently require C11.

@zware
Copy link
Member

zware commented Sep 24, 2024

Checking ./configure logs for recent buildbot builds, I see no indication that any platform ever gets a false result from the configure check (other than in cross-compiles), so I really see no reason to keep it.

@blhsing
Copy link
Contributor Author

blhsing commented Sep 25, 2024

We can only do that if we can unconditionally set that in any case, so we might as well remove the whole check. I think we should be able to do that; IIUC we currently require C11.

Very true. There's no need to check for C99 support since CPython now requires C11. Will remove the check entirely then.

@erlend-aasland
Copy link
Contributor

Very true. There's no need to check for C99 support since CPython now requires C11. Will remove the check entirely then.

For inspiration, take a look at how we detect libm C99 features around L6000. FTR, there's a few other places configure.ac where we can tear out C99 stuff.

@zanieb
Copy link
Contributor

zanieb commented Dec 19, 2024

@erlend-aasland Hey! I'm following up on this because I ran into the problem in #128104. When you say

For inspiration, take a look at how we detect libm C99 features around L6000.

Are you suggesting we continue to test if strftime behaves in a C99-compliant manner or just drop that check entirely? I don't quite follow how we could do the same thing was is done for libm.

edit: Followed up with a patch in #128106 — happy to chat there

@picnixz
Copy link
Member

picnixz commented Dec 21, 2024

Considering this PR has been stale for two months and that the other PR decides on a cleaner approach IMO, I'd suggest closing this one (I've also closed the corresponding issue I've opened since it's no more up-to-date).

@erlend-aasland
Copy link
Contributor

I agree with Bénédikt. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants