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

Fix python interpreter detection #262

Merged
merged 6 commits into from
May 9, 2022
Merged

Fix python interpreter detection #262

merged 6 commits into from
May 9, 2022

Conversation

sorekz
Copy link
Contributor

@sorekz sorekz commented May 4, 2022

Currently the composite action does not work on self-hosted Windows runners because the version check does not pass.
The version string is printed with an upper case P, at least for python 3.9. Not sure about older versions so I adjusted the check to work with both.

$ python -V
Python 3.9.12

@EnricoMi
Copy link
Owner

EnricoMi commented May 4, 2022

LGTM! Have you tested this with your self-hosted runner?

uses: sorekz/publish-unit-test-result-action/composite@master

Copy link
Owner

@EnricoMi EnricoMi left a comment

Choose a reason for hiding this comment

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

Pending test with self-hosted runner.

@github-actions
Copy link

github-actions bot commented May 4, 2022

Unit Test Results (reference)

       63 files  ±0         63 suites  ±0   3m 49s ⏱️ +49s
     272 tests ±0       272 ✔️ ±0      0 💤 ±0  0 ±0 
17 136 runs  ±0  16 632 ✔️ ±0  504 💤 ±0  0 ±0 

Results for commit cb6d7a2. ± Comparison against base commit 0389cb5.

♻️ This comment has been updated with latest results.

@sorekz
Copy link
Contributor Author

sorekz commented May 5, 2022

LGTM! Have you tested this with your self-hosted runner?

Yes. However there was one additional step. By default the $PATH contains %USERPROFILE%\AppData\Local\Microsoft\WindowsApps which contains a python3.exe that opens the Windows App Store. The which python3 will find that executable instead of using the python.exe. So I needed to remove that from the $PATH.

The best solution would probably be to check for python.exe first on Windows or to always check the python3 -V output to test if that executable is valid.

Edit:
After thinking about it maybe I should rework the commit some more as other users probably will run into the same problem and getting rid of the WindowsApps is not that straight forward. What do you think?

@EnricoMi
Copy link
Owner

EnricoMi commented May 5, 2022

I have added the version check to amend which python3. Please give it a try without the modified $PATH.

@sorekz
Copy link
Contributor Author

sorekz commented May 5, 2022

The check is working however I'm now getting an error when creating the link, which makes me think if I also copied the exe at some point 🤔 because I think that line shouldn't have worked before.
Anyway ... I think the parameters are in the wrong order. It should be ln -s $interpreter $interpreter3 as in ln -s <src> <target>.

Or of course even better when the action is not creating files next to the python.exe because in a company environment where the users eventually have a system python installation but no admin permission they can not write to C:/Programs and that operation will fail.

E.g. create the file in (a subfolder of) $RUNNER_TEMP and add that to the path. The $RUNNER_TEMP is cleaned up after each job.

@EnricoMi
Copy link
Owner

EnricoMi commented May 5, 2022

Very good points, I have fixed the symbolic link and now create it in $RUNNER_TEMP/bin.

Can you test the composite action from your repo?

@sorekz
Copy link
Contributor Author

sorekz commented May 6, 2022

Made some adjustments and used $GITHUB_PATH which prepends the bin/ folder to the PATH.
It works on my runner now.

@EnricoMi
Copy link
Owner

EnricoMi commented May 6, 2022

Yes, GITHUB_PATH is much better! I have modified your commit slightly, and removed the debug output. Can you run a final test?

@sorekz
Copy link
Contributor Author

sorekz commented May 7, 2022

ln: failed to create symbolic link 'C:\actions-runner-289\_work\_temp/bin//c/Users/.../AppData/Local/Programs/Python/Python39/python3': No such file or directory

$interpreter3 already contains the full path so it should be ln -s "$interpreter" "$RUNNER_TEMP/bin/python3".

@EnricoMi
Copy link
Owner

EnricoMi commented May 7, 2022

You are right, reverted that.

@sorekz
Copy link
Contributor Author

sorekz commented May 9, 2022

Works 😃
Tested on my self-hosted (with python) and windows-latest (with python3 pre-installed)

@EnricoMi
Copy link
Owner

EnricoMi commented May 9, 2022

Thanks for testing this through!

@EnricoMi EnricoMi merged commit eacea16 into EnricoMi:master May 9, 2022
@EnricoMi
Copy link
Owner

EnricoMi commented May 9, 2022

This has been released, thanks!

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.

2 participants