-
Notifications
You must be signed in to change notification settings - Fork 3.1k
try to build wheels for directories/vcs #4714
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
Changes from all commits
3517df5
b241c22
fa927a6
48ee5d7
b229fd4
c5297e8
8fbd8f4
e58e50f
2c67a91
ef1a3ae
a7b0e4f
58f6917
711c99a
25d5546
882c8b9
ade439c
396abfe
dd326a7
951dfdd
e168cbb
dbcd1a7
c28d614
bd31280
b6377d4
edcc6de
97c667b
43eb3c6
00281b3
02a5d58
fb09a60
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
pip now builds a wheel when installing from a local directory or a VCS URL. | ||
Wheels from these sources are not cached. |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -30,7 +30,7 @@ def test_no_clean_option_blocks_cleaning_after_install(script, data): | |||
build = script.base_path / 'pip-build' | ||||
script.pip( | ||||
'install', '--no-clean', '--no-index', '--build', build, | ||||
'--find-links=%s' % data.find_links, 'simple', | ||||
'--find-links=%s' % data.find_links, 'simple', expect_temp=True, | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The cache should be cleaning up after itself even if this happens? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was controlled by the |
||||
) | ||||
assert exists(build) | ||||
|
||||
|
@@ -132,7 +132,7 @@ def test_cleanup_prevented_upon_build_dir_exception(script, data): | |||
result = script.pip( | ||||
'install', '-f', data.find_links, '--no-index', 'simple', | ||||
'--build', build, | ||||
expect_error=True, | ||||
expect_error=True, expect_temp=True, | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The cache should be cleaning up after itself even if this happens? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @takluyver Can you explain this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I can't remember the details. At some point some tests failed, and that seemed to be the right thing to make them work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps the PR does not clean up the cache as advertised? What is the affect of this parameter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAICT, this parameter means that the script run is expected to leave some temporary files, which is not what we want. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What we can do it turn the following line into a pip/src/pip/_internal/commands/install.py Line 222 in 6e2391a
Is there any other location where the wheelcache is initialized? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @xoviat -- a simple grep tells me there are 6 places. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pradyunsg So this test is named There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool. That means that this change in tests corresponds to a change in the expected behaviour. :) PS: 41b83f8 was not the change I was suggesting. I thought there was no call to cleanup in finally. |
||||
) | ||||
|
||||
assert result.returncode == PREVIOUS_BUILD_DIR_ERROR | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -193,7 +193,7 @@ def test_pip_wheel_fail_cause_of_previous_build_dir( | |
result = script.pip( | ||
'wheel', '--no-index', '--find-links=%s' % data.find_links, | ||
'--build', script.venv_path / 'build', | ||
'simple==3.0', expect_error=True, | ||
'simple==3.0', expect_error=True, expect_temp=True, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The cache should be cleaning up after itself even if this happens? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto here for what I said below. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto here for what I said below. ;) |
||
) | ||
|
||
# Then I see that the error code is the right one | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be on
WheelCache
. Also, since it only clears the ephemeral cache, I'd suggest renaming this tocleanup_ephem
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking about this some more, I don't agree. I think that first of all,
cleanup
methods have very little support in the Python ecosystem. Second of all, when there is acleanup
method, it should be defined on the base class so that it's always called after using the object and specific implementations should go on derived classes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, specifically, I propose making this
pass
, and then moving the guts toWheelCache
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.