Skip to content
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

Fix canonical path vs symlink comparison #731

Merged
merged 1 commit into from
Feb 17, 2025
Merged

Fix canonical path vs symlink comparison #731

merged 1 commit into from
Feb 17, 2025

Conversation

ivanvig
Copy link
Contributor

@ivanvig ivanvig commented Feb 14, 2025

This PR fixes the macOS tests, which were broken by pull request #726 . The issue occurs because on macOS, /var and /tmp are symbolic links to /private/var and /private/tmp. When calling abspath on a relative path or filename, we get the canonical (real) path. However, the TemporaryDirectory() function used in the tests returns a symbolic link that is already an absolute path (starting with /var), so calling abspath on it does nothing.

I also replaced the abspath call when processing the ignored directories with realpath to avoid a mismatch between a symlink and the canonical path of the same directory.

Everything seems to be working now, but I'm still a bit worried that there might be another comparison between symlink paths and real paths elsewhere in the code, since abspath is used in multiple places. A more comprehensive solution might be to replace all abspath calls with realpath, but I didn't want to make such an intrusive change to the codebase.

When calling abspath on a filename, the function returns its canonical
(real) path. In our tests, this value is compared against a symlink,
which causes errors.
@olofk olofk merged commit ea493f5 into olofk:main Feb 17, 2025
12 checks passed
@olofk
Copy link
Owner

olofk commented Feb 17, 2025

Percecto. Gracias por eso!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants