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

Port python deps setup to windows #257

Merged
merged 15 commits into from
Oct 15, 2020
Merged
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
name: Test Python Package Installation
name: Test Python Package Installation on Linux

on:
push:
Expand Down
61 changes: 61 additions & 0 deletions .github/workflows/python-deps-windows.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
name: Test Python Package Installation on Windows


on:
push:
branches: [main, v1]
pull_request:

jobs:

test-setup-python-scripts:
runs-on: windows-latest
strategy:
fail-fast: false
matrix:
include:
- test_dir: python-setup/tests/pipenv/requests-2
python_version: 2
- test_dir: python-setup/tests/pipenv/requests-3
python_version: 3

- test_dir: python-setup/tests/poetry/requests-2
python_version: 2
- test_dir: python-setup/tests/poetry/requests-3
python_version: 3

- test_dir: python-setup/tests/requirements/requests-2
python_version: 2
- test_dir: python-setup/tests/requirements/requests-3
python_version: 3

- test_dir: python-setup/tests/setup_py/requests-2
python_version: 2
- test_dir: python-setup/tests/setup_py/requests-3
python_version: 3

steps:
# Checks-out your repository under $GITHUB_WORKSPACE, so your job can access it
- uses: actions/checkout@v2

- name: Initialize CodeQL
uses: github/codeql-action/init@v1
with:
languages: python

- name: Test Auto Package Installation
run: |
$cmd = $Env:GITHUB_WORKSPACE + "\\python-setup\\install_tools.ps1"
powershell -File $cmd

cd $Env:GITHUB_WORKSPACE\\${{ matrix.test_dir }}
py -3 $Env:GITHUB_WORKSPACE\\python-setup\\auto_install_packages.py C:\\hostedtoolcache\\windows\\CodeQL\\0.0.0-20200826\\x64\\codeql
- name: Setup for extractor
run: |
echo $Env:CODEQL_PYTHON

py -3 $Env:GITHUB_WORKSPACE\\python-setup\\tests\\from_python_exe.py $Env:CODEQL_PYTHON
- name: Verify packages installed
run: |
$cmd = $Env:GITHUB_WORKSPACE + "\\python-setup\\tests\\check_requests_123.ps1"
powershell -File $cmd ${{ matrix.python_version }}
29 changes: 25 additions & 4 deletions lib/init.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/init.js.map

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

57 changes: 44 additions & 13 deletions python-setup/auto_install_packages.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,13 @@ def _check_output(command):


def install_packages_with_poetry():
command = [sys.executable, '-m', 'poetry']
if sys.platform.startswith('win32'):
# In windows the default path were the deps are installed gets wiped out between steps,
# so we have to set it up to a folder that will be kept
os.environ['POETRY_VIRTUALENVS_PATH'] = os.path.join(os.environ['RUNNER_WORKSPACE'], 'virtualenvs')
RasmusWL marked this conversation as resolved.
Show resolved Hide resolved
try:
_check_call(['poetry', 'install', '--no-root'])
_check_call(command + ['install', '--no-root'])
except subprocess.CalledProcessError:
sys.exit('package installation with poetry failed, see error above')

Expand All @@ -33,44 +38,69 @@ def install_packages_with_poetry():
# virtualenv for the package, which was the case for using poetry for Python 2 when
# default system interpreter was Python 3 :/

poetry_out = _check_output(['poetry', 'run', 'which', 'python'])
poetry_out = _check_output(command + ['run', 'which', 'python'])
python_executable_path = poetry_out.decode('utf-8').splitlines()[-1]

if sys.platform.startswith('win32'):
# Poetry produces a path that starts by /d instead of D:\ and Windows doesn't like that way of specifying the drive letter.
# We completely remove it because it is not needed as everything is in the same drive (We are installing the dependencies in the RUNNER_WORKSPACE)
python_executable_path = python_executable_path[2:]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you explain the purpose of chopping off the start of the python_executable_path? If it is to get rid off the drive letter (eg C: ) then this code is likely wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Poetry and pipenv display the path wrongly. They produce /d/a/codeql-python-autobuild-playground/virtualenvs/requests-2-z_EHYAbo/Scripts/python instead of D:\a\codeql-python-autobuild-playground\codeql-action-python-autoinstall\Scripts\python. Windows doesn't like that way of specifying the drive letter. I completely removed because it is not needed as everything is in the same drive (We are installing the dependencies in the RUNNER_WORKSPACE)

Copy link
Member

Choose a reason for hiding this comment

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

since I also asked about this, can we maybe just add that comment to the code? 😊

Copy link
Collaborator

@aibaars aibaars Oct 8, 2020

Choose a reason for hiding this comment

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

The /d silly paths are likely due to cygwin or msys. You might want to check whether the paths are bad before chopping off letters. You never know when they'll replace pipenv and poetry with more windows-like versions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder why poetry or pipenv would behave this way. These are python programs. Are you accidentally running them with a non-windows (ie. cygwin/msys) python binary?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps you should invoke poetry like py -3 -m poetry or similar or even use sys.executable like [sys.executable like , '-m', 'poetry', 'run', 'which', 'python']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm now calling it with py -3 -m poetry and is still producing the same output, so I still need to chop off the start of the string

RasmusWL marked this conversation as resolved.
Show resolved Hide resolved
return python_executable_path


def install_packages_with_pipenv():
command = [sys.executable, '-m', 'pipenv']
if sys.platform.startswith('win32'):
# In windows the default path were the deps are installed gets wiped out between steps,
# so we have to set it up to a folder that will be kept
os.environ['WORKON_HOME'] = os.path.join(os.environ['RUNNER_WORKSPACE'], 'virtualenvs')
try:
_check_call(['pipenv', 'install', '--keep-outdated', '--ignore-pipfile'])
_check_call(command + ['install', '--keep-outdated', '--ignore-pipfile'])
except subprocess.CalledProcessError:
sys.exit('package installation with pipenv failed, see error above')

