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

Copy the correct file during PEP 517's build process #6236

Merged
merged 4 commits into from Feb 5, 2019

Conversation

pradyunsg
Copy link
Member

Fixes #6196

Copy link
Member

@cjerdonek cjerdonek left a comment

Choose a reason for hiding this comment

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

Some comments.

src/pip/_internal/wheel.py Outdated Show resolved Hide resolved
src/pip/_internal/wheel.py Outdated Show resolved Hide resolved
src/pip/_internal/wheel.py Outdated Show resolved Hide resolved
src/pip/_internal/wheel.py Outdated Show resolved Hide resolved
src/pip/_internal/wheel.py Show resolved Hide resolved
src/pip/_internal/wheel.py Outdated Show resolved Hide resolved
src/pip/_internal/wheel.py Outdated Show resolved Hide resolved
@pradyunsg
Copy link
Member Author

pradyunsg commented Feb 4, 2019

Okay.

It seems like setuptool's current backend code makes the same assumption as pip's existing code -- that only one file exists in the dist/ directory once things are built. Not sure what's wrong here anymore. Commenting out the addition of a wheel in the dist/ directory for now due to that.

https://github.com/pypa/setuptools/blob/64e60fc32981a1615c35962a60297d264bf16734/setuptools/build_meta.py#L155-L158


This seems good to go to me. /cc @cjerdonek

add_files_to_dist_directory(pkg_to_wheel)

result = script.pip('wheel', pkg_to_wheel, '-w', script.scratch_path)
assert "Installing build dependencies" not in result.stdout, result.stdout
Copy link
Member

Choose a reason for hiding this comment

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

I would assert something positive here or else make "Installing build dependencies" a variable that's used in both this test and the previous. Otherwise, if the message ever changes, the test can continue to pass but for the wrong reason.

Copy link
Member Author

@pradyunsg pradyunsg Feb 4, 2019

Choose a reason for hiding this comment

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

That's the same pattern as existing tests; we can fix this in a follow up.

logger.info('Stored in directory: %s', output_dir)
return wheel_path
return dest_path
except Exception:
pass
Copy link
Member

Choose a reason for hiding this comment

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

Note for future: should this be swallowing an exception without logging anything?

Copy link
Member Author

Choose a reason for hiding this comment

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

No clue. :)

@pradyunsg
Copy link
Member Author

@cjerdonek If you're okay with giving the green light to this, let's merge? :)

(the CI failures are due to a PyPI search outage; I pinged folks for that)

@pradyunsg pradyunsg closed this Feb 4, 2019
@pradyunsg pradyunsg reopened this Feb 4, 2019
* Return paths from legacy/PEP 517 build handlers
* Simplify the wheel moving logic
* Sort the result of os.listdir, to increase determinism
setuptools expects only one wheel file to be present in dist/
@pradyunsg
Copy link
Member Author

Let's run the CI again.

@pradyunsg
Copy link
Member Author

Going ahead and merging this; we can address any additional issues in a follow up PR.

@pradyunsg pradyunsg merged commit 5a61475 into pypa:master Feb 5, 2019
@pradyunsg pradyunsg deleted the fix/pip-wheel-locations branch February 5, 2019 07:12
@lock
Copy link

lock bot commented May 29, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pip-19.x: pip wheel does not pick a whl on PEP-517 project
2 participants