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

Support running inside windows self-hosted runner #43

Merged
merged 2 commits into from
Oct 30, 2023

Conversation

AngelMF
Copy link
Contributor

@AngelMF AngelMF commented Oct 26, 2023

Add couple lines in action.py, action.yml and venv.bash to support running inside windows self-hosted runner.

action.py Outdated
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 had encoding issues with emojis, that is why i added that new line for the emojis. I took as reference this post https://stackoverflow.com/questions/3597480/how-to-make-python-3-print-utf8

Copy link
Member

Choose a reason for hiding this comment

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

Rather than fiddling with sys.stdout.buffer directly, could you try using sys.stdout.reconfigure? For example, calling this once somewhere early in the program should be sufficient:

sys.stdout.reconfigure(encoding='utf-8')

...and then we can continue using print as normal everywhere else 🙂

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 have tested a couple of things to avoid the encoding issue but have no luck. I have just tested what you mentioned but got the error again. "UnicodeEncodeError: 'charmap' codec can't encode character '\u274c' in position 0: character maps to <undefined>". Maybe i need to think about something else to try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After, testing a little bit more i realized that i was missing encoding="utf-8" where GITHUB_STEP_SUMMARY and GITHUB_OUTPUT were opened. Hopefully, it won't break anything. I updated the code.

action.yml Outdated
Comment on lines 74 to 78
if [[ "${{runner.os}}" == "Windows" ]]; then
python "${{ github.action_path }}/action.py" "${{ inputs.inputs }}"
else
${{ github.action_path }}/action.py "${{ inputs.inputs }}"
fi
Copy link
Member

Choose a reason for hiding this comment

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

This Windows form should work well in both cases, so maybe skip the conditional entirely?

Suggested change
if [[ "${{runner.os}}" == "Windows" ]]; then
python "${{ github.action_path }}/action.py" "${{ inputs.inputs }}"
else
${{ github.action_path }}/action.py "${{ inputs.inputs }}"
fi
python "${{ github.action_path }}/action.py" "${{ inputs.inputs }}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh that would be fine then. I just thought about not changing the existing lines to not break anything since i don't have a Linux environment to test.

Copy link
Contributor Author

@AngelMF AngelMF Oct 27, 2023

Choose a reason for hiding this comment

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

I updated the code and left the "python" line.

setup/venv.bash Outdated
@@ -17,7 +17,12 @@ fi
# `python -m pip install ...` invocation might happen to choose.
if [[ -n "${GHA_PIP_AUDIT_VIRTUAL_ENVIRONMENT}" ]] ; then
if [[ -d "${GHA_PIP_AUDIT_VIRTUAL_ENVIRONMENT}" ]]; then
source "${GHA_PIP_AUDIT_VIRTUAL_ENVIRONMENT}/bin/activate"
if [[ "$OSTYPE" == "msys" || "$(uname)" == MSYS_NT* || "$(uname)" == MINGW* ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Where is OSTYPE coming from?

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 think it is coming from Bash. If you think it could break the Linux code i can remove it. Anyways i put two more validations to identify if OS is Windows. It will be ok if i have to remove it.

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 updated the code and removed $OSTYPE.

@woodruffw
Copy link
Member

Thanks for opening this, @AngelMF! This is a good initial start; I've left a few comments.

@woodruffw woodruffw added the enhancement New feature or request label Oct 27, 2023
@AngelMF
Copy link
Contributor Author

AngelMF commented Oct 30, 2023

Thanks for the comments @woodruffw. I have added a couple changes.

Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @AngelMF!

@woodruffw woodruffw merged commit 530374b into pypa:main Oct 30, 2023
7 checks passed
@woodruffw
Copy link
Member

Merged, thanks again. I'll look into configuring a Windows CI job for this as well, and then do a point release after that.

@AngelMF
Copy link
Contributor Author

AngelMF commented Oct 30, 2023

thanks @woodruffw

@hyrodium
Copy link

hyrodium commented Aug 8, 2024

Hi, thank you for creating this PR and maintaining this repository!
Is there any progress to release the next version with Windows support?

@woodruffw
Copy link
Member

Hi @hyrodium, thanks for the ping on this. I completely forgot to make a release after this was merged, but I'll look into making one now.

@woodruffw
Copy link
Member

This has been released with v1.1.0!

@hyrodium
Copy link

Thank you very much! The new version v1.1.0 worked fine on my project!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants