Python deps setup #155

Daverlo · 2020-08-20T17:32:28Z

Here I'm adding the support for the auto-install python dependancies. The PR adds the necessary scripts to the python-setup folder. The scripts were developed by @RasmusWL

This process is done in two steps, in the init action we install the necessary tools and try to get the dependencies, in the analyse step we configure the python extractor so it is able to find them.

The whole init process is not done in the runner as we don't want our cli to be messing around with things in the system. For the self hosted runners we don't try to install the tools for similar reasons, but we do try to get the dependencies, and that should work as long as the tools are present in the system.

There is a possibility of disabling this process by setting the input setup-python-dependencies to false.

The dependencies can be installed manually, and by providing the CODEQL_PYTHON env var in the analyse step the extractor should be able to pick them up. That en var should contain the path to the python executable that has access to the dependencies.

This comment contains more details about the different usage possibilities: https://github.com/github/codeql-python-team/issues/103#issuecomment-663252898

Merge / deployment checklist

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

Update Python scripts for "Python deps setup"

marcogario

This looks good! I think you should probably get a 👍 from @RasmusWL as well.

I left a few minor comments. I think my main concern is that there are quite a few situations that we want to support (poetry, pip, etc.), and it seems worthwhile to add a test scenario for each of them, and make sure we are indeed handling them correctly.

marcogario · 2020-09-24T08:41:20Z

python-setup/auto_install_packages.py

@@ -0,0 +1,141 @@
+#!/usr/bin/env python3


Could you add a docstring describing what this script is for and any extra assumptions.

python-setup/auto_install_packages.py

marcogario · 2020-09-24T08:48:49Z

python-setup/auto_install_packages.py

+
+    if python_executable_path is not None:
+        print("Setting CODEQL_PYTHON={}".format(python_executable_path))
+        print("::set-env name=CODEQL_PYTHON::{}".format(python_executable_path))


So the main purpose of this script is to do the installation and then set the env variable?

Is it an error if we do not suceed? do we want a different exit code?

marcogario · 2020-09-24T08:49:55Z

python-setup/auto_install_packages.py

+
+
+def install_packages() -> str:
+    if os.path.exists('poetry.lock'):


You are expecting this to run in the checkout folder of the project, right?

python-setup/extractor_version.py

marcogario · 2020-09-24T09:03:24Z

python-setup/auto_install_packages.py

+    elif version == 3:
+        _check_call(['python3', '-m', 'virtualenv', venv_path])


Can get_extractor_version return anything else apart from 2 or 3? If it can, then we do not install the virtual env, right? Is this expected? If it cannot then I would change this into:

Suggested change

elif version == 3:

_check_call(['python3', '-m', 'virtualenv', venv_path])

else:

assert version == 3

_check_call(['python3', '-m', 'virtualenv', venv_path])

it will only return 2 or 3.

marcogario · 2020-09-24T09:04:59Z

python-setup/install_tools.sh

+# subsequent actions in the current job, and not the current action.
+export PATH="$HOME/.local/bin:$PATH"
+
+python2 -m pip install --user --upgrade pip setuptools wheel


Is it worth checking that you actually have a python2 executable?

For a long term solution it probably would be worth checking if python2 is not available and install it otherwise. For now I am not too worried -- as long as we have some automated tests that run regularly, that should catch this case right? (what do you think @Daverlo)

python-setup/install_tools.sh

marcogario · 2020-09-24T09:12:16Z

python-setup/install_tools.sh

+python3 -m pip install --user virtualenv
+
+# venv is required for installation of poetry or pipenv (I forgot which)
+sudo apt-get install -y python3-venv


Ok, I dig up https://github.com/github/codeql-python-team/issues/103#issuecomment-692053819 and it seems we are expecting only the linux cloud runners to use this for now.

I haven't check yet but it would be good to have a comment saying where this should (and should not) work in the init, as that is where you can enable/disable this.

RasmusWL

Thanks for the review @marcogario 👍

The context for the Python scripts (that I wrote) is that it's something I made fairly quickly, and having in mind this should hopefully only be used for a limited time, I didn't invest a lot of effort into making this the highest quality code. I'll fix up some of the most blatant errors and bad coding practices.

RasmusWL · 2020-09-24T13:20:53Z

python-setup/extractor_version.py

+    else:
+        return buildtools.discover.get_version()


I think pylint has a standard rule making the same recommendation as you propose here, but I strongly disagree, since the control flow of the code is much easier to follow with the explicit else (in my opinion).

RasmusWL · 2020-09-24T13:23:35Z

python-setup/install_tools.sh

+# subsequent actions in the current job, and not the current action.
+export PATH="$HOME/.local/bin:$PATH"
+
+python2 -m pip install --user --upgrade pip setuptools wheel


For a long term solution it probably would be worth checking if python2 is not available and install it otherwise. For now I am not too worried -- as long as we have some automated tests that run regularly, that should catch this case right? (what do you think @Daverlo)

RasmusWL · 2020-09-24T13:24:39Z

python-setup/auto_install_packages.py

+    elif version == 3:
+        _check_call(['python3', '-m', 'virtualenv', venv_path])


it will only return 2 or 3.

RasmusWL · 2020-09-24T13:24:59Z

python-setup/auto_install_packages.py

+
+
+def install_packages() -> str:
+    if os.path.exists('poetry.lock'):


RasmusWL · 2020-09-24T13:27:25Z

python-setup/auto_install_packages.py

+            print('Found Pipfile, will install packages with Pipenv', flush=True)
+        return install_packages_with_pipenv()
+
+    version = extractor_version.get_extractor_version(sys.argv[1], quiet=False)


We should be passing in codeql_base_dir for sure 👍

We could run get_extractor_version before calling install_packages, I just didn't... if you feel very strongly about this one, let me know.

RasmusWL · 2020-09-24T13:28:14Z

python-setup/auto_install_packages.py

+            print('Found Pipfile, will install packages with Pipenv', flush=True)
+        return install_packages_with_pipenv()
+
+    version = extractor_version.get_extractor_version(sys.argv[1], quiet=False)


ehh, naming is apparently not too obvious. extractor_version.get_extractor_version returns the Python version the extractor thinks this repo is in.

python-setup/auto_install_packages.py

Co-authored-by: Marco Gario <marcogario@github.com>

RasmusWL · 2020-09-24T13:46:13Z

@Daverlo I comitted directly to the branch with my changes. That means I actually haven't run the script, which is kind of scary. Is it easy to add tests so we know the script is working? (or have they already been added?)

Daverlo · 2020-09-25T08:57:32Z

Tests are not going to be too helpful, if it doesn't work we just show a warning, and those are not easily visible. In this action this process failed on purpose https://github.com/dsp-testing/daverlo-djanjo-test/actions/runs/272089425, but you see all green no warning in the actions page https://github.com/dsp-testing/daverlo-djanjo-test/actions and in the pr page https://github.com/dsp-testing/daverlo-djanjo-test/pull/2

I would propose that we run the different tests on the django-test repo (basically update the workflows that I created for https://github.com/github/codeql-python-team/issues/103#issuecomment-663252898 so they run every day).

Do you have any suggestion on how can we do better testing?

python-setup/auto_install_packages.py

marcogario · 2020-09-25T09:51:29Z

Do you have any suggestion on how can we do better testing?

I think the typescript wrapper is not too complex. I would expect we can test the python script directly. Ideally these should all complete outputting the env for CODEQL and you can check that. Maybe create a new workflow file for this.

Daverlo · 2020-09-29T08:44:00Z

I've added a modified version of the playground workflow for testing the scripts. https://github.com/github/codeql-action/actions/runs/278176450

@RasmusWL That should cover all the cases am I right?

RasmusWL · 2020-09-29T11:57:56Z

I've added a modified version of the playground workflow for testing the scripts. https://github.com/github/codeql-action/actions/runs/278176450

@RasmusWL That should cover all the cases am I right?

Nice 👍

Unfortunate about the "expected failures", but not something I will personally worry too much about.

This should allow us to catch blatant error in the python script early, which is super good 🎉

I would propose that we run the different tests on the django-test repo (basically update the workflows that I created for github/codeql-python-team#103 (comment) so they run every day).

Is this still our best approach for something similar for integration tests?

Daverlo · 2020-09-29T13:53:12Z

Is this still our best approach for something similar for integration tests?

Yep, but as @marcogario suggested, by testing the scripts we should be quite covered

I think the typescript wrapper is not too complex. I would expect we can test the python script directly. Ideally these should all complete outputting the env for CODEQL and you can check that. Maybe create a new workflow file for this.

marcogario

With the testing in place, this is good on my side.

We might want to keep the cases in which we know we will fail in the tests, but that is a minor.

marcogario · 2020-09-29T15:00:15Z

.github/workflows/python-deps.yml

+        $GITHUB_WORKSPACE/python-setup/install_tools.sh
+        echo -e '\n\n\n\n\n' && sleep 0.5
+        cd $GITHUB_WORKSPACE/${{ matrix.test_dir }}
+        $GITHUB_WORKSPACE/python-setup/auto_install_packages.py /opt/hostedtoolcache/CodeQL/0.0.0-20200826/x64/codeql/


This will break when we upgrade the bundle version. Can you do a find before to get the version of the bunde against /opt/hostedtoolcache/CodeQL/0.0.0-XXXX/x64/codeql ?

This used to be set as an environment variable, but last time I had to run my scripts it was removed 😞 so that one is probably on me, although I have no idea of how to properly fix it.

What about https://github.com/github/codeql-action/pull/239/files ?

Daverlo force-pushed the daverlo/python branch 2 times, most recently from 7b84fad to 53f29ed Compare September 10, 2020 11:27

Daverlo and others added 9 commits September 11, 2020 10:53

Install python deps on init

8cea215

Setup python extractor for installed deps

52274f1

Add python scripts

5701037

Update python scripts to handle setup.py

7fc41c6

Handle error in poetry 1.0.10

1b20fa7

Merge pull request #173 from github/rasmuswl/update-python-scripts

4ab5cbc

Update Python scripts for "Python deps setup"

Merge branch 'main' into daverlo/python-v2

23a1a65

Check platform before installing python tools

1477345

Fix python string

3495596

Daverlo force-pushed the daverlo/python branch from 53f29ed to 3495596 Compare September 23, 2020 16:19

Daverlo marked this pull request as ready for review September 23, 2020 16:38

marcogario reviewed Sep 24, 2020

View reviewed changes

RasmusWL reviewed Sep 24, 2020

View reviewed changes

RasmusWL and others added 2 commits September 24, 2020 15:36

Apply suggestions from code review

6645c55

Co-authored-by: Marco Gario <marcogario@github.com>

Minor fixes to python installation scripts based on code review

dffce99

Improve warning message

5419fcd

marcogario reviewed Sep 25, 2020

View reviewed changes

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

RasmusWL and others added 4 commits September 25, 2020 12:07

Explain get_extractor_version for python setup scripts

1831270

Add workflow for testing python setup

67ddca1

Add x permission to the test scripts

5e2245c

Remove tests that should fail

4e39b76

marcogario approved these changes Sep 29, 2020

View reviewed changes

Use find in python-des tests

4ce302b

Daverlo force-pushed the daverlo/python branch from f01a3b3 to 4ce302b Compare October 4, 2020 22:04

Merge branch 'main' into daverlo/python

55eb02c

Daverlo merged commit a1fc3a5 into main Oct 5, 2020

Daverlo deleted the daverlo/python branch October 5, 2020 06:34

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

Merge main into v1 #252

Merged

edoardopirovano mentioned this pull request Jul 13, 2021

Install Python tools on self-hosted runners #618

Merged

3 tasks

CharliesAngel1 mentioned this pull request Dec 6, 2023

[Snyk] Upgrade del from 6.1.1 to 7.1.0 CharliesAngel1/codeql-action#5

Open

CharliesAngel1 mentioned this pull request Oct 10, 2024

[Snyk] Security upgrade del from 6.1.1 to 8.0.0 CharliesAngel1/codeql-action#9

Open

Elsokary22 mentioned this pull request Oct 13, 2024

[Snyk] Security upgrade del from 6.1.1 to 8.0.0 Elsokary22/codeql-action#41

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python deps setup #155

Python deps setup #155

Daverlo commented Aug 20, 2020 •

edited

Loading

marcogario left a comment

marcogario Sep 24, 2020

marcogario Sep 24, 2020

marcogario Sep 24, 2020

RasmusWL Sep 24, 2020

marcogario Sep 24, 2020

RasmusWL Sep 24, 2020

marcogario Sep 24, 2020

RasmusWL Sep 24, 2020

marcogario Sep 24, 2020

RasmusWL left a comment

RasmusWL Sep 24, 2020

RasmusWL Sep 24, 2020

RasmusWL Sep 24, 2020

RasmusWL Sep 24, 2020

RasmusWL Sep 24, 2020

RasmusWL Sep 24, 2020

RasmusWL commented Sep 24, 2020

Daverlo commented Sep 25, 2020

marcogario commented Sep 25, 2020

Daverlo commented Sep 29, 2020

RasmusWL commented Sep 29, 2020

Daverlo commented Sep 29, 2020

marcogario left a comment

marcogario Sep 29, 2020

RasmusWL Sep 30, 2020

marcogario Sep 30, 2020



		def install_packages() -> str:
		if os.path.exists('poetry.lock'):

		elif version == 3:
		_check_call(['python3', '-m', 'virtualenv', venv_path])

Python deps setup #155

Python deps setup #155

Conversation

Daverlo commented Aug 20, 2020 • edited Loading

Merge / deployment checklist

marcogario left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RasmusWL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RasmusWL commented Sep 24, 2020

Daverlo commented Sep 25, 2020

marcogario commented Sep 25, 2020

Daverlo commented Sep 29, 2020

RasmusWL commented Sep 29, 2020

Daverlo commented Sep 29, 2020

marcogario left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daverlo commented Aug 20, 2020 •

edited

Loading