"--user" fixes #511

Closed
wants to merge 27 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

qwcode commented Apr 16, 2012

Pull request to fix various "--user" issues

493: pip shouldn't allow --user installs in virtualenvs (w/o --system-site-packages)
440: pip install --user tries to uninstall system package
494: can't uninstall --user "develop" install
495: user site should be "local" in virtualenvs with --system-site-packages (mostly so we can test)

Tests are passing for py25, py26, py27, and py32 (against rebased branch from 4/15)

I'm happy to trim out or alter parts that may cause concern in the interest of getting as many of these issues resolved.

As mentioned to Carl over email, I'm concerned about the level of testing we can achieve for the "--user" feature, given that the test framework uses virtualenvs, and that full coverage would require real python installs.

I'd like to still add some mock/patch tests for the significantly altered util.egg_link_path, and more tests in general.

pip/commands/install.py
@@ -184,6 +185,8 @@ def run(self, options, args):
options.src_dir = os.path.abspath(options.src_dir)
install_options = options.install_options or []
if options.use_user_site:
+ if running_under_virtualenv() and virtualenv_no_global():
@carljm

carljm Apr 16, 2012

Contributor

why check both here, since virtualenv_no_global implies running_under_virtualenv?

@qwcode

qwcode Apr 17, 2012

Contributor

early on in the commit history, "virtualenv_no_global" didn't imply "running_under_virtualenv". the former was just checking for the global file. later I changed "virtualenv_no_global", but didn't update these if statments.
I can change this.

@qwcode

qwcode Apr 17, 2012

Contributor

well, even when "virtualenv_no_global" was only checking for the existence of a "no-global-site-packages.txt" in the right place, that's still pretty conclusive. not sure really why I ever had both checks in there : )

pip/commands/install.py
@@ -184,6 +185,8 @@ def run(self, options, args):
options.src_dir = os.path.abspath(options.src_dir)
install_options = options.install_options or []
if options.use_user_site:
+ if running_under_virtualenv() and virtualenv_no_global():
+ raise InstallationError("You are in virtualenv where the user site is not visible, will not continue.")
@carljm

carljm Apr 16, 2012

Contributor

I think this error message should explicitly name the installation flag that the user needs to remove to fix the problem (--user).

@qwcode

qwcode Apr 17, 2012

Contributor

ok

pip/locations.py
+#can't be replaced with property set at import. site.py hasn't done it's work yet
+def user_site():
+ "return user site as long as not in venv/no-global"
+ if not (running_under_virtualenv() and virtualenv_no_global()):
@carljm

carljm Apr 16, 2012

Contributor

again, no need to check both of those conditions, the latter implies the former

@qwcode

qwcode Apr 17, 2012

Contributor

agreed, see comment above

pip/locations.py
+#can't be replaced with property set at import. site.py hasn't done it's work yet
+def user_base():
+ "return user base as long as not in venv/no-global"
+ if not (running_under_virtualenv() and virtualenv_no_global()):
@carljm

carljm Apr 16, 2012

Contributor

again no need to check both

@qwcode

qwcode Apr 17, 2012

Contributor

agreed, see comment above

pip/req.py
@@ -661,7 +663,12 @@ def check_if_exists(self):
except pkg_resources.DistributionNotFound:
return False
except pkg_resources.VersionConflict:
- self.conflicts_with = pkg_resources.get_distribution(self.req.project_name)
+ existing_dist = pkg_resources.get_distribution(self.req.project_name)
+ if self.set.use_user_site:
@carljm

carljm Apr 16, 2012

Contributor

I would rather pass a user_site flag explicitly to each InstallRequirement rather than give InstallRequirement awareness of the RequirementSet.

@qwcode

qwcode Apr 17, 2012

Contributor

I had 2 thoughts on why I put it on the set

  1. an execution of the install command (which is what "--user" applies to) is installing a RequirementSet, so the Set is what holds the property, and it flows down to the individual requirements.
  2. it was simpler I thought to put it on the set. it just has one constructor.

Paul's fork, put the property on the InstallRequirement.

ultimately, I don't really care. I'll change it if it helps get it merged.

@carljm

carljm Apr 17, 2012

Contributor

Your logic in (1) is fine. The problem is that with this change, an InstallRequirement gains an implicit dependency on someone patching on a RequirementSet as its set attribute after it's instantiated, otherwise it will blow up with an AttributeError. This is bad API and breaks the abstraction of InstallRequirement, essentially requiring that it always be passed to RequirementSet.add_requirement before it can be used. This is a problem both for anyone using InstallRequirement directly (which is obviously not documented, so not a big deal), but more importantly for all future work on pip that makes use of this class.

