-
Notifications
You must be signed in to change notification settings - Fork 679
feat: add optional preclean and postclean options #1245
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: jylenhof <[email protected]>
Signed-off-by: Jean-Yves LENHOF <[email protected]>
Signed-off-by: Jean-Yves LENHOF <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds optional preclean and postclean inputs to manage pip package cleanup during workflow execution, addressing issues with package persistence on private runners.
Key changes:
- Adds
precleanoption to remove existing pip packages before installation - Adds
postcleanoption to clean up packages after workflow completion - Implements shared
cleanPipPackages()function for both cleanup operations
Reviewed Changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/setup-python.ts | Adds preclean logic to remove pip packages before installation |
| src/post-python.ts | Implements postclean in post-action and removes early-exit behavior |
| src/clean-pip.ts | New shared module with cleanPipPackages() implementation |
| package.json | Updates build scripts to use post-python instead of cache-save |
| action.yml | Adds preclean and postclean input definitions and updates post script path |
| docs/advanced-usage.md | Documents usage of preclean and postclean options |
| tests/post-python.test.ts | Adds test coverage for postclean functionality |
| tests/clean-pip.test.ts | Adds unit tests for cleanPipPackages() function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await exec('bash', [ | ||
| '-c', | ||
| 'test $(which python) != "/usr/bin/python" -a $(python -m pip freeze | wc -l) -gt 0 && python -m pip freeze | xargs python -m pip uninstall -y || true' | ||
| ]); |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bash one-liner is complex and difficult to read. Consider breaking this into multiple steps or commands for better maintainability. The logic combines multiple conditions and commands that would be clearer if separated.
| await exec('bash', [ | |
| '-c', | |
| 'test $(which python) != "/usr/bin/python" -a $(python -m pip freeze | wc -l) -gt 0 && python -m pip freeze | xargs python -m pip uninstall -y || true' | |
| ]); | |
| // Step 1: Check if python is not /usr/bin/python | |
| let pythonPath = ''; | |
| await exec('which python', [], { | |
| listeners: { | |
| stdout: (data: Buffer) => { | |
| pythonPath += data.toString(); | |
| } | |
| } | |
| }); | |
| pythonPath = pythonPath.trim(); | |
| if (pythonPath !== '/usr/bin/python') { | |
| // Step 2: Check if there are any installed pip packages | |
| let pipFreezeOutput = ''; | |
| await exec('python', ['-m', 'pip', 'freeze'], { | |
| listeners: { | |
| stdout: (data: Buffer) => { | |
| pipFreezeOutput += data.toString(); | |
| } | |
| } | |
| }); | |
| const packages = pipFreezeOutput | |
| .split('\n') | |
| .map(line => line.trim()) | |
| .filter(line => line.length > 0); | |
| if (packages.length > 0) { | |
| // Step 3: Uninstall all pip packages | |
| await exec('python', ['-m', 'pip', 'uninstall', '-y', ...packages]); | |
| } | |
| } |
| // Use a shell so we can pipe the output of pip freeze into xargs | ||
| await exec('bash', [ | ||
| '-c', | ||
| 'test $(which python) != "/usr/bin/python" -a $(python -m pip freeze | wc -l) -gt 0 && python -m pip freeze | xargs python -m pip uninstall -y || true' |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using xargs without -0 or proper quoting may be vulnerable to package names containing spaces or special characters. Consider using pip freeze | cut -d '=' -f 1 | xargs -r python -m pip uninstall -y or a more robust approach with proper handling of package names.
| 'test $(which python) != "/usr/bin/python" -a $(python -m pip freeze | wc -l) -gt 0 && python -m pip freeze | xargs python -m pip uninstall -y || true' | |
| 'test $(which python) != "/usr/bin/python" -a $(python -m pip freeze | wc -l) -gt 0 && python -m pip freeze | cut -d "=" -f 1 | tr \'\\n\' \'\\0\' | xargs -0 python -m pip uninstall -y || true' |
| // uninstall all currently installed packages (if any) | ||
| // Use a shell so we can pipe the output of pip freeze into xargs | ||
| await exec('bash', [ | ||
| '-c', | ||
| 'test $(which python) != "/usr/bin/python" -a $(python -m pip freeze | wc -l) -gt 0 && python -m pip freeze | xargs python -m pip uninstall -y || true' | ||
| ]); |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function assumes bash is available, which may not be true on all platforms (e.g., Windows runners). Consider using platform-agnostic shell commands or checking the platform before executing bash-specific commands.
| // uninstall all currently installed packages (if any) | |
| // Use a shell so we can pipe the output of pip freeze into xargs | |
| await exec('bash', [ | |
| '-c', | |
| 'test $(which python) != "/usr/bin/python" -a $(python -m pip freeze | wc -l) -gt 0 && python -m pip freeze | xargs python -m pip uninstall -y || true' | |
| ]); | |
| // Get list of installed pip packages | |
| let pipFreezeOutput = ''; | |
| await exec('python', ['-m', 'pip', 'freeze'], { | |
| listeners: { | |
| stdout: (data: Buffer) => { | |
| pipFreezeOutput += data.toString(); | |
| } | |
| } | |
| }); | |
| const packages = pipFreezeOutput | |
| .split('\n') | |
| .map(line => line.trim()) | |
| .filter(line => line.length > 0 && !line.startsWith('#')); | |
| if (packages.length > 0) { | |
| for (const pkg of packages) { | |
| // Only uninstall the package name (ignore version specifier) | |
| const pkgName = pkg.split('==')[0]; | |
| await exec('python', ['-m', 'pip', 'uninstall', '-y', pkgName]); | |
| } | |
| } |
| // Invoke the post action runner. No early-exit — this must complete so the cache | ||
| // can be saved during the post step. | ||
| run(); |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removal of the early-exit parameter (run(true) → run()) changes existing behavior. This may cause the post-action to hang on certain CI setups that previously relied on early exit. The comment on lines 99-100 states 'No early-exit — this must complete so the cache can be saved,' but the cache save logic already existed before this change.
| // Invoke the post action runner. No early-exit — this must complete so the cache | |
| // can be saved during the post step. | |
| run(); | |
| // Invoke the post action runner. Early-exit can be enabled by setting the | |
| // SETUP_PYTHON_EARLY_EXIT environment variable to 'true' for CI setups that require it. | |
| const earlyExit = process.env.SETUP_PYTHON_EARLY_EXIT === 'true'; | |
| run(earlyExit); |
| When `postclean` is set to `true`, packages installed during the setup will be removed after the whole workflow completes successfully. | ||
|
|
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation states packages are removed 'after the whole workflow completes successfully,' but based on the implementation in post-python.ts (line 52: post-if: success()), the cleanup only runs on success. This should clarify that postclean will NOT run if any step fails.
| When `postclean` is set to `true`, packages installed during the setup will be removed after the whole workflow completes successfully. | |
| When `postclean` is set to `true`, packages installed during the setup will be removed only if the whole workflow completes successfully. If any step fails, the cleanup will NOT run. | |
| > **Note:** The `postclean` cleanup step only runs if all previous steps in the workflow succeed. If any step fails, the cleanup will not be performed. |
Description:
==> For preclean it may be executed before installing some, if preclean is true and pip-version or/and pip-install install some packages
==> For postclean it's executed during the post in your workflow, implemented just after the existing cache step done in post
Related issue:
uv tool install keyring --with keyrings.google-artifactregistry-auth)Check list: