Temporary build and src directories no longer created in cwd/pwd (bugs #339, #381)

[TC01]

This has been an issue with pip for a while (the latest bugs about this appear to be #339 and #381, but glancing through I found a couple of other reports), where the temporary build dir (and the temporary src dir) is created in a user's pwd/cwd.

This fix (unlike the proposed fix linked in #381) will always use tempfile to create the build and src directories, using a format similar to other temporary pip directories (/tmp/pip-[foo]-build, /tmp/pip-[bar]-src).

I also moved the configuration directory from ~/.pip to ~/.config/pip on Unix systems- ~/.config being a place where more and more programs are storing their configuration files, as a way to avoid clutter in a user's home folder.

[pnasrat]

I'd rather separate the moving of the config dir as we probably need to ensure a migration path for users and that is documented.

[merwok]

Yes, compliance with the XDG Base Directory specification is its own issue.

[TC01]

I've pulled that change from this pull request and will get working on it in a different branch (and pull request).

Aside from that, what about the rest of this change?

[pnasrat]

The current status says it can't be merged - can you pull develop and fix any conflicts then repush.

Thinking out loud - we currently don't have tests on that codepath, which does concern me.

At the very least you probably want to add error handling for the rmdir's.

The comment on it being a hack does concern me that we could take another approach - looking where we are using build_prefix and src_prefix and handling that more correctly. Certainly in some cases req.py uses mkdtemp.

[TC01]

Pulled develop and repushed it- it should be able to merge now (I hope). And I added a try/except around the rmdirs.

I did some research into this and I think the problem is that, because you can override the location of build_prefix on the command line, pip needs to be able to distinguish between temporary directories it has created and directories that were already present on the system.

This is currently done by creating build_prefix (and src_prefix) in req.py (I believe), rather than in locations.py. If the directories are created in locations.py (or already existed before pip started running), they will not be flagged for deletion.

Ideally, I suppose, directories should be flagged for deletion if their name matches build_prefix instead? Then they could be created in locations.py but still deleted when pip finishes running.

[travisbot]

This pull request fails (merged fa9cc7c into e0db44e).

[TC01]

Uh, so, looking at the build logs, this seems to fail only in Python 3.2, and I'm not sure exactly what the test is that's causing it to fail, or why.. any thoughts? Here's the failure:

FAIL: Test installing an editable git package from a repository, upgrading the repository,

Traceback (most recent call last):
File "/home/vagrant/virtualenv/python3.2/lib/python3.2/site-packages/nose/case.py", line 198, in runTest
self.test(*self.arg)
File "/home/vagrant/builds/pypa/pip/tests/test_upgrade.py", line 167, in test_editable_git_upgrade
assert 'some different version' in version2.stdout, "Output: %s" % (version2.stdout)
AssertionError: Output: 0.1

[pnasrat]

This looks like it's a known flaky test I'm investigating in Issue #530

[TC01]

I've managed to get pip to cleanup the build and source dirs if they are the same directories that get created in locations.py, so the "hack" I added to locations.py can be removed. However- this only works for the install command, as install is the only command that calls cleanup_files. So we'd need to have all the commands run that cleanup_files command.

I'm also not sure how this would affect bundles- it seems like src_dir used to only be created there is a bundle involved, but now it's always being created all the time.

I feel like it might be simpler to keep removing the temporary directories when they're created and re-add them later. Should I keep working on everything I mentioned above? Or should I keep this pull request updated with upstream and wait if someone wants to merge?

[pnasrat]

It may make sense to merge this in the short term and do the bigger change in a seperate pull request.

[pnasrat]

Unless you're already pretty much there and have a pull request ready - in which case feel free to close this one out.

[TC01]

No, I'm nowhere near having finished all of that other stuff- so I'll leave this open and keep it updated, awaiting a merge.

[pnasrat]

OK I'll merge and close this, and please follow up with a further pull request when you are ready.

Thanks for your contribution.

[carljm]

Does this break the workflow of doing a --no-install followed by a --no-download? I know some users are using that, and we'll likely get flak if we break it. AFAIK that workflow was the main reason why build/ was created in cwd, so a subsequent run with --no-download could find the previously downloaded and unpacked stuff.

Personally I'd be happy to lose this "feature", but I'm a bit shy of removing features after the complaints I got around the removal of -E.

[TC01]

Well- the build directory is no longer always the same, so technically yes. However, if you wanted the workflow to be like that (and I'm not sure why you would), you could run --no-install --build /some/folder and then --no-install --build /some/folder.

This could possibly be fixed if the name of the folder generated by tempfile was cached somewhere (so rather than generating a new one every time locations.py gets imported, it would be generated once and written to a config file somewhere).

[carljm]

Yes, one could still use --build explicitly, though that's still a breaking change for anyone currently relying on this workflow.

There's another issue, which is that with this change editables will be checked out to a temp directory. This is wrong, since stuff left hanging around in /tmp should be safely deletable, but deleting the temp src directory after an editable has been installed will delete that editable and make it no longer importable. Either src needs to continue to use cwd, or we have to require --src to be given explicitly on the command line whenever an editable is installed outside a virtualenv.

At this point I think the best option available is to make this change opt-in-only via an env var / config file entry, so as to avoid breaking existing workflows. And if this change is switched on, fail with a clear and explicit error message if --no-download or --no-install is used without --build or an editable is installed without --src (all only outside a virtualenv).

I think until there's a pull request ready that fixes this with more care for backwards-compatibility, this change should be reverted. Not actually doing that until I can discuss with @pnasrat. (Also, one other note for @pnasrat - if merging a change with any user-facing implications at all, please be sure to record it in docs/news.txt.)

[TC01]

It sounds like then that there are two different issues: where src is created and where build is created. As I understand it.. build should be a temporary directory, but always the same temporary directory so existing workflow will still work. But src needs to be a permanent directory because if it's being used at all, it's being used to install editable packages?

The first is pretty simple to fix. The second is also simple to fix if we just move src back to cwd, but... isn't there somewhere else we could put it?

[carljm]

That's right, except I'm not sure that trying to cache the build location between runs is going to work well. For one thing, there isn't really a good place to cache that information. Also, in the long run I wouldn't mind deprecating --no-download and --no-install -- it just needs to be done gradually without outright breaking anyone's workflow in a single release. This is why I propose leaving the default behavior exactly as it was before this change, and making the use of a temp directory for build an opt-in choice for now. And if you opt in to it, you must explicitly specify --build if you use --no-download or --no-install.

For src, when running outside a virtualenv I don't really see a better default location than CWD. Perhaps the user's home directory? That seems even more implicit and potentially confusing. Or the direction I'd considered above, which is to deprecate using editables outside a virtualenv without explicit --src flag. But I'm not sure that's really an improvement, so I think I lean towards just leaving src as-is.

[TC01]

Okay.. how about build_prefix = os.path.join(tempfile.gettempdir(), 'pip-build') then? This will still be a temp directory, but it will be the same temp directory each time. (It also fixes the problem I was going on about before, as this won't actually create it, unlike the mkdtemp function).

I'd be okay with the opt-in thing for now as well, but I think this might be a better solution.

[carljm]

Actually, yeah, that's a pretty good solution.

[TC01]

Alright- I'll just open another pull request with that, then, and with src still in CWD.