Port python deps setup to mac #270
Conversation
There was a problem hiding this comment.
A few minor comments, but looks like this went smooth!
It would be good to get a 👍 from @RasmusWL too.
.github/workflows/python-deps.yml
Outdated
| run: | | ||
| set -x | ||
| $GITHUB_WORKSPACE/python-setup/install_tools.sh | ||
| echo -e '\n\n\n\n\n' && sleep 0.5 |
There was a problem hiding this comment.
I might have missed it in the previous review, but is the sleep there necessary?
There was a problem hiding this comment.
Output got garbled without this. I don't remember the situation specifically, but I think output from install_tools.sh got mixed with the output from auto_install_packages.py. I'm to blame for this for sure though, and don't think sleeping for 0.5 seconds is going to be the end of the world 😄
.github/workflows/python-deps.yml
Outdated
| unameOut="$(uname -s)" | ||
| case "${unameOut}" in | ||
| Linux*) basePath="/opt";; | ||
| Darwin*) basePath="/Users/runner";; |
There was a problem hiding this comment.
You should know which runner it is by accessing the matrix variable, no?
.github/workflows/python-deps.yml
Outdated
| unameOut="$(uname -s)" | ||
| case "${true}" in | ||
| Linux*) true="/bin/true";; | ||
| Darwin*) true="/usr/bin/true";; | ||
| esac |
There was a problem hiding this comment.
Same as above.
Can't the condition below this be wrapped in an if ? if [ ! -z $CODEQL_PYTHON ]; then ... ; fi ? Or that is bash specific and on mac you are not sure what shell you get?
Like that you would not need to find the true executable.
| @@ -1,4 +1,4 @@ | |||
| name: Test Python Package Installation on Linux | |||
| name: Test Python Package Installation on Linux and Mac | |||
There was a problem hiding this comment.
Could we move the windows job here too? So that we have all these workflows in one place?
There was a problem hiding this comment.
Windows jobs are completely different, I prefer to have them separated.
There was a problem hiding this comment.
I realize my comment might have been ambiguous. Ok if you prefer to keep it separate, but let me clarify below.
I am suggesting you copy the job from the python-deps-windows.yml workflow file into this workflow file, not that you combine the jobs directly, as indeed they are very different. You would still have one job for both linux and mac, and one job for windows (e.g., test-setup-python-scripts and test-setup-python-scripts-win). However, since they both test the python dependencies, I would have put them in the same file.
There was a problem hiding this comment.
Very cool 👍
# venv is required for installation of poetry or pipenv (I forgot which)
sudo apt-get install -y python3-venv
This part ☝️ is probably available on the base image that is used by the codeql-action, but wasn't available on a fresh Ubuntu 18.04 image that I originally targeted when developing these scripts.
if everything is working without those lines, that's a win for sure 💪
Closes https://github.com/github/code-scanning/issues/2107
This PR ports the auto install python deps functionality so it also works on mac 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/315284024
https://github.com/dsp-testing/daverlo-djanjo-test/security/code-scanning
@RasmusWL I had to remove this from install tools
It seems it is not needed in mac or linux, all the integration tests pass, and it finds the alerts on linux too:
https://github.com/dsp-testing/daverlo-djanjo-test/actions/runs/315301338
https://github.com/dsp-testing/daverlo-djanjo-test/security/code-scanning?query=ref%3Arefs%2Fheads%2Flinux
Merge / deployment checklist