Prevent Tox from hanging with --installpkg sdist due to orphaned build backend
#3530
Conversation
|
Thanks for taking a look at this @gaborbernat! The question remains though what we should do with #3512? Should we leave it open? What would the correct fix be for the case of the builder's root actually being reconfigured to another path, and in which scenario can this happen? |
|
OK, I'll link the newsfragment to this PR, leaving the issue as-is for now. |
There was a problem hiding this comment.
@gaborbernat I need help on the tests.
I tried to transplant my MRE from the issue into the test suite, and it seems to work just fine, however, if I deliberately reintroduce the bug, it still passes. 🤔
I verified that a new backend executor is created in that case, however, that doesn't seem to cause any issues, unlike in the real run.
| """ | ||
| sdist = pkg_with_pdm_backend / "dist" / "skeleton-0.1.1337.tar.gz" | ||
| proj = tox_project({"tox.ini": tox_ini}, base=pkg_with_pdm_backend) | ||
| result = proj.run("--installpkg", str(sdist), "--exit-and-dump-after", "10") |
There was a problem hiding this comment.
This --exit-and-dump-after should be increased to something larger (like 120) once I iron out the tests.
|
Seems this stalled 🤔 |
|
Hi @gaborbernat! The patch itself works for us, and we use it internally at work (hosting a custom post-release of There is my reproducer on #3512 that I tried to transplant into the test suite, but for some reason I cannot reproduce it therein, only when running Tox "normally". Maybe I could try to take a look at writing a non-functional, unit-test like, test, just mocking Python attributes? |
|
Can you fix the CI? |
I don't remember what was wrong there, I can take a fresh look. But IIRC my attempt at running the said "functional" test passed but caused the CI gates to crash or hang for some reason that I didn't understand either. As said, I can also try to write more a unit-test like case unless you could point me to an example how to write a "functional" test running a real process. I saw that some tests did that but I don't understand the difference or the proper way to implement it. |
|
We'd need to get the CI passing as is before we can think of adding more tests 🤔 |
|
@gaborbernat to sum up, I believe my fix does what it should, and my fixture |
|
We don't have functional tests, just integration tests, there are examples of that in the suite 🤔 |
|
Aha, thanks, I'll take a look. Maybe my attempt to |
|
@gaborbernat that seemed to be the case, I removed that safeguard, and the suite now ostensibly passes just fine. To be precise in good faith though, IIRC my proposed regression test did not catch the issue even if I undid the fix. Probably there are subtle differences between running Or maybe it is good enough for now? Cause I don't know how to reproduce this in tests short of a "functional" run. |
vytas7
changed the title
--installpkg sdist due to orphaned build backend
2 participants
See the discussion on the issue (#3512) for the detailed explanation of the problem.
Fixes #3512.
tox -e fix)docs/changelogfolder