Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support installing the package inside a virtual environment. #40

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

bartfeenstra
Copy link
Contributor

@bartfeenstra bartfeenstra commented Aug 5, 2017

The change to setup.py is necessary, but I'd have to look up the exact reason. Without the change, though, running this package using an environment-provided Python executable (which is what virtual environments do) is not possible on my system (testing on Python 2.7).

The README now includes step-by-step instructions on how to set this up.

This PR works well with #39.

If #39 is merged before this one, this PR must first update README.md to change the manual Python package installations to pip install -r requirements.txt.

Copy link
Owner

@wendlers wendlers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

I don't get why the procedure for setting up a VENV should be as described here. Wouldn’t it be sufficient to do the following from within the mpfshell sources dir?

python3 -m virtualenv venv
source venv/bin/activate
pip install -r requirements.txt
python setup.py install

Or maybe I am missing the point here?

@bartfeenstra
Copy link
Contributor Author

Hi! I'm not sure I understand your question. Aren't the commands you shared here pretty much the same as the one in the PR, with the exception of a few clarifications as the python executable and virtual environment paths are up to the user? I guess it's a matter of how detailed you'd like the instructions to be.

@wendlers
Copy link
Owner

wendlers commented Aug 7, 2017

Hi,
I see the following difference between the two scripts:

# this is in both approaches the same ...
$python_global -m virtualenv venv

# but the created venv is not activated, and thus the pip command will affect the 
# default Python environment, and not the virtual one 
pip install -r requirements.txt
# also this all affects the non virtual env
sudo python setup.py build -e "/usr/bin/env python"
sudo python setup.py install

I don't see how anything is installed in the venv here ..

So might be that only the activation is missing here. But still, then we don't need sudo, and also this is then is the same as already described in the README at the very end.

@bartfeenstra
Copy link
Contributor Author

The activation was indeed missing and I had indeed missed the other documentation further in the file. I tried reproducing my original problem with the shebang ('fixed' by passing -e to the build script) but can't.

I also noticed you published the package on PyPI today (thanks!), so perhaps it would be best to recommend that approach combined with a virtual environment at the top of the file. The instructions could be made shorter and less likely to fail this way. What do you think of that?

@wendlers
Copy link
Owner

wendlers commented Aug 7, 2017

Hi,
sounds valid. Having the shortened instructions for virtual setup near the system wide instructions, and removing the complete last section from the README about the virtualenv might definitely clear things up! I think it would be good to have instructions for the pip variant and the non pip (from source) variante (since pip offers released version, and people might prefer using stuff from head). So sketched up roughly, it might come to something like this:

...
From PiPy

To install the latest release from PyPi system-wide:

sudo pip install mpfshell

To install in a virtual environment

new instructions for venv (create, activate, pip install)
...

And

...
From Source

Clone this repository:

....

To install for Python 2, execute the following:
...
To install for Python 3, execute the following:
...

To install in a virtual environmen:
new instructions for venv (create, activate, pip with requirements, setup.py)

...
remove section "Running the Shell in a Virtual Environment"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants