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

Fix persistent build/ dir with tox wheels #1098

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

sirosen
Copy link
Member

@sirosen sirosen commented Oct 31, 2024

tox build_wheel has been producing a build/ directory (setuptools)
which is not cleared between runs. This manifests as a very bad and
confusing developer experience when a module is renamed or moved, and
both the old and new files are present in the sdist and wheel for
testing. This can result in module name shadowing and other hard to
debug issues.

We have attempted to solve this in the past by means of custom
external packaging environments, but have never found a satisfactory
solution. This new approach is to "bite the bullet" and use tox's
plugin extension points. Via pluggy, tox is providing the same
architecture for extension which pytest is based around, meaning that
the 'toxfile.py' should be thought of as analogous to 'conftest.py'
(already familiar) and the hook decorator should be understood to be
similar to @pytest.fixture or definition of hook methods like
pytest_configure.

We only define one hook, which is invoked each time tox is
installing into an environment. We inspect the environment name and
abort if it's anything other than "build_wheel". If and only if
we're running the install phase of "build_wheel", we will remove the
build/ directory if present.

For my final test runs of the new behavior, I ran tox, manually added
files to build/, then ran tox again. I was able to confirm that the
extra files in build/ were removed. The same testing procedure fails
on main. I also confirmed that tox p was not measurably slower,
indicating that the build-once semantics are being preserved.


📚 Documentation preview 📚: https://globus-sdk-python--1098.org.readthedocs.build/en/1098/

tox build_wheel has been producing a `build/` directory (setuptools)
which is not cleared between runs. This manifests as a very bad and
confusing developer experience when a module is renamed or moved, and
both the old and new files are present in the sdist and wheel for
testing. This can result in module name shadowing and other hard to
debug issues.

We have attempted to solve this in the past by means of custom
external packaging environments, but have never found a satisfactory
solution. This new approach is to "bite the bullet" and use tox's
plugin extension points. Via pluggy, tox is providing the same
architecture for extension which pytest is based around, meaning that
the 'toxfile.py' should be thought of as analogous to 'conftest.py'
(already familiar) and the hook decorator should be understood to be
similar to `@pytest.fixture` or definition of hook methods like
`pytest_configure`.

We only define one hook, which is invoked each time `tox` is
installing into an environment. We inspect the environment name and
abort if it's anything other than `"build_wheel"`. If and only if
we're running the install phase of `"build_wheel"`, we will remove the
`build/` directory if present.

For my final test runs of the new behavior, I ran tox, manually added
files to `build/`, then ran tox again. I was able to confirm that the
extra files in `build/` were removed. The same testing procedure fails
on `main`. I also confirmed that `tox p` was not measurably slower,
indicating that the build-once semantics are being preserved.
Copy link
Member

@kurtmckee kurtmckee left a comment

Choose a reason for hiding this comment

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

Merge immediately. 🙏

@sirosen sirosen merged commit f9bae4c into globus:main Nov 1, 2024
17 checks passed
@sirosen sirosen deleted the cleanup-tox-build branch November 1, 2024 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants