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-91321: Add _testcppext C++ extension #32175

Merged
merged 2 commits into from
May 2, 2022
Merged

gh-91321: Add _testcppext C++ extension #32175

merged 2 commits into from
May 2, 2022

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Mar 29, 2022

Build a C++ test extension to check that the Python C API is
compatible with C++ and does not emit C++ compiler warnings.

Add test_cppext.py and setup_testcppext.py to Lib/test/.

https://bugs.python.org/issue39355

@vstinner
Copy link
Member Author

Quick & dirty hack of the Python C API (Include/ header files) to make it compatible with C++.

By the way, using NULL in a macro doesn't emit a warning even with clang -Wzero-as-null-pointer-constant. It's a clang feature! Extract of a LLVM clang comment:

// If it is a macro from system header, and if the macro name is not "NULL",
// do not warn

@iritkatriel
Copy link
Member

https://bugs.python.org/issue39355 is closed. What is the status of this PR?

@vstinner
Copy link
Member Author

This PR is now for https://bugs.python.org/issue47165

I'm working on fixing C++ compiler warnings: https://bugs.python.org/issue47164

@vstinner vstinner changed the title bpo-39355: WIP: Add _testcppext C++ extension gh-91320: WIP: Add _testcppext C++ extension Apr 26, 2022
@vstinner
Copy link
Member Author

I updated this PR for #91320

@vstinner vstinner changed the title gh-91320: WIP: Add _testcppext C++ extension gh-91321: WIP: Add _testcppext C++ extension Apr 26, 2022
@vstinner
Copy link
Member Author

Oops, sorry, this PR is for #91321

@vstinner
Copy link
Member Author

I created #91959 for the "CAST" commit.

@vstinner vstinner changed the title gh-91321: WIP: Add _testcppext C++ extension gh-91321: Add _testcppext C++ extension Apr 27, 2022
@vstinner vstinner marked this pull request as ready for review April 27, 2022 10:15
@vstinner
Copy link
Member Author

Oh, the build fails with MSVC:

Error: Modules/_testcppext.cpp(229): error C7555: use of designated initializers requires at least '/std:c++20'
test test_cppext failed
Error: Modules/_testcppext.cpp(244): error C7555: use of designated initializers requires at least '/std:c++20'
Error: Modules/_testcppext.cpp(364): error C7555: use of designated initializers requires at least '/std:c++20'

I initialize structures with .attr = value syntax:

static PyType_Spec Xxo_Type_spec = {
    .name = "_testcppext.Xxo",
    .basicsize = sizeof(XxoObject),
    .itemsize = 0,
    .flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC,
    .slots = Xxo_Type_slots,
};

@vstinner
Copy link
Member Author

Tests / Windows (x64) (pull_request) Failing after 19m

Error:

LINK : fatal error LNK1104: cannot open file 'python311.lib'

@steve-s
Copy link
Contributor

steve-s commented Apr 28, 2022

FYI we were/are trying to address the same issue with HPy headers. My understanding is that this only tests the static inline functions and macros that are used in the example extension. It is still possible that some other macro or static inline function could cause warnings or for macros even errors. So my conclusion so far has been that to have a fully C++ compat tested headers, one has to write a C++ extension(s) that use all the static inline functions and macros from the headers. Something tested is way better than nothing tested, so no objections to this PR -- just wanted to check if you see it the same way?

@vstinner
Copy link
Member Author

@steve-s:

FYI we were/are trying to address the same issue with HPy headers. My understanding is that this only tests the static inline functions and macros that are used in the example extension. It is still possible that some other macro or static inline function could cause warnings or for macros even errors. So my conclusion so far has been that to have a fully C++ compat tested headers, one has to write a C++ extension(s) that use all the static inline functions and macros from the headers. Something tested is way better than nothing tested, so no objections to this PR -- just wanted to check if you see it the same way?

Sadly, my experience is the same: a C++ compiler doesn't complain when a static inline function is defined. Only when it's being used.

My practical plan is:

  • Write a bare minimum C++ extension which does nothing and check in the Python test suite that we can build it for C++11.
  • First, I tried to load the extension but my code didn't work when Python is installed. Maybe we can try to load and test the built C++ extension later. But for now, I prefer to skip that.
  • Slowly test more and more static inline functions in this C++ extension.
  • Test more C++ versions.

Right now, my main blocker issue is to test if Python has a working C++ compiler. For example, the "Tests / Windows (x64)" fails with cannot open file 'python311.lib'. This CI has a C++ compiler but not the required python library to link the C++ extension :-( Maybe it's just a matter of using the right compiler command line. I don't know.

@vstinner vstinner added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 29, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @vstinner for commit 31b7a4f7ab6092998865e06dd0f4b0d5ea79b13f 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 29, 2022
@vstinner
Copy link
Member Author

I simplified Modules/_testcppext.cpp to make it as small as possible. I prefer to start with a small extension and then extend it slowly. As I wrote, right now, my practical issue is more to ensure that there is a working C++ compiler all on CIs used by Python.

@vstinner
Copy link
Member Author

Oh, when Python is installed, the test fails with:

error: could not create 'build': Permission denied

For example, it failed on these buildbots:

  • AMD64 Fedora Stable Clang Installed PR
  • s390x Fedora Clang Installed PR

Build a basic C++ test extension to check that the Python C API is
compatible with C++ and does not emit C++ compiler warnings.

* Add Modules/_testcppext.cpp: C++ extension
* Add Lib/test/test_cppext.py: test building the C++ extension.
@vstinner
Copy link
Member Author

I rewrote the PR to build the C++ extension in a temporary directory and fix the test on installed Python: _testcppext.cpp is now part of Lib/test/ and so is installed.

@vstinner vstinner added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 2, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @vstinner for commit e067677 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 2, 2022
@vstinner
Copy link
Member Author

vstinner commented May 2, 2022

buildbot/s390x RHEL7 PR — Build done.

The error is unrelated to my change:

1 test altered the execution environment: test_threading

@encukou
Copy link
Member

encukou commented Jul 12, 2022

reinterpret_cast breaks existing code, see #94731.
#94782 is a PR to revert back to C-style casts.

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.

6 participants