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: canonicalize the path of TemporaryFs #5227

Merged
merged 1 commit into from
Mar 4, 2025

Conversation

siketyan
Copy link
Member

@siketyan siketyan commented Mar 1, 2025

Summary

Tests added in #5200 are failing on macOS. I found that the directory path of the TemporaryFs used in tests is not canonicalized, which leads path inconsitency. On macOS, temp_dir() returns a path under /var by default, which is a symlink to /private/var. oxc_resolver follows symlinks to resolve a module, so the resolved path will be under /private/var.

This pull request adds canonicalization to TemporaryFs::new to avoid this inconsitency.

Test Plan

Unfortunately, this PR will NOT make the failing tests green. At least the current failing position will be passed, but there is another problem failing the cases. 😢

@siketyan siketyan requested review from arendjr and a team March 1, 2025 14:24
@siketyan siketyan self-assigned this Mar 1, 2025
@github-actions github-actions bot added A-CLI Area: CLI A-Core Area: core labels Mar 1, 2025
@siketyan siketyan force-pushed the fix/temporary-fs-symlink branch from e4cdf9d to 5719e8f Compare March 1, 2025 14:41
@siketyan siketyan marked this pull request as draft March 1, 2025 15:52
@siketyan siketyan force-pushed the fix/temporary-fs-symlink branch from 5719e8f to 9a95d19 Compare March 2, 2025 06:27

// On macOS, the temporary directory is in `/var`, which is a symlink to `/private/var`.
// We need to get the actual path here, or we will get path inconsistency.
#[cfg(target_os = "macos")]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to do std::fs::canonicalize for all platforms here, but it will change the path to \\?\C:\... on Windows, which is not properly handled in this project. 😢

@siketyan siketyan marked this pull request as ready for review March 2, 2025 06:32
@arendjr
Copy link
Contributor

arendjr commented Mar 2, 2025

I wonder if we should make the watcher smart enough that any sources outside a project will be automatically watched in destinations pointed to by symlinks. It might be a more cross-platform approach that could help users too.

Not sure yet how complex that would get…

@siketyan
Copy link
Member Author

siketyan commented Mar 3, 2025

@arendjr The problem here is that the workspace is opened in a symlinked path. The diagnostics should appear before the watcher detects a change, but it's not appeared now as oxc_resolver resolves the module to an actual path (symlink target). Workspace watcher should also follow symlinks, but it's not yet a problem in this case.

@arendjr
Copy link
Contributor

arendjr commented Mar 4, 2025

Workspace watcher should also follow symlinks, but it's not yet a problem in this case.

I think this requires some delicate interplay between the scanner and its watcher. When the watcher receives events, it would only be for the destination paths, so it cannot traverse backwards to see which symlinks pointed there. But the scanner should be able to catch when it discovers symlinks and tell the watcher to watch those destinations if they are outside the project folder. At least, that's how I think it should work.

For now I'll merge this, because it's a step towards getting the macOS tests green. I'll try to get the remainder of the tests green as well. I've created #5271 for improving the symlink handling.

@arendjr arendjr merged commit 6a36760 into biomejs:main Mar 4, 2025
11 checks passed
@arendjr
Copy link
Contributor

arendjr commented Mar 4, 2025

Huh, I just pulled after merging, and it did make the tests green locally. Watching the CI now 👀

@arendjr
Copy link
Contributor

arendjr commented Mar 4, 2025

Oh, I see. They're sometimes green, but indeed unreliable now.

@siketyan siketyan deleted the fix/temporary-fs-symlink branch March 4, 2025 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-Core Area: core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants