Skip to content

`deactivate` conflicts with Python's virtualenv #14

Closed
chrsm opened this Issue Mar 5, 2014 · 23 comments

4 participants

@chrsm
chrsm commented Mar 5, 2014

Thankfully, virtphp itself doesn't have any other conflicting commands, but deactivate is used by virtualenv already - shouldn't this be renamed?

@ramsey ramsey added the question label Mar 5, 2014
@ramsey
ramsey commented Mar 5, 2014

Would the conflict only occur if you have activated a virtualenv environment and a virtphp environment at the same time?

@chrsm
chrsm commented Mar 5, 2014

Yes, that's true. But for anyone who has a multi-language set up, this prevents using deactivate for either virtphp or virtualenv, depending on which is activated last.

@jwoodcock
VirtPHP member

It can happen that someone forgets to exit a virtualenv session before activating virtphp. I've been known to just leave my virtualenvs up for a long while. Bad practice I know.

We could do one of two things:

1) Change the command to something like exit or exitvirt, or goodday to add some personality to the software.

2) As you mentioned off channel Ben, we could look to see if a virtualenv is already opened and request they exit before we startup.

@ramsey
ramsey commented Mar 5, 2014

It looks like virtualenv sets a VIRTUAL_ENV environment variable that we can check for. We are also setting our own VIRTPHP_ENV_PATH environment variable when we activate a virtphp environment. I can't find whether we're checking to see if an existing environment exists, though.

@jwoodcock
VirtPHP member

I thought we did have a check for virtphp environments. I'll look at the code base as well.

@jwoodcock
VirtPHP member

Okay, i think this is what we did. We are not checking, but we did test switching virt env while building and found that there was no conflicts, the env just switched.

@jwoodcock
VirtPHP member

Looks like we can put in a simple check on line 83 of: https://github.com/virtphp/virtphp/blob/master/src/Virtphp/Command/CreateCommand.php to check for the VIRTUAL_ENV value.

@ramsey
ramsey commented Mar 5, 2014

I don't think we want to check when creating a virtphp environment. I think we want to check when activating. So, that would be in the res/activate.sh script.

@chrsm
chrsm commented Mar 5, 2014

I think that having the ability to have both environments on is desirable - what about renaming deactivate to deactivatephp in the generated env/bin/activate?

@jwoodcock
VirtPHP member

ramsey, good catch. Doh.

chrsm, I believe we've namespaced all our variables and functions to pretty close to unique but we'll have to do some collision testing before feeling comfortable having both running at the same time.

@chrsm
chrsm commented Mar 5, 2014

Sure. Just wanted to throw the idea out there. Thanks for your hard work on virtphp.

@jwoodcock
VirtPHP member

Looking over our activate script, I'm afraid we'll have some funky behavior even though we have namespaced our variables well. My concern is mostly around the prompt. We follow virtualenv's lead on changing the prompt in the a very similar way. One would win, the last one activate, and remove all visual keys that the other is still running.

For less confusion I would suggest we add the check and prevent activation. What do you say Ben and Jordan?

@jwoodcock
VirtPHP member
@jakerella
VirtPHP member

Agreed that things would get hairy with multiple environments running. I see the use case, but I don't think the authors of virtualenv thought of this use case either. That is, the two would have to both play nicely together, otherwise us changing things would be fragile.

@ramsey
ramsey commented Mar 5, 2014

I agree that we should do the check in the activate script, and if another virtphp or virtualenv environment is running, we prevent activation until the other environment has been deactivated.

@jwoodcock
VirtPHP member

@ramsey Do you think we need to prevent when switching from virtphp env to virtphp env? Our script resets all variables on run so we won't ever have a collision there.

@ramsey
ramsey commented Mar 5, 2014

I think we should at least warn the user and prompt them to see if it's okay to change virtphp envs.

@jakerella
VirtPHP member

I really could have sworn we did that check..... Correction, I could have sworn I wrote that check...

@jwoodcock
VirtPHP member

Ramsey: I'm okay with that.
Jordan: We talked about it, switched a few times, and felt that was acceptable.

@jwoodcock
VirtPHP member

Opening an issue for this now.

@jwoodcock
VirtPHP member

And by that I mean, converting this to a resolvable issue.

@jwoodcock jwoodcock assigned jwoodcock and unassigned jwoodcock Mar 5, 2014
@ramsey
ramsey commented Mar 6, 2014

Implemented in PR #16.

@ramsey ramsey closed this Mar 6, 2014
@chrsm
chrsm commented Mar 10, 2014

Bit late here, but 👍. You guys rock, Thanks for virtphp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.