Compare package paths with symlinks resolved - #154
lgoettgens wants to merge 1 commit into
Conversation
GAP 4.17 and newer store root and package directories with symlinks resolved (gap-system/gap#5930), while `PKGMAN_CustomPackageDir` holds the string the user gave. Resolve both sides via `GAP_realpath` before the prefix comparisons in `PKGMAN_UserPackageInfo` and `PKGMAN_RemoveDir`; on GAP before 4.15 the raw strings are compared as before. Assisted-by: Claude Code (Fable 5.1)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #154 +/- ##
==========================================
- Coverage 86.79% 85.20% -1.59%
==========================================
Files 22 22
Lines 1022 1034 +12
==========================================
- Hits 887 881 -6
- Misses 135 153 +18
🚀 New features to boost your workflow:
|
|
This fails CI tests in GAP 4.12 as that doesn't have |
| InstallGlobalFunction(PKGMAN_RealPath, | ||
| function(path) | ||
| local res; | ||
| if IsBound(GAP_realpath) then |
There was a problem hiding this comment.
This line tries to limit this change to versions that have GAP_realpath. Maybe this doesn't work then as yi expect?
mtorpey
left a comment
mtorpey
left a comment
There was a problem hiding this comment.
A couple of comments with things I'm not sure about. The overall question is: when precisely do we need to pass paths through PKGMAN_RealPath and when don't we? Obviously there are hundreds of places we work with file paths in this package, and we only change a few of them here.
You only need to use realpaths if you want to compare them for equality (or some kind of prefix-check). Everything else should be handled by the file system transparently. |
|
Thanks! I think I'm happy with the principle. |
This branch has not been deployed
3 participants
Once gap-system/gap#5930 is merged (expected in GAP 4.17), GAP stores root and package directories with symlinks resolved, while
PKGMAN_CustomPackageDirholds whatever string the user gave.PKGMAN_UserPackageInfoandPKGMAN_RemoveDircompare the two by string prefix, so on macOS, where/varis a symlink to/private/var, packages installed into the user package directory are no longer recognised as such. See gap-system/gap#5930 (comment) for the analysis.Resolve both sides via
GAP_realpath(available since GAP 4.15) before comparing, and fall back to the raw strings on older GAP. Includes a test that reproduces the mismatch; it is excluded below GAP 4.15.This should only be merged if we plan to proceed with gap-system/gap#5930.
🤖 Created with the help of Claude Code (Fable 5.1)