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

Python-Setup: run auto_install_packages.py with -B flag #1126

Merged
merged 5 commits into from Jun 30, 2022

Conversation

aibaars
Copy link
Collaborator

@aibaars aibaars commented Jun 27, 2022

This avoids creating a pycache folder in the _actions folder, which
may cause file ownership problems on self-hosted runners
when run in a docker container.

Merge / deployment checklist

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

This avoids creating a __pycache__ folder in the _actions folder, which
may cause file ownership problems on self-hosted runners
when run in a docker container.
@aibaars aibaars requested a review from a team as a code owner June 27, 2022 14:41
@aibaars aibaars marked this pull request as draft June 27, 2022 14:42
@aibaars aibaars marked this pull request as ready for review June 27, 2022 16:53
@aeisenberg
Copy link
Contributor

Is there any performance improvements to creating a pycache folder? We could use the -X pycache_prefix=PATH option instead to place them in the runner temp folder.

@aeisenberg
Copy link
Contributor

Ah...I see you already went over that in the internal issue. That option only exists in 3.8 or later. If there is likely to be noticeable speedup when using the pycache, then we can first detect which python version we are using and only use the option for 3.8 or later.

I suspect that this is not the case, though and what you have is fine.

@RasmusWL
Copy link
Member

out of curiosity, why did you choose to revert the PYTHONDONTWRITEBYTECODE=1 version of this fix?

@aibaars
Copy link
Collaborator Author

aibaars commented Jun 28, 2022

out of curiosity, why did you choose to revert the PYTHONDONTWRITEBYTECODE=1 version of this fix?

I meant to mention that approach in the PR and ask whether it would be preferred or not. The drawback of setting the environment variable is that it affects all python invocations, not just the problematic one running the script in the setup-python action. I doubt it would cause any problems, but who knows.

@aibaars aibaars enabled auto-merge June 29, 2022 14:42
@aibaars aibaars force-pushed the aibaars/python-setup-no-pycache branch from dabaeef to cae9a1f Compare June 30, 2022 07:07
@aibaars aibaars force-pushed the aibaars/python-setup-no-pycache branch from 4cdea43 to 53bc5e6 Compare June 30, 2022 07:10
@aibaars aibaars merged commit ca8a203 into main Jun 30, 2022
@aibaars aibaars deleted the aibaars/python-setup-no-pycache branch June 30, 2022 08:08
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.

None yet

3 participants