fix: Re-enable updating of symlinks to outside subproject #2427
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2427 +/- ##
==========================================
- Coverage 97.26% 96.89% -0.37%
==========================================
Files 55 55
Lines 6288 6348 +60
==========================================
+ Hits 6116 6151 +35
- Misses 172 197 +25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I see the windows tests are failing, but I wasn't able to work out how to reproduce the test suite results with just the new test failing (as here in CI), I always had other tests failing too, which meant I didn't feel confident to start debugging the test as I might have other system issues. I tried running in git bash, started as admin, which got me as far as only 4 tests failing. If we could either add some notes to the contributing docs about how to run the tests on windows more easily (or discuss them here, and I can raise a PR to document it once it's clear) I'm happy to continue working on this. Or if it's obvious to a maintainer with a working Windows setup how to fix it, happy for it just to be adjusted. The CONTRIBUTING.md recommends using gitpod. I'm not clear if that would have supported windows, but I didn't explore that much because it was requesting github permissions to my private repos, which I don't want to give just to use it for a public repo |
sisp
left a comment
There was a problem hiding this comment.
Thanks for discovering this regression and submitting a fix, @cbrown1234! 🙇 I've left two inline comments.
| tmpl / "a_symlink.txt": other / "outside.txt", | ||
| tmpl / "symlink_dir": other, |
There was a problem hiding this comment.
Just an idea why the test might be failing on Windows (I'm on mobile right now, can't test it): Perhaps an absolute path to the symlink target doesn't work. Looking at other tests in this file, the targets are relative paths.
| path_to_check = dst_root.joinpath(dst_relpath) | ||
| if path_to_check.is_symlink(): | ||
| # If destination path is a symlink, it can safely point outside subproject dir, | ||
| # so long as nothing is templated into it (which would be caught by its own check), | ||
| # while still itself existing within the subproject. | ||
| # Therefore check parent is within subproject to avoid resolving the symlink itself | ||
| # Note: Typically occurs when updating preserve_symlinks = true templates | ||
| path_to_check = path_to_check.parent | ||
| if not path_to_check.resolve().is_relative_to(dst_root): |
There was a problem hiding this comment.
I think the fix is correct, but I needed a moment to consider the implications of checking the parent directory of a symlink to avoid following the symlink via <symlink>.resolve(). I checked the pathlib docs, and it seems there really is no alternative to Path.resolve() that only normalizes a path without resolving a symlink. But all we really need is a purely lexical normalization of the symlink file path to normalize path segments like ..; os.path.normpath offers exactly this. What do you think about simplifying the check by applying os.path.normpath on the full path and removing the parent directory "hack"?
There was a problem hiding this comment.
Yeah, I did review the pathlib docs too, and also couldn't find a good alternative, unfortunately.
I had assumed that the behaviour of .resolve() in resolving any intermediate symlinks on the way to the target file was desired/intentional, and it wasn't just the path normalisation that was needed. e.g. to check an intermediate symlink in the path didn't redirect you outside the destination directory. We'd lose this behaviour with os.path.normpath
The check of the parent seems sound as dst_root.is_relative_to(dst_root) == True and the resolved parent dir + filename has to be the same file to my understanding
We could even potentially simply the logic and check the resolved parent for every case. It shouldn't matter if the file/dir is a symlink or not
Fix for #2426