Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
pradyunsg merged 7 commits intopython:mainfrompradyunsg:test-cppext
Apr 13, 2023

Conversation

pradyunsg
Copy link
Member

@pradyunsgpradyunsg commentedApr 6, 2023
edited by bedevere-bot
Loading

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

@zware
Copy link
Member

!buildbot (?i).installed.

merwok reacted with eyes emoji

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@zware for commit635492e 🤖

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
MemberAuthor

pradyunsg commentedApr 6, 2023
edited
Loading

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 regularpip install run without relying on setuptools directly.

@pradyunsg
Copy link
MemberAuthor

!buildbot (?i).installed.

@bedevere-bot
Copy link

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

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

The builders matched are:

@pradyunsg
Copy link
MemberAuthor

!buildbot (?i).installed.

@bedevere-bot
Copy link

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

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 commentedApr 6, 2023
edited
Loading

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

Copy link
MemberAuthor

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

Copy link
MemberAuthor

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

@pradyunsg
Copy link
MemberAuthor

!buildbot (?i).installed.

@bedevere-bot
Copy link

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

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

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

@pradyunsgpradyunsg merged commit9e67740 intopython:mainApr 13, 2023
@pradyunsgpradyunsg deleted the test-cppext branchApril 13, 2023 04:17
carljm added a commit to carljm/cpython that referenced this pull requestApr 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 requestApr 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 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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@zwarezwarezware left review comments

@FFY00FFY00FFY00 approved these changes

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@pradyunsg@zware@bedevere-bot@CAM-Gerlach@FFY00

[8]ページ先頭

©2009-2025 Movatter.jp