Basically, there is currently a uni-directional dependency here: RequirementSet knows about and depends on InstallRequirement, but the reverse is not true. This change would turn that into a circular dependency, where both classes know about, require, and depend on the other. This breaks encapsulation, will make future maintenance more difficult, and is worth avoiding.

I definitely want InstallRequirement to have its own use_user_site attribute (which should have a default value set in __init__, not rely on it being monkeypatched on by RequirementSet.add_requirement), and not have direct access to a RequirementSet at all. This avoids the "implicit dependency" and "bidirectional dependency" issues. I don't mind, though, if RequirementSet also has a use_user_site attribute and its add_requirement method changes the value of the use_user_site attribute on the InstallRequirement it is passed accordingly (rather than having to pass in a use_user_site argument to each constructor of InstallRequirement); RequirementSet already has knowledge of InstallRequirement internals.

@qwcode

qwcode Apr 18, 2012

Contributor

at the time, I was just focused on it's use as a member of a RequirementSet.
I see your pt. I'll change it.

@qwcode

qwcode Apr 19, 2012

Contributor

I reread your post.

just curious why you see what I was doing as monkey patching?

  • the InstallRequirement constructor was initializing self.set to None
  • then Requirement.add_requirement() was setting the "set" property on the requirement to the RequirementSet object.
    where is the monkey patch in that?
    I tend to use that phrase mostly when patching methods on classes (usually in libraries I don't control)

I do understand your maintenance argument in wanting to keep InstallRequirements ignorant of sets and their properties.

@carljm

carljm Apr 20, 2012

Contributor

You're right, it's not really monkeypatching, that was a careless use of the term. Whatever it's called, though, it's a non-obvious and error-prone API to require external code to set a particular attribute of an instance before that instance is usable. The fact that it's set by the constructor doesn't really help in this case because it's set to a non-functional value, and there's no error-handling around the use of self.set to account for the possibility that it could be None.

pip/req.py
@@ -809,12 +817,14 @@ def add_requirement(self, install_req):
name = install_req.name
if not name:
self.unnamed_requirements.append(install_req)
+ install_req.set = self
@carljm

carljm Apr 16, 2012

Contributor

don't like this

@qwcode

qwcode Apr 17, 2012

Contributor

see above comment.

pip/req.py
else:
if self.has_requirement(name):
raise InstallationError(
'Double requirement given: %s (aready in %s, name=%r)'
% (install_req, self.get_requirement(name), name))
self.requirements[name] = install_req
+ install_req.set = self
@carljm

carljm Apr 16, 2012

Contributor

or this

pip/util.py
- Return True if path is within sys.prefix, if we're running in a virtualenv.
-
- If we're not in a virtualenv, all paths are considered "local."
+ If we're not in a virtualenv, or path is in site.USER_BASE, then considered "local."
@carljm

carljm Apr 16, 2012

Contributor

user-site should definitely not be considered local in a no-site-packages virtualenv, but it seems that with this code it would.

@qwcode

qwcode Apr 17, 2012

Contributor

yes, I should fix that.

pip/util.py
+ Return True if given Distribution is installed in user site
+
+ """
+ return normalize_path(dist_location(dist)).startswith(normalize_path(user_site()))
@carljm

carljm Apr 16, 2012

Contributor

Really this check should ensure that the containing path ends with os.path.sep before doing the startswith check, otherwise it would claim that foo is "within" an adjacent foobar.

It would probably be better to extract a separate path_contains_path or some such utility function.

@qwcode

qwcode Apr 19, 2012

Contributor

I took this approach exactly from the is_local function that's there already.
https://github.com/pypa/pip/blob/develop/pip/util.py#L282

Is this different than that somehow, or they all suffer the weakness?

I think you said it in reverse anyway. (foobar might be seen as within in bar you mean?)

so an example might be this?:
user site ~/.local/lib/python2.6/site-packages
dist location ~/.local/lib/python2.6/site-packages_dist

I guess it makes sense to worry about this?
probably much more likely to occur in the path_in_userbase method

so extract some path_contains_path function, and fix all 3 methods?

@carljm

carljm Apr 20, 2012

Contributor

Yeah, the existing code has this problem equally - and you're right that it's not likely to occur in reality, though it'd be a nasty one to track down if it did. If you don't feel like fixing it, that's fine, since it's not really related to this pull request, but we should still extract the path_contains_path function rather than duplicating that same bug two more times.

pip/util.py
+
+ """
+ if path.strip() and user_base():
+ return normalize_path(path).startswith(normalize_path(user_base()))
@carljm

carljm Apr 16, 2012

Contributor

extract a utility function and use it both here and above.

@qwcode

qwcode Apr 17, 2012

Contributor

ok

pip/req.py
@@ -454,7 +456,7 @@ def uninstall(self, auto_confirm=False):
'easy-install.pth')
paths_to_remove.add_pth(easy_install_pth, './' + easy_install_egg)
- elif os.path.isfile(develop_egg_link):
+ elif develop_egg_link and os.path.isfile(develop_egg_link):
@carljm

