Automattic / studio

Studio by WordPress.com, a free desktop app that helps developers streamline their local WordPress development workflow.
https://developer.wordpress.com/studio/
GNU General Public License v2.0
193 stars 18 forks source link

Enable support for symlinks #510

Closed jeroenpf closed 1 month ago

jeroenpf commented 2 months ago

Related to https://github.com/Automattic/dotcom-forge/issues/8817

Proposed Changes

This PR aims to ensure that symlinks found inside of a site directory are working. We do this by first scanning and later watching the file system for symlinks and then creating mounts for the targets of each found symlink.

Since symlink targets inside of the NODEFS are not always predictable, we need to get the actual target of the link as seen by the FS inside the PHP runtime. When we determine the correct target, we create a mount point that points to the realpath of the symlink on the host system, essentially bypassing the need for chained links. (e.g.link -> link 2 -> target )

We also need to ensure that we keep a reference count of how many links point to a specific mounted target. It is possible that multiple symlinks point to the same target. As we are trying to unmount unnecessary targets, we need to be sure no other links point to it when we attempt to delete it.

Additionally, we need to ensure that we do not delete pre-existing files or directories at the target path. E.g. if a target already existed before we found a link pointing to it, we never want to remove it if the symlinks are removed.

Potential issue: I found that deleting a symlink correctly removes files and links from the NODESF but when trying to load it via the browser, it throws an error of file not found. I think this might be related to how PHP or the underlying server is caching things or perhaps something related to the NODEFS. We might need to investigate that at some point.

Testing Instructions

Extra scenarios to test:

Pre-merge Checklist

jeroenpf commented 1 month ago

@fluiddot I've done some refactoring of the symlink manager class and moved it to the lib directory. It did not make much sense to me to keep making changes in the vendor folder.

I've also solved the issue with the watcher not being stopped properly and also return a promise from startWatching which makes more sense to me. Additionally, the AbortController triggers an error that we should handle separately as an 'expected' error. This is also documented in the nodejs reference documentation.

jeroenpf commented 1 month ago

On macOS the PR is working great, however, symlinks are not working on Windows. Here's the results I'm getting:

How to create symlink

  1. Open Powershell session with admin privileges.
  2. Go to <SITE_FOLDER>/wp-content/plugins.
  3. Run the command New-Item -Path new-plugin.php -ItemType SymbolicLink -Value hello.php.

Results

  • When adding a symlink, this statement always return false. I investigated further and this is a problem with the path separator used. When using path.join, the package picks up the one based on the OS, in this case, Windows. However, the PHP environment is Unix-based, so Windows paths won't work.
  • I forced using Unix separate when generating vfsPath, but the path returned when reading the symlink (ref) is returning a wrong value. E.g. vfsTarget = '/var/www/html/wp-content/plugins/C:\\Users\\Carlos\\Sstudio\\my-site\\wp-content\\plugins\\hello.php'.

@jeroenpf I'm open to tackle this in a separate PR, in case we'd like to only provide support for symlinks in macOS. WDYT?

Good find! I will address it in this PR. We should be able to use path.posix.join to force posix compliant paths when dealing with paths inside WASM. I will try this out.

jeroenpf commented 1 month ago

I've added some tests and fixed some more small items based on feedback.

fluiddot commented 1 month ago

@jeroenpf not a blocker but it would be great if we could solve the file conflicts with trunk.