GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-95299: Rework test_cppext.py to not invoke setup.py directly by pradyunsg · Pull Request #103316 · python/cpython

Conversation

@pradyunsg

@pradyunsg pradyunsg commented Apr 6, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

  • Issue: Remove bundled setuptools #95299

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pradyunsg
pythongh-95299: Rework test_cppext.py to not invoke setup.py directly
41c5a66
@bedevere-bot bedevere-bot mentioned this pull request Apr 6, 2023
Remove bundled setuptools #95299
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Apr 6, 2023
@pradyunsg pradyunsg mentioned this pull request Apr 6, 2023
gh-95299: Stop installing setuptools as a part of ensurepip and venv #101039
Merged
zware
zware reviewed Apr 6, 2023
View reviewed changes
Comment thread Lib/test/cppextdata/setup.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@zware zware added the skip news label Apr 6, 2023
@pradyunsg
Add tests/cppextdata data to TESTSUBDIRS
635492e
@zware

zware commented Apr 6, 2023

Copy link
Copy Markdown
Member

!buildbot (?i).installed.
merwok reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Apr 6, 2023

Copy link
Copy Markdown

🤖 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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pradyunsg added 4 commits April 6, 2023 19:22
@pradyunsg
Revert "Add tests/cppextdata data to TESTSUBDIRS"
63d691c 
This reverts commit 635492e.
@pradyunsg
Revert "pythongh-95299: Rework test_cppext.py to not invoke setup.py …
d79d6a6 
…directly"

This reverts commit 41c5a66.
@pradyunsg
Build and install the extension in a temporary directory instead
7c40f3c
@pradyunsg
Merge branch 'main' into test-cppext
f6c4024
@pradyunsg

pradyunsg commented Apr 6, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pradyunsg

pradyunsg commented Apr 6, 2023

Copy link
Copy Markdown
Member Author

!buildbot (?i).installed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Apr 6, 2023

Copy link
Copy Markdown

🤖 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:
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pradyunsg

pradyunsg commented Apr 6, 2023

Copy link
Copy Markdown
Member Author

!buildbot (?i).installed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Apr 6, 2023

Copy link
Copy Markdown

🤖 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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@CAM-Gerlach

CAM-Gerlach commented Apr 6, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zware
zware reviewed Apr 6, 2023
View reviewed changes
Comment thread Lib/test/test_cppext.py Outdated
@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

@zware zware Apr 6, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pradyunsg pradyunsg Apr 7, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pradyunsg pradyunsg Apr 7, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pradyunsg
Pull in wheels for setuptools and wheel for testing extension builds
dd16041
@pradyunsg

pradyunsg commented Apr 8, 2023

Copy link
Copy Markdown
Member Author

!buildbot (?i).installed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Apr 8, 2023

Copy link
Copy Markdown

🤖 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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
FFY00
FFY00 approved these changes Apr 11, 2023
View reviewed changes

@FFY00 FFY00 left a comment

Copy link
Copy Markdown
Member

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 👍.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Apr 11, 2023
@pradyunsg
pradyunsg merged commit 9e67740 into python:main Apr 13, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label 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
@carljm
Merge branch 'main' into superopt
9384106 
* 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
@pradyunsg @aisk
pythongh-95299: Rework test_cppext.py to not invoke setup.py directly (…
a702bcd 
…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
@pradyunsg pradyunsg mentioned this pull request May 18, 2023
PEP 632: Remove the distutils package (Python 3.12) #92584
Closed
@arhadthedev arhadthedev mentioned this pull request Jun 19, 2023
Deprecate and remove distutils #85454
Closed
@sobolevn sobolevn mentioned this pull request Jan 23, 2024
gh-108303: Move .whl test files to Lib/test/whldata/ #114343
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zware zware zware left review comments
@FFY00 FFY00 FFY00 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@pradyunsg @zware @bedevere-bot @CAM-Gerlach @FFY00
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.