carljm Apr 16, 2012

Contributor

your updated version of egg_link_path will only return a path that exists, so there's no need to keep the isfile check here

@qwcode

qwcode Apr 17, 2012

Contributor

yes, good pt.

tests/test_pip.py
+ # imported *before* the sys.path is fully initalized, causing pkg_resources.working_set
+ # to be improperly initialized
+ # this at least puts back the user site so it can be tested properly
+ pth.write("import pkg_resources; pkg_resources.working_set.add_entry('%s'); " %self.user_site_path)
@carljm

carljm Apr 16, 2012

Contributor

Haven't fully wrapped my mind around this one yet, but this workaround smells bad. Can't we fix the actual problem?

@qwcode

qwcode Apr 17, 2012

Contributor

the source of the problem is PyPIProxy setup living in the pth file.
I'll dig deeper on this though and see what I can come up with.

@carljm

carljm Apr 17, 2012

Contributor

Yeah, the problem is that the proxy setup imports pip.backwardcompat, which implicitly causes an import of pip/__init__.py, which imports a bunch of other stuff, including indirectly pkg_resources.

I think pypi_intercept.pth is probably overkill in the first place; with TestPipEnvironment we already control setup and teardown of a bunch of stuff for tests, seems like we could just set up the proxy there instead without missing any requests. I'll try to look at that today.

@qwcode

qwcode Apr 29, 2012

Contributor

the proxy setup is monkey patching urllib2.
currently, that patch executes in a pth, which means it's active for any python program that runs in the test virtualenv (including pip)
another option I see is to hack the pip console script (after install) to include the patch.
neither are pretty, but at least the latter would prevent the corruption of the pkg_resources working set.

you mentioned the TestPipEnvironment setup, but I don't see what the offers us?

@qwcode

qwcode Apr 29, 2012

Contributor

a cleaner option that's working and passing tests is to execute the proxy setup in a sitecustomize.py (instead of a pth file)
this allows the sys.path to fully build before pkg_resources is imported.
I think that's the winner.

Contributor

carljm commented Apr 16, 2012

Thanks for working on this, generally looks great! More tests would be excellent, though I'd probably merge it with the current level of testing, pending comments above.

Contributor

qwcode commented Apr 17, 2012

ok, cool, I'll post some updates in a day or two.

Contributor

qwcode commented Apr 20, 2012

so far, I've got all this done except the pypi setup issue, and I want to add some more tests.
I went ahead and created a new path_in_path function that those 3 functions are now using.
I'll try to push stuff up tomorrow.

Contributor

qwcode commented Apr 25, 2012

what's in the 2 sets of commits (listed above):
-- remove the unnecessary logical conditions you found
-- no InstallRequirement.set property anymore
-- the new install error msg references the --user option
-- removed loophole from is_local you found
-- new util.path_in_path function
-- mock test cases for util.path_in_path and util.egg_link_path

concerns:
-- a smarter way to do util.path_in_path? mine smells bad.
-- py25 doesn't support class decorators, hence the mock patching I've done is noisy; a better way?

my todo:
--test path_in_path on windows/mac? (I'm on linux)
--look thru the new util test cases again; easy to make mistakes there.
--still haven't looked at the pypi setup issue

Owner

qwcode commented on pip/util.py in d2fc1a4 Apr 25, 2012

need to track down how this was unnoticed in the testing. : (

Owner

qwcode replied Apr 27, 2012

2 things came together for this to be missed in the tests.

  1. due to the typos the elif block was always false.
  2. due to the test framework locating the user site in sys.prefix, the else block was deeming user installs "local"
    the typos are fixed in a commit to come.

qwcode added some commits Apr 26, 2012

perform PyPIProxy.setup() in sitecustomize instead of pth file to pre…
…vent premature import of pkg_resources, which corrupts pkg_resources.working_set
Contributor

qwcode commented May 5, 2012

I think i've addressed everything that's been mentioned

questions:

  1. whether we should "fix" issue 495 (see comments in that issue). in this branch, it's "fixed"
  2. whether to use my new "path_in_dir" function or just to fall back to the old code

next weekend (when I have some more time) I plan on getting my environment straight once and for all, so I can pass all the tests.
currently, I always have some stragglers that fail. I'll feel more confident after that.

tests I've been running:
--linux: py24, py25, py26, py27, py32
--windows: py27

pip/backwardcompat.py
@@ -112,6 +113,14 @@ def fwrite(f, s):
from distutils.sysconfig import get_python_lib, get_python_version
+def get_user_site():
+ "return site.USER_SITE or None if not implemented in earlier version of python"
@pnasrat

pnasrat May 13, 2012

Contributor

Nit we should follow http://www.python.org/dev/peps/pep-0257/#one-line-docstrings and make it a sentence ie capitalize Return and end in a period.

pip/backwardcompat.py
+ return getattr(site,'USER_SITE',None)
+
+def get_user_base():
+ "return site.USER_BASE or None if not implemented in earlier version of python"
@pnasrat

pnasrat May 13, 2012

Contributor

Ditto re docstring.

Contributor

pnasrat commented May 13, 2012

lgtm - if you're happy with your testing

Contributor

qwcode commented May 13, 2012

ok, I worked on my environments, and ran tests again for
linux: py24, py25, py26, py27, py32
windows: py27

for linux testing, down to 2 unrelated failures:

  1. 1 bz2 test failing for my non-py26 installs, due to it not being compiled in those py versions
  2. test_vcs_backends.test_git_with_tag_name_and_update fails across the board due to my git version having a different format for that kind of error

on windows:

  1. various failures related to my svn installation that I didn't take the time to sort out. I'm pretty out of touch with svn config on windows.
  2. a few failures because I don't have bzr installed
Contributor

pnasrat commented May 13, 2012

Github is complaining about mergability. I can try merge and thus cause a travis CI run on my personal develop branch, or maybe we can wait a little longer and get the test on pull request working see Issue #527

Contributor

qwcode commented May 13, 2012

I'm willing to rebase my userscheme branch on my locally updated develop branch (and retest) and then force push userscheme to my origin. github shouldn't complain then about you merging userscheme, but I guess that would blow away all the comment references in this pull request. so probably not desired?

Contributor

pnasrat commented May 13, 2012

Are you on #pip on freenode? May be easier to discuss there.

Contributor

qwcode commented May 14, 2012

ok, I think I did what we talked about on #pip
there's was just a minor conflict in util.py in the import lines.
tests still look good.

Contributor

qwcode commented May 23, 2012

Carl/Paul:

I fear the scope of this branch has gotten too large for anyone to feel comfortable merging it.
I'm willing break this up into a series of smaller pull requests that build up to the same result.
The commit history in the smaller pulls would be much cleaner a 2nd time through, now that I've sorted thru all the issues.

let me know, if you think that would help?

Contributor

pnasrat commented May 23, 2012

If you feel willing and more comfortable doing that - that's fine with me.

However I'm probably happy enough to merge this shortly - I was hoping to fix #530 first so we have a good green build to depend on.

Contributor

qwcode commented May 23, 2012

hmm...It's hard to decide. I know I'm more making more work for myself. : )
I'm inclined to be really conservative knowing pip is so widely used, and honestly I'm pretty new to contributing to a major project like this.
A series of smaller pulls with clear/concise commit histories would probably serve the history of the project better.
I have time this weekend, so I could start on it.

OTOH, if that sounds like more work for you, I understand.

Contributor

pnasrat commented May 23, 2012

That works for me, thanks for all your work on this! I'm travelling back home this weekend so I wouldn't expect responsiveness, but I'll try address next week.

Contributor

carljm commented May 23, 2012

I'm with Paul - I wouldn't have asked you to do that extra work, but I do find more smaller pull requests easier to deal with when possible.

Contributor

qwcode commented May 29, 2012

here's my plan for breaking this up
https://gist.github.com/2822510

I'll reference the plan when I do requests.

Contributor

qwcode commented Jul 8, 2012

closing, as this request is being handled in smaller parts:
https://gist.github.com/2822510

@qwcode qwcode closed this Jul 8, 2012

@pnasrat pnasrat referenced this pull request Nov 12, 2012

Merged

Rework command handling v2 #721

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment