Pass 'allow_global' to resolve_deps #1097

techalchemy · 2017-11-19T18:48:34Z

Fixes Pipenv doesn't resolve --system python correctly. #1002

nateprewitt

Could we get a test on this and then I think we're set.

techalchemy · 2017-11-19T21:57:47Z

Once I remember how to cause the problem

galuszkak · 2017-11-20T07:48:59Z

@techalchemy I think there is bug here.
Second invocation of resolve_deps doesn't use allow_global this line (L1114):
https://github.com/kennethreitz/pipenv/pull/1097/files#diff-e1e68886e05fef92bb74ed1699e5179eR1114

I think You should add it.

nateprewitt · 2017-11-20T07:56:17Z

Yep, this is the same thing that wasn't addressed in the last attempted patch for this. Good spot @galuszkak, thanks!

- Add --system Flag to all CLI functions that interact with python environments - Add 'envvar' parameter to click options to parse into system flag if we miss it in environments.py

techalchemy · 2017-11-20T14:54:13Z

OK, and now we pray to the ci gods. That was a good catch, and I realized we aren't passing the --system flag through or picking it up in a majority of our CLI functions so I added it. That should be helpful

techalchemy · 2017-11-20T17:49:57Z

@kennethreitz and this is the rest of why i needed --system

kennethreitz · 2017-11-20T17:50:50Z

pipenv/cli.py

@@ -1764,7 +1766,7 @@ def do_py(system=False):
 @click.option('--dev', '-d', is_flag=True, default=False, help="Install package(s) in [dev-packages].")
 @click.option('--three/--two', is_flag=True, default=None, help="Use Python 3/2 when creating virtualenv.")
 @click.option('--python', default=False, nargs=1, help="Specify which version of Python virtualenv should use.")
-@click.option('--system', is_flag=True, default=False, help="System pip management.")
+@click.option('--system', is_flag=True, default=False, help="System pip management.", envvar='PIPENV_USE_SYSTEM')


we don't use envvar anywhere else -- I don't think it's inherently bad, but i think we should stick to the conventions established.

kk, I wasn't sure so I'll nix it

kennethreitz · 2017-11-20T18:01:20Z

I don't think the title of this pull request encapsulates what's going on here, which is a lot.

What are you trying to accomplish, specifically?

techalchemy · 2017-11-20T18:18:17Z

pipenv/cli.py

