Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.1k
gh-95299: Rework test_cppext.py to not invoke setup.py directly#103316
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
!buildbot (?i).installed. |
bedevere-bot commentedApr 6, 2023
🤖 New build scheduled with the buildbot fleet by@zware for commit635492e 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
pradyunsg commentedApr 6, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 |
!buildbot (?i).installed. |
bedevere-bot commentedApr 6, 2023
🤖 New build scheduled with the buildbot fleet by@pradyunsg for commitf6c4024 🤖 The command will test the builders whose names match following regular expression: The builders matched are: |
!buildbot (?i).installed. |
bedevere-bot commentedApr 6, 2023
🤖 New build scheduled with the buildbot fleet by@pradyunsg for commitf6c4024 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
CAM-Gerlach commentedApr 6, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Just to cross reference for others' benefit, previous buildbot discussion:#101039 (comment) |
Lib/test/test_cppext.py Outdated
@@ -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 |
There was a problem hiding this comment.
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 grabsetuptools
. I think we'd be better off stashing asetuptools
wheel (probably the one currently used byensurepip
:)) 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 ofsetuptools
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.
There was a problem hiding this comment.
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 forsetuptools
(andwheel
) 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.
There was a problem hiding this comment.
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.
!buildbot (?i).installed. |
bedevere-bot commentedApr 8, 2023
🤖 New build scheduled with the buildbot fleet by@pradyunsg for commitdd16041 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
There was a problem hiding this 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 👍.
* 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)
…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 commit635492e.* Revert "pythongh-95299: Rework test_cppext.py to not invoke setup.py directly"This reverts commit41c5a66.* Build and install the extension in a temporary directory instead* Pull in wheels for setuptools and wheel for testing extension builds
Uh oh!
There was an error while loading.Please reload this page.
Pulling these changes out of#101039, as requested by@vstinner. :)