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

Make create_virtualenv not assume python is the executable name. #2

Closed
wants to merge 1 commit into from

Conversation

ammaraskar
Copy link
Member

Ran into this while trying to run under windows.

First of all, my executable is called python_d.exe (the default name from the windows build target)
and it wasn't under the bin directory, as you'd expect on a unix system.

Found this note on the venv documentation explaining it: https://virtualenv.pypa.io/en/stable/userguide/#windows-notes

if os.name == "nt":
venv_python = os.path.join(venv_path, 'Scripts', python_executable)
else:
venv_python = os.path.join(venv_path, 'bin', python_executable)
Copy link
Member

Choose a reason for hiding this comment

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

Please keep just "python" on UNIX. It's common that I rename the Python binary when I try a patched version of Python, only use basename(sys.executable) on Windows.

You might try to support OS X:

python_executable = 'python.exe' if sys.platform == 'darwin' else 'python'
venv_python = os.path.join(venv_path, 'bin', python_executable)

Copy link
Member

Choose a reason for hiding this comment

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

On OS X, the interpreter executable in the build directory may either be 'python' or 'python.exe' depending on the file system in use. But, when installed or in a virtual environment like the case here, it is always some form of 'python', not 'python.exe'.

@ammaraskar
Copy link
Member Author

Okay thanks for all the advice, I agree that on non-windows systems it would be best to assume the executable is python.

Fortunately on python3 this is an easily solved problem, using something like this. It gives the name of the python executable completely platform agnostic-ally.

def create_virtualenv_py3(venv, venv_path):
    class BenchmarkEnv(venv.EnvBuilder):
        def post_setup(self, context):
            self.executable_name = context.env_exe

    virtualenv = BenchmarkEnv(with_pip=True)

    venv_path = os.path.join(ROOT_DIR, 'venv', virtualenv_name())
    print("Creating the virtual environment %s" % venv_path)
    try:
        virtualenv.create(venv_path)
    except:
        if os.path.exists(venv_path):
            shutil.rmtree(venv_path)
        raise

    return virtualenv.executable_name

The problem that I ran into doing this is I used the following condition:

    try:
        import venv
        venv_python = create_virtualenv_py3(venv, venv_path)
    except ImportError:
        venv_python = create_virtualenv_py2(venv_path)

Unfortunately the import env is still successful on python2, because <module 'benchmark.venv' from '...\benchmarks\benchmark\venv.pyc'>

Would this be a good approach, and do you have any advice on solving that problem?

@vstinner
Copy link
Member

Use "from future import absolute_import" for py2 (not sure of the
syntax).

@vstinner
Copy link
Member

Ping @ammaraskar ?

@ammaraskar
Copy link
Member Author

Sorry, been a little busy. I'll update this tonight. It should work perfectly for py3, py2 will still need investigation on the optimal way to figure out the virtualenv executable.

Do not assume that the virtualenv executable lives under the bin folder, this is untrue for windows.
@ammaraskar
Copy link
Member Author

Updated, please take a look.

@@ -193,6 +206,25 @@ def create_virtualenv():
return venv_python


def create_virtualenv_py3(venv, venv_path):
class BenchmarkEnv(venv.EnvBuilder):
Copy link
Member

Choose a reason for hiding this comment

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

Why not using virtualenv module on Python2? Since we use the virtualenv command, the Python 2 virtualenv module must be available not?

Copy link
Member Author

Choose a reason for hiding this comment

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

The API isn't nearly as nice as the one in py3's stdlib: https://github.com/pypa/virtualenv/blob/master/virtualenv.py

I can look into it and see if its viable today.

@vstinner
Copy link
Member

I modified venv.py to always uses the virtualenv python, because I had issues running "python3 -m venv" inside a virtual environment on Fedora 24. It looks like ensurepip doesn't work when run in a virtual environment.

I also made the minimum change to fix Windows: commit 4f74a9d.

Finally, I reorganize all files to be able to install benchmarks.

So you need to rebase your pull request, sorry :-(

@ammaraskar
Copy link
Member Author

ammaraskar commented Aug 25, 2016

I'll close the change since you already commited the fix for windows and removed the usage of the venv module. At best, with the virtualenv package we can remove our Scripts directory logic since we can do this

    venv_path = os.path.join("venv", venv_name)
    venv_path, _, _, bin_path = virtualenv.path_locations(venv_path)
    if os.name == "nt":
        python_executable = os.path.basename(python)
        venv_python = os.path.join(bin_path, python_executable)
    else:
        venv_python = os.path.join(bin_path, 'python')

but that isn't too beneficial. The logic for actually getting the name of the executable is deep within the virtualenv package so I don't think there's a safe way to extract it: https://github.com/pypa/virtualenv/blob/master/virtualenv.py#L1202-L1212

@ammaraskar ammaraskar closed this Aug 25, 2016
@vstinner
Copy link
Member

"The logic for actually getting the name of the executable is deep within the virtualenv package so I don't think there's a safe way to extract it"

Hum. Maybe we can just reuse an existing tool like virtualenv wrapper or vex, rather than trying to reimplement our own code?

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.

3 participants