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.

Sign up for GitHub

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-95299: Rework test_cppext.py to not invoke setup.py directly #103316

Merged
merged 7 commits into from
Apr 13, 2023
Merged

gh-95299: Rework test_cppext.py to not invoke setup.py directly #103316

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
41c5a66
gh-95299: Rework test_cppext.py to not invoke setup.py directly
pradyunsg Apr 6, 2023
635492e
Add tests/cppextdata data to `TESTSUBDIRS`
pradyunsg Apr 6, 2023
63d691c
Revert "Add tests/cppextdata data to `TESTSUBDIRS`"
pradyunsg Apr 6, 2023
d79d6a6
Revert "gh-95299: Rework test_cppext.py to not invoke setup.py directly"
pradyunsg Apr 6, 2023
7c40f3c
Build and install the extension in a temporary directory instead
pradyunsg Apr 6, 2023
f6c4024
Merge branch 'main' into test-cppext
pradyunsg Apr 6, 2023
dd16041
Pull in wheels for setuptools and wheel for testing extension builds
pradyunsg Apr 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions Lib/test/setup_testcppext.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# gh-91321: Build a basic C++ test extension to check that the Python C API is
# compatible with C++ and does not emit C++ compiler warnings.
import os
import sys
from test import support

Expand All @@ -25,14 +26,8 @@

def main():
cppflags = list(CPPFLAGS)
if '-std=c++03' in sys.argv:
sys.argv.remove('-std=c++03')
std = 'c++03'
name = '_testcpp03ext'
else:
# Python currently targets C++11
std = 'c++11'
name = '_testcpp11ext'
std = os.environ["CPYTHON_TEST_CPP_STD"]
name = os.environ["CPYTHON_TEST_EXT_NAME"]

cppflags = [*CPPFLAGS, f'-std={std}']

Expand Down
28 changes: 18 additions & 10 deletions Lib/test/test_cppext.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
# gh-91321: Build a basic C++ test extension to check that the Python C API is
# compatible with C++ and does not emit C++ compiler warnings.
import os.path
import shutil
try:
import ssl
except ImportError:
ssl = None
import sys
import unittest
import subprocess
Expand Down Expand Up @@ -31,6 +36,8 @@ def test_build_cpp03(self):
@unittest.skipIf(
'-fsanitize' in (sysconfig.get_config_var('PY_CFLAGS') or ''),
'test does not work with analyzing builds')
# the test uses pip which needs a TLS connection to PyPI
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we want to make this test reach out to PyPI to grab setuptools. I think we'd be better off stashing a setuptools wheel (probably the one currently used by ensurepip :)) somewhere and installing it before using it to build the test module; that avoids network traffic and, more importantly, gives us control over the version of setuptools without the obligation to keep it up to date for distribution.

The other option would be to just enshrine a couple of golden compiler commands, and if they don't work 🤷‍♂️ oh well they probably worked on a different platform. This test already has a ton of skip possibilities anyway.

Copy link
Member Author

@pradyunsg pradyunsg Apr 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Adding a x-ref to #101039 (comment) which lists the three boolean questions that determine which of the 6-8 (ish) possible approaches we could take here. Opinions/inputs welcome on what we want the answers to be.

From what I'm reading, you'd prefer that we don't reach out over the network. If it's preferable to just assume we can rely on setuptools and completely circumvent pip, that works. If it's preferable to have hard-coded paths compiler invocations and avoid any/all packaging tooling in the test suite, that also works for me.

To that end, I've gone ahead and added wheels for setuptools (and wheel) into the repository -- they're both necessary, assuming we want to continue having the build processes avoid reaching out to PyPI as newer versions of pip come out.

Copy link
Member Author

@pradyunsg pradyunsg Apr 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

TBH, I'd prefer to defer crafting golden compiler commands to a follow up. It is likely a better solution but it would end up being more than I'd wanna do for this.

It's not something that's immediately clear to me how we'd do in the current style of testing (this is, AFAICT, the only test that compiles something) and, IMO, it's likely better to have a test based off of something that's using battle-tested compiler lookup+invocation logic and then trim scope of it if we find that undesirable.

@unittest.skipIf(ssl is None, 'No ssl module')
# the test uses venv+pip: skip if it's not available
@support.requires_venv_with_pip()
def check_build(self, std_cpp03, extension_name):
Expand All @@ -39,6 +46,10 @@ def check_build(self, std_cpp03, extension_name):
self._check_build(std_cpp03, extension_name)

def _check_build(self, std_cpp03, extension_name):
pkg_dir = 'pkg'
os.mkdir(pkg_dir)
shutil.copy(SETUP_TESTCPPEXT, os.path.join(pkg_dir, "setup.py"))

venv_dir = 'env'
verbose = support.verbose

Expand All @@ -59,11 +70,15 @@ def _check_build(self, std_cpp03, extension_name):
python = os.path.join(venv_dir, 'bin', python_exe)

def run_cmd(operation, cmd):
env = os.environ.copy()
env['CPYTHON_TEST_CPP_STD'] = 'c++03' if std_cpp03 else 'c++11'
env['CPYTHON_TEST_EXT_NAME'] = extension_name
if verbose:
print('Run:', ' '.join(cmd))
subprocess.run(cmd, check=True)
subprocess.run(cmd, check=True, env=env)
else:
proc = subprocess.run(cmd,
env=env,
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
text=True)
Expand All @@ -72,16 +87,9 @@ def run_cmd(operation, cmd):
self.fail(
f"{operation} failed with exit code {proc.returncode}")

# Build the C++ extension
cmd = [python, '-X', 'dev',
SETUP_TESTCPPEXT, 'build_ext', '--verbose']
if std_cpp03:
cmd.append('-std=c++03')
run_cmd('Build', cmd)

# Install the C++ extension
# Build and install the C++ extension
cmd = [python, '-X', 'dev',
SETUP_TESTCPPEXT, 'install']
'-m', 'pip', 'install', os.path.abspath(pkg_dir)]
run_cmd('Install', cmd)

# Do a reference run. Until we test that running python
Expand Down