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-105813: Autoconf: add and use PY_DEFINE macro #128525

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jan 5, 2025

@erlend-aasland
Copy link
Contributor Author

Finally got around to look at this again. I think this may be a good start, @encukou. Re. capi-workgroup/problems#46. I excluded defines like _NETBSD_SOURCE and similar. Perhaps we should limit this PR to only HAVE_ and WITH_ defines for now.

@erlend-aasland erlend-aasland changed the title gh-105813: WIP Autoconf: add and use PY_DEFINE macro gh-105813: Autoconf: add and use PY_DEFINE macro Jan 5, 2025
@erlend-aasland erlend-aasland requested a review from encukou January 7, 2025 00:04
@encukou
Copy link
Member

encukou commented Jan 7, 2025

That does look like a great start! Not familiar enough with Autotools to comment on the implementation.
But, it someone who's not an Autotools expert can now experiment solving the other issues (naming scheme and migration path).

I don't think limiting this to Py_HAVE_ and Py_WITH is worth it. We might want to change the prefix anyway (perhaps to _PY_ -- https://discuss.python.org/t/py-prefix-for-if-friendly-macros/60825 and private so we can remove without deprecation).
Perhaps the ones other than Py_HAVE_ & Py_WITH_ need another common prefix (PyPlatform_?)... but if they end up private, probably not.

One thing I'm worried about is keeping the two sets of names in sync. I'm not sure if/how a redistributor can set something like POSIX_SEMAPHORES_NOT_ENABLED, and how that is propagated to the Py_-prefixed version.

@@ -53,6 +53,16 @@ AC_DEFUN([WITH_SAVE_ENV],
[RESTORE_ENV]
)dnl

dnl AC_DEFINE*() wrappers.
AC_DEFUN([PY_DEFINE], [dnl
AC_DEFINE([[Py_]$1], [$2], [$3])dnl
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, do you want to allow defining private macros (namely _Py) or not?

dnl AC_DEFINE*() wrappers.
AC_DEFUN([PY_DEFINE], [dnl
AC_DEFINE([[Py_]$1], [$2], [$3])dnl
AC_DEFINE([$1], [$2], [$3])dnl
Copy link
Contributor

Choose a reason for hiding this comment

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

If you change the Py macro for whatever reason, the non-prefixed Py_ macro would not be changed. Instead, it should be aliased to [Py_]#1 directly.

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.

3 participants