pipenv_out = _check_output(['pipenv', 'run', 'which', 'python'])
pipenv_out = _check_output(command + ['run', 'which', 'python'])
python_executable_path = pipenv_out.decode('utf-8').splitlines()[-1]

if sys.platform.startswith('win32'):
# Pipenv produces a path that starts by /d instead of D:\ and Windows doesn't like that way of specifying the drive letter.
# We completely remove it because it is not needed as everything is in the same drive (We are installing the dependencies in the RUNNER_WORKSPACE)
python_executable_path = python_executable_path[2:]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks strange to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the other comment

return python_executable_path


def _create_venv(version: int):
# create temporary directory ... that just lives "forever"
venv_path = mkdtemp(prefix='codeql-action-python-autoinstall-')
venv_path = os.path.join(os.environ['RUNNER_WORKSPACE'], 'codeql-action-python-autoinstall')
print ("Creating venv in " + venv_path, flush = True)

# virtualenv is a bit nicer for setting up virtual environment, since it will provide
# up-to-date versions of pip/setuptools/wheel which basic `python3 -m venv venv` won't

if version == 2:
_check_call(['python2', '-m', 'virtualenv', venv_path])
elif version == 3:
_check_call(['python3', '-m', 'virtualenv', venv_path])
if sys.platform.startswith('win32'):
if version == 2:
_check_call(['py', '-2', '-m', 'virtualenv', venv_path])
elif version == 3:
_check_call(['py', '-3', '-m', 'virtualenv', venv_path])
else:
if version == 2:
_check_call(['python2', '-m', 'virtualenv', venv_path])
elif version == 3:
_check_call(['python3', '-m', 'virtualenv', venv_path])

return venv_path


def install_requirements_txt_packages(version: int):
venv_path = _create_venv(version)

venv_pip = os.path.join(venv_path, 'bin', 'pip')
venv_python = os.path.join(venv_path, 'bin', 'python')

if sys.platform.startswith('win32'):
venv_pip = os.path.join(venv_path, 'Scripts', 'pip')
venv_python = os.path.join(venv_path, 'Scripts', 'python')

try:
_check_call([venv_pip, 'install', '-r', 'requirements.txt'])
except subprocess.CalledProcessError:
Expand All @@ -81,9 +111,14 @@ def install_requirements_txt_packages(version: int):

def install_with_setup_py(version: int):
venv_path = _create_venv(version)

venv_pip = os.path.join(venv_path, 'bin', 'pip')
venv_python = os.path.join(venv_path, 'bin', 'python')

if sys.platform.startswith('win32'):
venv_pip = os.path.join(venv_path, 'Scripts', 'pip')
venv_python = os.path.join(venv_path, 'Scripts', 'python')

try:
# We have to choose between `python setup.py develop` and `pip install -e .`.
# Modern projects use `pip install -e .` and I wasn't able to see any downsides
Expand Down Expand Up @@ -135,10 +170,6 @@ def install_packages(codeql_base_dir) -> Optional[str]:

codeql_base_dir = sys.argv[1]

# The binaries for packages installed with `pip install --user` are not available on
# PATH by default, so we need to manually add them.
os.environ['PATH'] = os.path.expanduser('~/.local/bin') + os.pathsep + os.environ['PATH']

python_executable_path = install_packages(codeql_base_dir)

if python_executable_path is not None:
Expand Down
13 changes: 13 additions & 0 deletions python-setup/install_tools.ps1
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#! /usr/bin/pwsh

py -2 -m pip install --user --upgrade pip setuptools wheel
py -3 -m pip install --user --upgrade pip setuptools wheel

# virtualenv is a bit nicer for setting up virtual environment, since it will provide up-to-date versions of
# pip/setuptools/wheel which basic `python3 -m venv venv` won't
py -2 -m pip install --user virtualenv
py -3 -m pip install --user virtualenv

# poetry 1.0.10 has error (https://github.com/python-poetry/poetry/issues/2711)
py -3 -m pip install --user poetry!=1.0.10
py -3 -m pip install --user pipenv
28 changes: 28 additions & 0 deletions python-setup/tests/check_requests_123.ps1
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#! /usr/bin/pwsh

$EXPECTED_VERSION=$args[0]

$FOUND_VERSION="$Env:LGTM_PYTHON_SETUP_VERSION"
$FOUND_PYTHONPATH="$Env:LGTM_INDEX_IMPORT_PATH"

write-host "FOUND_VERSION=$FOUND_VERSION FOUND_PYTHONPATH=$FOUND_PYTHONPATH "

if ($FOUND_VERSION -ne $EXPECTED_VERSION) {
write-host "Script told us to use Python $FOUND_VERSION, but expected $EXPECTED_VERSION"
exit 1
} else {
write-host "Script told us to use Python $FOUND_VERSION, which was expected"
}

$env:PYTHONPATH=$FOUND_PYTHONPATH

$INSTALLED_REQUESTS_VERSION = (py -3 -c "import requests; print(requests.__version__)")

$EXPECTED_REQUESTS="1.2.3"

if ($INSTALLED_REQUESTS_VERSION -ne $EXPECTED_REQUESTS) {
write-host "Using $FOUND_PYTHONPATH as PYTHONPATH, we found version $INSTALLED_REQUESTS_VERSION of requests, but expected $EXPECTED_REQUESTS"
exit 1
} else {
write-host "Using $FOUND_PYTHONPATH as PYTHONPATH, we found version $INSTALLED_REQUESTS_VERSION of requests, which was expected"
}
Loading