Port python deps setup to windows #257

Daverlo · 2020-10-06T12:54:25Z

Closes https://github.com/github/code-scanning/issues/2108

This PR ports the auto install python deps functionality so it also works on windows workers.

I tested this branch with the django example and it worked as expected, providing two alerts:
https://github.com/dsp-testing/daverlo-djanjo-test/actions/runs/291392369
https://github.com/dsp-testing/daverlo-djanjo-test/security/code-scanning

@RasmusWL would you mind taking a look at the python scripts? I created a copy of auto-install-packages and modified it so it works on windows.

Merge / deployment checklist

Confirm this change is backwards compatible with existing workflows.
Confirm the readme has been updated if necessary.

RasmusWL

I would rather have seen auto_install_packages.py being adapted to work on both Windows and Linux instead of duplicating the script and making adjustments (using sys.platform or os.name). I really think that is the right approach.

As an example of why this is important, the version of the linux script you ported is not the most up to date (compare with https://github.com/github/codeql-action/blob/main/python-setup/auto_install_packages.py)

I also don't understand the need to strip out the first 3 characters when using pipenv and poetry, can you explain?

Daverlo · 2020-10-07T11:00:50Z

I also don't understand the need to strip out the first 3 characters when using pipenv and poetry, can you explain?

They were producing a path like
/d/a/codeql-python-autobuild-playground/virtualenvs/autoinstall-test-FP1YIZRL-py2.7/Scripts/python

and windows didn't like the /d prefix, it complained about the file not existing. 🤷

RasmusWL

Nice, this version looks much better, good job 🥇

There is an issue with hardcoding version of python, but otherwise this looks good to me!

RasmusWL · 2020-10-07T12:48:02Z

python-setup/auto_install_packages.py

    # PATH by default, so we need to manually add them.
-    os.environ['PATH'] = os.path.expanduser('~/.local/bin') + os.pathsep + os.environ['PATH']
+    if sys.platform.startswith('win32'):
+        os.environ['PATH'] = os.path.expandvars('%APPDATA%\Python\\Python38\\scripts') + os.pathsep + os.environ['PATH']


This hardcoded Python version (3.8) looks a bit scary to me. Is there a way to not hardcode the version? 😬

That is the default version of Python in Windows workers and it's always there, no matter if you add the setup-python action with like 2.7 or 3.5 or whatever, in fact, that action is not needed at all. I

prefer to leave it like this if you don't mind, right now it is working for all the cases and I don't think its the best use of my time at this moment.

https://github.com/dsp-testing/codeql-python-autobuild-playground/runs/1213872774?check_suite_focus=true
https://github.com/dsp-testing/codeql-python-autobuild-playground/runs/1221157769?check_suite_focus=true
https://github.com/dsp-testing/codeql-python-autobuild-playground/runs/1221281906?check_suite_focus=true

3.8 is "always" there until someone updates the virtual machine image. 3.9 was just released, so it may change sooner than you think. You should be able to get the USER_BASE directory from the site module. See also https://docs.python.org/3/library/site.html#site.USER_BASE

If I understand correctly you should also be able to set PYTHONUSERBASE to have full control over the install location. You could consider stuffing things in the RUNNER_TEMP folder to avoid interfering with future other jobs on self_hosted workers.

I think it's a good idea @aibaars is pointing out, and I'm probably to blame for not thinking about this in the first place.

I think @aibaars added this comment to the wrong thread:

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']

For solving this problem, I think using sys.executable instead of hardcoding the 3.8 path is a very good trade-off.

We just need to make sure that we only invoke this script with Python 3, since we only install pipenv/poetry for Python 3. (but I guess the JS code already does ensures this, right?)

I wasn't able to make it work with the sys.executable

_check_call([sys.executable like , '-m', 'poetry', 'install', '--no-root']) ^ SyntaxError: invalid syntax

But I managed to make it work with the py -3!!

We just need to make sure that we only invoke this script with Python 3, since we only install pipenv/poetry for Python 3. (but I guess the JS code already does ensures this, right?)

Yep, it is always called with py -3

Real code without syntax error would be

_check_call([sys.executable, '-m', 'poetry', 'install', '--no-root'])

python-setup/auto_install_packages.py

aibaars · 2020-10-08T09:01:02Z

python-setup/auto_install_packages.py

    python_executable_path = poetry_out.decode('utf-8').splitlines()[-1]

+    if sys.platform.startswith('win32'):
+        python_executable_path = python_executable_path[2:]


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.

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)

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

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.

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?

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']

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

python-setup/auto_install_packages.py

aibaars · 2020-10-08T09:02:57Z

python-setup/auto_install_packages.py

    python_executable_path = pipenv_out.decode('utf-8').splitlines()[-1]

+    if sys.platform.startswith('win32'):
+        python_executable_path = python_executable_path[2:]


This looks strange to me.

See the other comment

python-setup/auto_install_packages.py

aibaars · 2020-10-08T09:14:27Z

python-setup/auto_install_packages.py

    # PATH by default, so we need to manually add them.
-    os.environ['PATH'] = os.path.expanduser('~/.local/bin') + os.pathsep + os.environ['PATH']
+    if sys.platform.startswith('win32'):
+        os.environ['PATH'] = os.path.expandvars('%APPDATA%\Python\\Python38\\scripts') + os.pathsep + os.environ['PATH']


3.8 is "always" there until someone updates the virtual machine image. 3.9 was just released, so it may change sooner than you think. You should be able to get the USER_BASE directory from the site module. See also https://docs.python.org/3/library/site.html#site.USER_BASE

src/init.ts

aibaars · 2020-10-08T09:20:03Z

python-setup/auto_install_packages.py

    # PATH by default, so we need to manually add them.
-    os.environ['PATH'] = os.path.expanduser('~/.local/bin') + os.pathsep + os.environ['PATH']
+    if sys.platform.startswith('win32'):
+        os.environ['PATH'] = os.path.expandvars('%APPDATA%\Python\\Python38\\scripts') + os.pathsep + os.environ['PATH']


If I understand correctly you should also be able to set PYTHONUSERBASE to have full control over the install location. You could consider stuffing things in the RUNNER_TEMP folder to avoid interfering with future other jobs on self_hosted workers.

Co-authored-by: Arthur Baars <aibaars@github.com>

Daverlo · 2020-10-13T15:59:18Z

@aibaars @RasmusWL I got this working using the py -3 -m poetry/py -3 -m pipenv calls, so no more hardcoded python version. Could you re-review the PR please?

RasmusWL

A few minor nitpicks, but generally I won't be against merging it now 😊

python-setup/auto_install_packages.py

RasmusWL

Nice, looks good to me now 👍

Thanks for working through all the things 💪

Daverlo added 4 commits October 6, 2020 14:14

Port python deps setup to windows

74afd3c

Update python-deps-windows test_dir paths

2748e6c

Update installPythonDeps for windows

0f2fa46

Only run tests on main and v1

526dac0

RasmusWL requested changes Oct 6, 2020

View reviewed changes

Combine auto_install scripts

e97bdbd

Daverlo force-pushed the daverlo/python-windows branch from 4cb342f to e97bdbd Compare October 7, 2020 10:55

RasmusWL reviewed Oct 7, 2020

View reviewed changes

aibaars reviewed Oct 8, 2020

View reviewed changes

Daverlo and others added 7 commits October 8, 2020 11:59

Apply suggestions from code review

424a9cf

Co-authored-by: Arthur Baars <aibaars@github.com>

Add powershell to call

735ec7d

Add call to py

e3d2d4a

Add missing [

9bc8c56

Merge branch 'main' into daverlo/python-windows

72e430f

Call pipenv and poetry through python

0d97ea8

Merge branch 'main' into daverlo/python-windows

e63596e

Use sys.executable

2c3dafc

RasmusWL requested changes Oct 15, 2020

View reviewed changes

python-setup/auto_install_packages.py Outdated Show resolved Hide resolved

python-setup/auto_install_packages.py Show resolved Hide resolved

python-setup/auto_install_packages.py Show resolved Hide resolved

Address comments

7d1f309

RasmusWL approved these changes Oct 15, 2020

View reviewed changes

Merge branch 'main' into daverlo/python-windows

e0f7838

Daverlo merged commit c8b8c04 into main Oct 15, 2020

Daverlo deleted the daverlo/python-windows branch October 15, 2020 12:48

github-actions bot mentioned this pull request Oct 19, 2020

Merge main into v1 #269

Merged

snyk-bot mentioned this pull request Apr 25, 2023

[Snyk] Fix for 9 vulnerabilities aliscco/codeql-action#101

Open

Elsokary22 mentioned this pull request Jul 1, 2023

[Snyk] Security upgrade tough-cookie from 3.0.1 to 4.1.3 Elsokary22/codeql-action#16

Open

Port python deps setup to windows #257

Port python deps setup to windows #257

Uh oh!

Conversation

Daverlo commented Oct 6, 2020

Merge / deployment checklist

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

Daverlo commented Oct 7, 2020

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aibaars Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Daverlo commented Oct 13, 2020

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aibaars Oct 8, 2020 •

edited

Loading