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

Do not attempt setup.py clean for failed pep517 builds #7477

Merged
merged 2 commits into from
Jan 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions news/6642.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Do not attempt to run ``setup.py clean`` after a ``pep517`` build error,
since a ``setup.py`` may not exist in that case.
5 changes: 3 additions & 2 deletions src/pip/_internal/wheel_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -231,11 +231,12 @@ def _build_one_inside_env(
req.name, e,
)
# Ignore return, we can't do anything else useful.
_clean_one(req, global_options)
if not req.use_pep517:
_clean_one_legacy(req, global_options)
return None


def _clean_one(req, global_options):
def _clean_one_legacy(req, global_options):
# type: (InstallRequirement, List[str]) -> bool
clean_args = make_setuptools_clean_args(
req.setup_py_path,
Expand Down
3 changes: 3 additions & 0 deletions tests/data/packages/pep517_wrapper_buildsys/mybuildsys.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@


def build_wheel(*a, **kw):
if os.environ.get("PIP_TEST_FAIL_BUILD_WHEEL"):
raise RuntimeError("Failing build_wheel, as requested.")

# Create the marker file to record that the hook was called
with open(os.environ['PIP_TEST_MARKER_FILE'], 'wb'):
pass
Expand Down
1 change: 1 addition & 0 deletions tests/functional/test_install.py
Original file line number Diff line number Diff line change
Expand Up @@ -1256,6 +1256,7 @@ def test_cleanup_after_failed_wheel(script, with_wheel):
shebang = open(script_py, 'r').readline().strip()
assert shebang != '#!python', shebang
# OK, assert that we *said* we were cleaning up:
# /!\ if in need to change this, also change test_pep517_no_legacy_cleanup
assert "Running setup.py clean for wheelbrokenafter" in str(res), str(res)


Expand Down
16 changes: 16 additions & 0 deletions tests/functional/test_install_cleanup.py
Original file line number Diff line number Diff line change
Expand Up @@ -136,3 +136,19 @@ def test_cleanup_prevented_upon_build_dir_exception(script, data):
assert result.returncode == PREVIOUS_BUILD_DIR_ERROR, str(result)
assert "pip can't proceed" in result.stderr, str(result)
assert exists(build_simple), str(result)


@pytest.mark.network
def test_pep517_no_legacy_cleanup(script, data, with_wheel):
"""Test a PEP 517 failed build does not attempt a legacy cleanup"""
to_install = data.packages.joinpath('pep517_wrapper_buildsys')
script.environ["PIP_TEST_FAIL_BUILD_WHEEL"] = "1"
res = script.pip(
'install', '-f', data.find_links, to_install,
expect_error=True
)
# Must not have built the package
expected = "Failed building wheel for pep517-wrapper-buildsys"
assert expected in str(res)
# Must not have attempted legacy cleanup
Copy link
Member

@xavfernandez xavfernandez Jan 1, 2020

Choose a reason for hiding this comment

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

Since this is a "negative" test, it might be worth it to add a comment linking to this test in a "positive" test like test_cleanup_after_failed_wheel (for the possible future, when we might modify Running setup.py clean for %s log into something not containing setup.py clean)
PS: tell me if I'm unclear ^^

Copy link
Member Author

Choose a reason for hiding this comment

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

@xavfernandez good point, done.

assert "setup.py clean" not in str(res)