fix(utils): improve path handling for Windows compatibility #2887
Conversation
StephenMaturrin
force-pushed
the
fix/utils/windows-path-support
branch
3 times, most recently
from
b1a2c24 to
7052e9d
Compare
There was a problem hiding this comment.
Hi,
Thanks for the contribution.
I will test this PR soon. I currently do not have access to windows.
In the meantime, I have a question.
Do you think it might be better to create a shortcut (.lnk) in windows instead of copying the whole directory when there are no admin privileges.
StephenMaturrin
force-pushed
the
fix/utils/windows-path-support
branch
from
7052e9d to
56dca8b
Compare
Signed-off-by: Miguel Angel SOLINAS <miguelangel.solinas@se.com>
StephenMaturrin
force-pushed
the
fix/utils/windows-path-support
branch
from
56dca8b to
f63b7aa
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes Windows compatibility issues in the path utilities module by adding Windows-specific handling for directory junctions and symbolic links. The changes address problems with path operations on Windows systems, particularly around versioned directory creation and link management.
- Adds Windows junction detection and safe removal utilities
- Implements Windows-specific latest directory creation using junctions
- Updates
create_versioned_dirto use platform-specific link handling
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/anomalib/utils/path.py
Outdated
| subprocess.run( # noqa: S603 | ||
| ["cmd", "/c", "mklink", "/J", str(tmp), str(target.resolve())], # noqa: S607 |
There was a problem hiding this comment.
While the noqa comments acknowledge the security scanner warnings, using subprocess with shell commands can be risky. Consider validating that both tmp and target.resolve() paths don't contain shell metacharacters or use subprocess.run with shell=False (which is already the case here) and ensure paths are properly sanitized.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Ashwin Vaidya <ashwinnitinvaidya@gmail.com>
…g directory junctions on Windows. Yet to be tested.
|
I added some changes to avoid subprocess. But I guess the issue might remain that a user might need admin priv. I will test this on a windows machine later this week. I will mark this as draft till then so we don't merge it. |
|
I tested the changes - it looks like there is no simple way to add shortcut paths in windows without a shell command (if we use pythonic way, we need admin privileges). |
…o and improve fallback mechanism for 'latest' link creation.
|
I tested on windows with these changes - This works. I added some extra checks to make sure the paths are validated before running shell commands. |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| result = subprocess.run( # noqa: S603 | ||
| [ # noqa: S607 | ||
| "cmd", | ||
| "/c", | ||
| "mklink", | ||
| "/J", | ||
| str(tmp), | ||
| str(target.resolve()), | ||
| ], | ||
| capture_output=True, | ||
| text=True, | ||
| check=False, | ||
| ) |
There was a problem hiding this comment.
Using subprocess with shell commands poses security risks. While path validation is performed, consider using the Windows API directly through ctypes or a dedicated library for creating junctions to eliminate subprocess usage entirely.
|
While the current solution works on windows, I would like to have one where we do not make use of shell commands if possible. |
5 participants
📝 Description
Improved path utilities for anomaly detection.
Enhanced versioned directory creation, snake/case conversion, and output filename generation.
🛠️ Fixes compatibility issues when using anomalib on Windows (especially with symbolic links and path handling).
✨ Changes
Select what type of change your PR is: