Fix bug with traversing extracted packages when cleaning tarballs - #4353
Fix bug with traversing extracted packages when cleaning tarballs#4353mikemhenry wants to merge 7 commits into
Conversation
|
Hmm, linux-64 osx-64 |
|
It looks like those same CI errors pop up in other PRs and don't look related to the work done in this one -- are these failures known? |
|
Ah looks like this will fix that issue conda-forge/micromamba-feedstock#304 |
jjerphan
left a comment
There was a problem hiding this comment.
Thank you for your contribution.
I have a few comments, and we need to elucidate the change of symlink on other platforms.
| # Creating symlinks may require elevated privileges on Windows, so the | ||
| # regular payload file above provides cross-platform coverage. | ||
| dangling_symlink = None | ||
| if os.name != "nt": |
There was a problem hiding this comment.
Nit: there must be some test utilities defined for detecting Windows.
There was a problem hiding this comment.
I couldn't find anything, but now I'm using the same method from other tests: https://github.com/mamba-org/mamba/blob/main/micromamba/tests/helpers.py#L66
| continue; | ||
| } | ||
| if (!p.is_directory() | ||
| if (p.is_regular_file() |
There was a problem hiding this comment.
On Unix this means archive-named symlinks elsewhere in the package cache tree would no longer be removed unless they resolve as regular files. I think this can be a problem.
There was a problem hiding this comment.
Good point, I’ll restore the !is_directory() condition
45bd819 to
a0f9d5d
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4353 +/- ##
==========================================
- Coverage 55.41% 55.40% -0.01%
==========================================
Files 243 243
Lines 30382 30383 +1
Branches 3256 3256
==========================================
Hits 16835 16835
- Misses 13544 13545 +1
Partials 3 3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Wooo, nice to see CI this green 😎 |
|
I think is ready for another review :) |
Description
Fixes this bug reported here: conda-forge/ambertools-feedstock#193
tl;dr when cleaning up tarballs, the code descends into extracted packages and gets confused when it finds things that look like tarballs or .conda packages. This PR adds some safeguards + a test for this bug.
Type of Change
Checklist
pre-commit run --alllocally in the source folder and confirmed that there are no linter errors.