@@ -1970,7 +1972,7 @@ def uninstall(
        system = True

    # Ensure that virtualenv is available.
-    ensure_project(three=three, python=python)
+    ensure_project(three=three, python=python, system=system)


@kennethreitz are you ok with this change in particular? this is for uninstall

this essentially just doesn't create a virtualenv

techalchemy · 2017-11-20T19:14:24Z

This doesn't work, because now we still pipenv install --system for our CI pipeline, but then we pipenv run pytest in order to run the tests. Since pipenv install --system doesnt make a virtualenv we can't use pipenv run. I feel like the easiest solution and the best one for consistency is to keep the api call the same no matter what the user is doing, whether it just acts as a wrapper to a direct call or invokes something inside a virtualenv.

I don't know. What is the best option here?

nateprewitt · 2017-11-20T19:59:08Z

From a reviewing standpoint, can we get a few changes made

It's difficult to tell what this issue is about now, could we get a brief synopsis of the problem and the proposed solution in the original post?

There are now multiple sytlistic changes in the PR which make telling which changes are relevant and which are cosmetic difficult. Let's spin cleanup off to a new PR if we need to make those changes. We don't have a hard cap at 80 though, so I'm not sure they're needed.

The test also isn't very clear what it's trying to prove. Can we get a docstring on that?

techalchemy · 2017-11-20T21:27:57Z

Ok. Let me describe the problem.

I am a user. I am using pipenv with --system, inside of a docker container, but my pipenv install is not inside a virtualenv. I make a folder directly off my filesystem root. For some reason that is required to cause the bug.

When we resolve currently, we create a fake python version using which('python'). Under the circumstances I described, which('python') returns /bin/python because there is no project yet, there is no virtualenv, and --system doesn't get dropped through to the resolver. That causes an error.

I would like to note that this error doesn't occur in master due to the use of a backup resolver:

 python_path = which('python', allow_global=allow_global)
 backup_python_path = shellquote(sys.executable)

But I don't really feel like that is a justification for leaving what is definitively a bug in the code.

I have now spent a significant amount of time explaining why we should pass a necessary argument to a function that expects it, trying to prove that the issue did exist, create a test for a thing that can't currently be reproduced, and this feels pretty silly at this point so I'll undo all of my changes and if that still isn't sufficient I'll just close the pull request and we can leave the bug in the code.

galuszkak · 2017-11-21T05:23:32Z

create a test for a thing that can't currently be reproduced

@techalchemy in original issue I attached how to reproduce it. If You are unable to do so I can create instruction or code that reproduce that issue from ground, and put it in repository.

Thanks for hard work on that @techalchemy

techalchemy · 2017-11-21T05:34:17Z

@galuszkak no worries I grabbed your instructions and included them in the docstring for the test. It just took me awhile to figure out what exactly caused it to error out, turns out the projects first parent has to be / as well. We’ll see if this one passes muster.

kennethreitz · 2017-11-21T08:40:24Z

my inclination here is that when using install --system, lock and other commands are not useful or needed, and that virtualenv creation is neccessary for those features (e.g. the current implementation is just fine).

…

On Tue, Nov 21, 2017 at 12:34 AM, Dan Ryan ***@***.***> wrote: @galuszkak <https://github.com/galuszkak> no worries I grabbed your instructions and included them in the docstring for the test. It just took me awhile to figure out what exactly caused it to error out, turns out the projects first parent has to be / as well. We’ll see if this one passes muster. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/kennethreitz/pipenv/pull/1097#issuecomment-345921895>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHUVYhixDN7Y_kw2ZtSkubQQS4vrjHkks5s4mDdgaJpZM4QjfPM> .

galuszkak · 2017-11-21T08:55:10Z

@kennethreitz for me that contradicts purpose. So proposed solution with Docker workflow is using --system (per documentation), but when I want to pindown version through docker workflow You are saying it's not necessary. For me it doesn't matter if it's with --system flag or not, I expect that for example pipenv lock will work correctly with or without that flag, because people have different workflows, and if they are using docker they don't need virtualenv because they isolate on container level, and they still need Pipfile.lock for their peer workers when they will work on that codebase.

kennethreitz · 2017-11-21T09:20:43Z

I envision a dock or user generating a locked file before sending things to Docker, always

…

Sent from my iPad

On Nov 21, 2017, at 3:55 AM, Kamil Gałuszka ***@***.***> wrote: @kennethreitz for me that contradicts purpose. So proposed solution with Docker workflow is using --system (per documentation), but when I want to pindown version through docker workflow You are saying it's not necessary. For me it doesn't matter if it's with --system flag or not, I expect that for example pipenv lock will work correctly with or without that flag, because people have different workflows, and if they are using docker they don't need virtualenv because they isolate on container level, and they still need Pipfile.lock for their peer workers when they will work on that codebase. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

kennethreitz · 2017-11-21T09:21:39Z

In other words this functionality is offered for deployments not for development workflow

…

Sent from my iPad

On Nov 21, 2017, at 3:55 AM, Kamil Gałuszka ***@***.***> wrote: @kennethreitz for me that contradicts purpose. So proposed solution with Docker workflow is using --system (per documentation), but when I want to pindown version through docker workflow You are saying it's not necessary. For me it doesn't matter if it's with --system flag or not, I expect that for example pipenv lock will work correctly with or without that flag, because people have different workflows, and if they are using docker they don't need virtualenv because they isolate on container level, and they still need Pipfile.lock for their peer workers when they will work on that codebase. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

galuszkak · 2017-12-16T17:34:38Z

@techalchemy I think this PR needs to be rebased.

Is this PR waiting still for review of @kennethreitz ?

…l-resolving Pass 'allow_global' to resolve_deps

#1097 remerge

Pass 'allow_global' to resolve_deps

63934a4

- Fixes pypa#1002

nateprewitt requested changes Nov 19, 2017

View reviewed changes

Test --system flag

aa13648

Add system flag to all resolve_deps calls

12a05a5

- Add --system Flag to all CLI functions that interact with python environments - Add 'envvar' parameter to click options to parse into system flag if we miss it in environments.py

techalchemy requested a review from kennethreitz November 20, 2017 17:49

techalchemy added the Status: Requires Approval This issue requires additional approval to move forward. label Nov 20, 2017

kennethreitz reviewed Nov 20, 2017

View reviewed changes

techalchemy commented Nov 20, 2017

View reviewed changes

techalchemy added 2 commits November 20, 2017 13:22

Scrub needless system flag additions

e2730d7

Fix click formatting

ced0a15

Undo all the cleanup, fix tests

e244e45

techalchemy force-pushed the bugfix/1002-allow-global-resolving branch 2 times, most recently from f64e739 to e244e45 Compare November 20, 2017 22:03

techalchemy mentioned this pull request Nov 23, 2017

Pipenv doesn't resolve --system python correctly. #1002

Closed

Merge branch 'master' into bugfix/1002-allow-global-resolving

6163898

kennethreitz merged commit 546b399 into pypa:master Dec 18, 2017

kcchu pushed a commit to kcchu/pipenv that referenced this pull request Jan 17, 2018

Merge pull request pypa#1097 from techalchemy/bugfix/1002-allow-globa…

c37f0f8

…l-resolving Pass 'allow_global' to resolve_deps

techalchemy added the needs remerge label Jan 23, 2018

nateprewitt mentioned this pull request Jan 26, 2018

Revised master #1361

Merged

techalchemy mentioned this pull request Jan 27, 2018

#1097 remerge #1363

Merged

nateprewitt added a commit that referenced this pull request Jan 28, 2018

Merge pull request #1363 from pypa/hotfix/1097_remerge

72081cc

#1097 remerge

techalchemy deleted the bugfix/1002-allow-global-resolving branch April 1, 2018 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass 'allow_global' to resolve_deps #1097

Pass 'allow_global' to resolve_deps #1097

techalchemy commented Nov 19, 2017

nateprewitt left a comment

techalchemy commented Nov 19, 2017

galuszkak commented Nov 20, 2017

nateprewitt commented Nov 20, 2017

techalchemy commented Nov 20, 2017 •

edited

Loading

techalchemy commented Nov 20, 2017

kennethreitz Nov 20, 2017

techalchemy Nov 20, 2017

kennethreitz commented Nov 20, 2017

techalchemy Nov 20, 2017

techalchemy Nov 20, 2017

techalchemy commented Nov 20, 2017

nateprewitt commented Nov 20, 2017

techalchemy commented Nov 20, 2017

galuszkak commented Nov 21, 2017

techalchemy commented Nov 21, 2017

kennethreitz commented Nov 21, 2017 via email

galuszkak commented Nov 21, 2017

kennethreitz commented Nov 21, 2017 via email

kennethreitz commented Nov 21, 2017 via email

galuszkak commented Dec 16, 2017

Pass 'allow_global' to resolve_deps #1097

Pass 'allow_global' to resolve_deps #1097

Conversation

techalchemy commented Nov 19, 2017

nateprewitt left a comment

Choose a reason for hiding this comment

techalchemy commented Nov 19, 2017

galuszkak commented Nov 20, 2017

nateprewitt commented Nov 20, 2017

techalchemy commented Nov 20, 2017 • edited Loading

techalchemy commented Nov 20, 2017

kennethreitz Nov 20, 2017

Choose a reason for hiding this comment

techalchemy Nov 20, 2017

Choose a reason for hiding this comment

kennethreitz commented Nov 20, 2017

techalchemy Nov 20, 2017

Choose a reason for hiding this comment

techalchemy Nov 20, 2017

Choose a reason for hiding this comment

techalchemy commented Nov 20, 2017

nateprewitt commented Nov 20, 2017

techalchemy commented Nov 20, 2017

galuszkak commented Nov 21, 2017

techalchemy commented Nov 21, 2017

kennethreitz commented Nov 21, 2017 via email

galuszkak commented Nov 21, 2017

kennethreitz commented Nov 21, 2017 via email

kennethreitz commented Nov 21, 2017 via email

galuszkak commented Dec 16, 2017

techalchemy commented Nov 20, 2017 •

edited

Loading