try to build wheels for directories/vcs #4714

ghost · 2017-09-07T19:50:06Z

I think that the rebase is OK, but let me know whether I made any errors.

Closes #4562.

Follow up from gh-4144 To allow build system abstractions, we want installation to go through wheels in more cases. In particular, installing packages from a local directory or a VCS URL currently runs 'setup.py install'. The aim of this PR is to have it build a wheel, which is stored in an ephemeral cache directory, used for installation, and then discarded. We can't cache it permanently based on the path/URL, because the code there might change, but our cache wouldn't be invalidated.

When there's a previous build directory, no_clean is set

pfmoore · 2017-09-07T21:37:51Z

Is this a rebase of a different PR? Can you at least provide a link back to the original?

ghost · 2017-09-07T21:44:38Z

#4562

pfmoore · 2017-09-07T21:49:22Z

Ah, OK. That's someone else's PR, and it appears to be getting worked on by them. Not sure why you've picked this up, but a comment on the original PR by the author confirming that you've taken over the PR would be useful, if that's what's happened.

ghost · 2017-09-07T21:57:52Z

The last comment from the author was:

@pradyunsg if you'd like to take over this PR, that'd be great.

I'm actually the third person to take over this PR. If you don't feel that my actions were appropriate given the circumstances then feel free to close this.

pfmoore · 2017-09-07T22:02:25Z

I assume from this that @pradyunsg is working on it (or maybe plans on doing so). Anyway, I understand the situation now, which is all I wanted - thanks for clarifying.

pradyunsg · 2017-09-08T01:34:09Z

@pfmoore I had actually started working on it. Then, I think I discussed it with @dstufft who said that this something along the lines of this should probably be lower priority than the resolver for me. So, I moved it down my todo list.

I got swamped with a lot of stuff in the past few weeks and have had basically much less free time.

I won't mind if someone else does take the PR forward - it's not exactly a trivial PR and it affects a critical code path, so I wanna be a little careful on that.

IIRC, there's some discussion to be had on it too.

(edit: fleshed out sentence)

pradyunsg · 2017-09-08T09:16:40Z

src/pip/_internal/cache.py

@@ -117,7 +117,7 @@ class WheelCache(Cache):
    def __init__(self, cache_dir, format_control):
        super(WheelCache, self).__init__(cache_dir, format_control, {"binary"})

-    def get_path_for_link(self, link):
+    def get_path_for_link(self, link, ephem=False):


Instead of having a boolean in the signature, I'd suggest you implement a new method WheelCache.get_ephem_path_for_link.

Relevant Blog Post: https://martinfowler.com/bliki/FlagArgument.html

pradyunsg · 2017-09-08T09:19:10Z

news/4501.feature

@@ -0,0 +1,3 @@
+Installing from a local directory or a VCS URL now builds a wheel to install,


Could you rephrase this to:

pip now builds a wheel when installing from a local directory or a VCS URL. Wheels from these sources are not cached.

?

pradyunsg · 2017-09-08T09:22:17Z

src/pip/_internal/wheel.py

@@ -741,6 +741,7 @@ def build(self, session, autobuilding=False):

        buildset = []
        for req in reqset:
+            ephem_cache = False


bike-shedding: I'll suggest painting this as use_ephem_cache; I won't push if anyone disagrees.

pradyunsg · 2017-09-08T09:23:06Z

tests/functional/test_install_user.py

    def test_install_curdir_usersite(self, script, virtualenv, data):
        """
        Test installing current directory ('.') into usersite
        """
        virtualenv.system_site_packages = True
+        script.pip("install", "wheel")


Are you sure this is actually needed?

I have not validated this myself but from experience with related tests, I would strongly suspect that it is.

pradyunsg · 2017-09-08T09:24:18Z

tests/functional/test_install_user.py

    def test_install_curdir_usersite(self, script, virtualenv, data):
        """
        Test installing current directory ('.') into usersite
        """
        virtualenv.system_site_packages = True
+        script.pip("install", "wheel")


nit: use the common_wheels fixture.

pradyunsg · 2017-09-08T09:25:15Z

tests/functional/test_wheel.py

@@ -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,


The cache should be cleaning up after itself even if this happens?

Ditto here for what I said below.

Ditto here for what I said below. ;)

pradyunsg · 2017-09-08T09:25:40Z

tests/functional/test_install_cleanup.py

@@ -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,


The cache should be cleaning up after itself even if this happens?

@takluyver Can you explain this?

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.

Perhaps the PR does not clean up the cache as advertised? What is the affect of this parameter?

AFAICT, this parameter means that the script run is expected to leave some temporary files, which is not what we want.

What we can do it turn the following line into a with statement and set the wheelcache as a context manager:

pip/src/pip/_internal/commands/install.py

Line 222 in 6e2391a

wheel_cache = WheelCache(options.cache_dir, options.format_control)

Is there any other location where the wheelcache is initialized?

@xoviat -- a simple grep tells me there are 6 places.

@pradyunsg So this test is named cleanup_prevented_on_build_dir_exception, and if you look at the control flow on install, it makes sense why the directory would not cleaned up. I attempted to fix that with the contextmanager, but your proposed solution necessitates that temporary files will be created here.

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.

pradyunsg · 2017-09-08T09:25:55Z

tests/functional/test_install_cleanup.py

@@ -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,


The cache should be cleaning up after itself even if this happens?

This was controlled by the no_clean option but I have disabled that behavior.

pradyunsg · 2017-09-08T09:38:11Z

src/pip/_internal/req/req_set.py

@@ -16,9 +16,6 @@ def __init__(self,
                 require_hashes=False, target_dir=None, use_user_site=False,
                 pycompile=True):
        """Create a RequirementSet.
-


This is an unrelated (albeit obvious) change. It'd be nice if you revert this hunk and make a new PR for it.

This was 396abfe done by @alex. I would need to get his consent before removing it.

Uhhh, no clue, feel free to ignore whatever I did.

I will remove this then.

ghost · 2017-09-08T16:19:07Z

So I've addressed some of the concerns. Unfortunately travis seems to be having problems so I'm not going to proceed further until I can get tests passing.

ghost · 2017-09-09T18:55:52Z

src/pip/_internal/commands/install.py

-        with self._build_session(options) as session:
+        with self._build_session(options) as session, \
+                WheelCache(options.cache_dir, options.format_control) as  \
+                wheel_cache:


This looks super ugly but it is the best that I could come up with. If anyone else has any better ideas, I am open to them.

Why not put a cleanup call in the finally of the try clause starting below at line 261 or 251?

I don't see why this needs to be a context manager. It doesn't make sense to me.

It needs to be a context manager because the files were not cleaned up correctly before, as you pointed out.

needs to be a context manager

I think just calling the cleanup method in the existing finally clause should be just fine.

I don't want to add one-more-way to use a WheelCache, especially since this behaviour is not really a thing in Cache.

pradyunsg · 2017-09-09T20:06:29Z

@xoviat -- please revert 41b83f8.

I'm down with a headache right now. I'll get back to this at a later time.

ghost · 2017-09-09T20:17:42Z

@pradyunsg The contextmanager idea came directly from here. I will drop the three commits that address the cleanup issues.

ghost · 2017-09-09T20:18:24Z

I hope your headache gets better.

pradyunsg · 2017-09-15T04:51:51Z

STATUS: BLOCKED UNTIL #4715 IS ADDRESSED

@xoviat - you can cherry-pick the fix from #4719.

ghost · 2017-09-15T16:42:25Z

I'm not going to play these games. Master should always pass, and if it doesn't it should be fixed as soon as possible. Maybe Paul has time to review #4719?

ghost · 2017-09-15T18:54:33Z

@pradyunsg Are you ready to approve these changes or have you got more feedback for me? No rush by the way.

pradyunsg · 2017-09-17T10:40:49Z

I haven't looked at this since the last time. I'll try to find time to do it sometime next week but I genuinely don't know.

pradyunsg · 2017-09-17T10:44:44Z

src/pip/_internal/cache.py

@@ -102,6 +106,10 @@ def _link_for_candidate(self, link, candidate):

        return index.Link(path_to_url(path))

+    def cleanup(self):


This should be on WheelCache. Also, since it only clears the ephemeral cache, I'd suggest renaming this to cleanup_ephem.

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 a cleanup 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.

So, specifically, I propose making this pass, and then moving the guts to WheelCache.

pradyunsg · 2017-09-17T10:45:06Z

src/pip/_internal/cache.py

@@ -131,6 +139,18 @@ def get_path_for_link(self, link):
        # join them all together.
        return os.path.join(self.cache_dir, "wheels", *parts)

+    def get_ephem_path_for_link(self, link):
+        """Return a driectory to store cached wheels for link


