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

Merged
merged 7 commits into from
Apr 13, 2023

Conversation

pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Apr 6, 2023

Pulling these changes out of #101039, as requested by @vstinner. :)

Lib/test/cppextdata/setup.py Outdated Show resolved Hide resolved
@zware zware added the skip news label Apr 6, 2023
@zware
Copy link
Member

zware commented Apr 6, 2023

!buildbot (?i).installed.

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @zware for commit 635492e 🤖

The command will test the builders whose names match following regular expression: (?i).*installed.*

The builders matched are:

  • AMD64 Fedora Stable Clang Installed PR
  • s390x Fedora Clang Installed PR
  • aarch64 Fedora Stable Clang Installed PR
  • PPC64LE Fedora Stable Clang Installed PR
  • x86 Gentoo Installed with X PR

@pradyunsg
Copy link
Member Author

pradyunsg commented Apr 6, 2023

Hmm... I didn't intend for the reverts to make it into the PR, but oh well, they're here now. 😅

The main change is that it's no longer moving the files around, and instead using the temporary directory created for the build within the outer-caller of the test; this serves as a temporary directory for the package to be built and installed via a regular pip install run without relying on setuptools directly.

@pradyunsg
Copy link
Member Author

!buildbot (?i).installed.

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pradyunsg for commit f6c4024 🤖

The command will test the builders whose names match following regular expression: (?i).installed.

The builders matched are:

@pradyunsg
Copy link
Member Author

!buildbot (?i).installed.

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pradyunsg for commit f6c4024 🤖

The command will test the builders whose names match following regular expression: (?i).*installed.*

The builders matched are:

  • AMD64 Fedora Stable Clang Installed PR
  • s390x Fedora Clang Installed PR
  • aarch64 Fedora Stable Clang Installed PR
  • PPC64LE Fedora Stable Clang Installed PR
  • x86 Gentoo Installed with X PR

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Apr 6, 2023

Just to cross reference for others' benefit, previous buildbot discussion: #101039 (comment)

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

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

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.

@pradyunsg
Copy link
Member Author

!buildbot (?i).installed.

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pradyunsg for commit dd16041 🤖

The command will test the builders whose names match following regular expression: (?i).*installed.*

The builders matched are:

  • AMD64 Fedora Stable Clang Installed PR
  • s390x Fedora Clang Installed PR
  • aarch64 Fedora Stable Clang Installed PR
  • PPC64LE Fedora Stable Clang Installed PR
  • x86 Gentoo Installed with X PR

Copy link
Member

@FFY00 FFY00 left a comment

Choose a reason for hiding this comment

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

I think ideally we'd want to get rid of setuptools entirely here, but this is a reasonable solution while that doesn't happen (it's not trivial to implement). The implementation itself looks good to me, so you have my 👍.

@pradyunsg pradyunsg merged commit 9e67740 into python:main Apr 13, 2023
@pradyunsg pradyunsg deleted the test-cppext branch April 13, 2023 04:17
carljm added a commit to carljm/cpython that referenced this pull request Apr 13, 2023
* main:
  pythongh-103479: [Enum] require __new__ to be considered a data type (pythonGH-103495)
  pythongh-103365: [Enum] STRICT boundary corrections (pythonGH-103494)
  pythonGH-103488: Use return-offset, not yield-offset. (pythonGH-103502)
  pythongh-103088: Fix test_venv error message to avoid bytes/str warning (pythonGH-103500)
  pythonGH-103082: Turn on branch events for FOR_ITER instructions. (python#103507)
  pythongh-102978: Fix mock.patch function signatures for class and staticmethod decorators (python#103228)
  pythongh-103462: Ensure SelectorSocketTransport.writelines registers a writer when data is still pending (python#103463)
  pythongh-95299: Rework test_cppext.py to not invoke setup.py directly (python#103316)
aisk pushed a commit to aisk/cpython that referenced this pull request Apr 18, 2023
…python#103316)

* pythongh-95299: Rework test_cppext.py to not invoke setup.py directly

* Add tests/cppextdata data to `TESTSUBDIRS`

* Revert "Add tests/cppextdata data to `TESTSUBDIRS`"

This reverts commit 635492e.

* Revert "pythongh-95299: Rework test_cppext.py to not invoke setup.py directly"

This reverts commit 41c5a66.

* Build and install the extension in a temporary directory instead

* Pull in wheels for setuptools and wheel for testing extension builds
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