Typo: directory

pradyunsg · 2017-09-17T10:45:59Z

There's probably more; this is all I could look at in the 5 minutes I had. 🤷‍♂️

pradyunsg · 2017-09-17T10:50:15Z

Also worth noting, I'm on the fence about adding the ephemeral cache stuff to WheelCache. Maybe adding an EphemeralWheelCache that inherits from WheelCache would be cleaner? Idk myself.

API design is hard. This looks about okay to me.

pradyunsg

Oh well. I forgot to submit this review. :/

pradyunsg · 2017-10-04T12:39:14Z

src/pip/_internal/cache.py

@@ -102,6 +106,10 @@ def _link_for_candidate(self, link, candidate):

        return index.Link(path_to_url(path))

+    def cleanup(self):


pradyunsg · 2017-10-04T13:11:46Z

@xoviat I took this forward in #4764. I hope you don't mind. :)

ghost · 2017-10-04T15:34:06Z

No problem.

takluyver and others added 24 commits September 7, 2017 13:06

Add news file

b241c22

Fix list comp

fa927a6

Allow temp files for another test

48ee5d7

When there's a previous build directory, no_clean is set

Fix cleanup on 'pip install' with no args

b229fd4

Cleanup wheel cache after 'pip freeze'

c5297e8

Expect temp files in another test

8fbd8f4

Update test of install output

e58e50f

Expect temp files on another test

2c67a91

Update another install output test

ef1a3ae

Update test to check for dist-info

a7b0e4f

Remove string formatting operation

58f6917

Install 'wheel' for test that now requires it

711c99a

fix merge issues

25d5546

flake8

882c8b9

fix

ade439c

not a thing anymore

396abfe

these are probably needed

dd326a7

flake8

951dfdd

cache: implement epherm cache directory and cleanup

e168cbb

cache: add epem parameter

dbcd1a7

tests: install: fix merge errors

c28d614

🎨

bd31280

🎨

b6377d4

pradyunsg reviewed Sep 8, 2017

View reviewed changes

xoviat added 4 commits September 8, 2017 10:58

tests: user: use common_wheels fixture

edcc6de

wheel: rename ephen_cache flag

97c667b

📰

43eb3c6

cache: move ephem flag into two methods

00281b3

ghost commented Sep 9, 2017

View reviewed changes

req_set: revert 396abfe.

02a5d58

ghost closed this Sep 15, 2017

ghost reopened this Sep 15, 2017

pradyunsg reviewed Sep 17, 2017

View reviewed changes

cache: fix spelling error

fb09a60

pradyunsg mentioned this pull request Oct 4, 2017

Do wheel installs from VCS/directories using ephemeral caching #4764

Merged

pradyunsg reviewed Oct 4, 2017

View reviewed changes

ghost closed this Oct 4, 2017

This pull request was closed.

		@@ -0,0 +1,3 @@
		Installing from a local directory or a VCS URL now builds a wheel to install,

		@@ -102,6 +106,10 @@ def _link_for_candidate(self, link, candidate):

		return index.Link(path_to_url(path))

		def cleanup(self):

try to build wheels for directories/vcs #4714

try to build wheels for directories/vcs #4714

Conversation

ghost commented Sep 7, 2017 • edited by pradyunsg

pfmoore commented Sep 7, 2017

ghost commented Sep 7, 2017

pfmoore commented Sep 7, 2017

ghost commented Sep 7, 2017

pfmoore commented Sep 7, 2017

pradyunsg commented Sep 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 8, 2017 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg commented Sep 9, 2017 • edited

ghost commented Sep 9, 2017

ghost commented Sep 9, 2017

pradyunsg commented Sep 15, 2017

ghost commented Sep 15, 2017

ghost commented Sep 15, 2017 via email

pradyunsg commented Sep 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg commented Sep 17, 2017

pradyunsg commented Sep 17, 2017

pradyunsg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg commented Oct 4, 2017

ghost commented Oct 4, 2017

ghost commented Sep 7, 2017 •

edited by pradyunsg

pradyunsg commented Sep 8, 2017 •

edited

ghost commented Sep 8, 2017 •

edited by ghost

pradyunsg commented Sep 9, 2017 •

edited