No more symlinks on Windows

[rcsilva83]

Replace file symlink by shims and dir symlinks on Windows by junctions

closes #1397

[rcsilva83]

Hi,

I'm trying to fix the tests but I couldn't make them load on VS Code's Testing panel. I'm not a Rust developer but I'm searching on the Internet and installed all VS Code's extensions for Rust testing and I couldn't make it work. Can someone help me?

[charlespierce]

Thanks for tackling this and many apologies on the delayed response! Left some comments / suggestions on the implementation. At a higher level, I'm a little wary of the overhead from this approach in general - it's an extra indirection of calling a child command which isn't free. This is the hottest path for Volta and one of the design goals of the project is to have as low startup as possible, so that users ideally can't tell that Volta is intercepting & redirecting their commands.

For Unix especially, we should likely continue to take advantage of symlinks, as there aren't the same set of security / usability concerns on those platforms.

Finally, for the failing tests, it appears that there are a couple of files in the tests directory that import the symlink_file method, which no longer exists. This is causing a compile failure when trying to compile the tests.

[rcsilva83]

@charlespierce is there anything else I need to do?

[Xstoudi]

Any update? Could be awesome to merge it is ready to allow company-constrained environment to run Volta.

[charlespierce]

At a high-level, do you have any details about the runtime cost of this on Windows? Adding an extra indirection should be relatively quick, but it might not be negligible. What I've done in the past to test this is to use a tool like hyperfine to do a bunch of runs calling node -v (which necessarily goes through the shim) to get a sense for the overhead that Volta imposes. We don't need super significant results, just a sense of how the startup time is impacted.

[Xstoudi]

Running hyperfine, this is the result on the current Volta official release on my Windows 10 virtual machine:

Benchmark 1: node -v
  Time (mean ± σ):      47.7 ms ±  10.7 ms    [User: 13.1 ms, System: 25.8 ms]
  Range (min … max):    40.0 ms …  93.9 ms    57 runs

And this is the result on the release exec compiled from this pull request, again on my Windows 10 virtual machine:

Benchmark 1: node -v
  Time (mean ± σ):      46.7 ms ±   3.6 ms    [User: 12.8 ms, System: 29.7 ms]
  Range (min … max):    42.8 ms …  65.0 ms    55 runs

I'm not really sure how to interpret this and it's the first time I ever use hyperfine.

[charlespierce]

One remaining issue with unilaterally changing the behavior for all platforms when the change is intended to impact Windows.

Separately, reviewing this and continuing to think things through, I wonder if we could leverage volta run for all of the Windows use-cases? It is already set up to work similarly to what we're building in this change: Reading the command from the later arguments rather than the name of the executable. We're also already building a helper script as part of setting up Windows shims. I suspect we could make the "shims" for Windows be .cmd files with a short script that calls volta run {arguments} directly, which would then have the same effect as this change while requiring less overall churn.

I'm going to rough up that approach, which combined with the change here to use junctions rather than directory symlinks may achieve the same goals.

[charlespierce]

I'm still a little unsure about making this the behavior for all platforms. It's relatively minor, but it is another indirection and since Rust std::process::Command doesn't use exec, it may cause unexpected issues on top of any small performance hit.

One that comes to mind is signals (e.g. pressing Ctrl-C in the terminal while a process is running). We don't handle them perfectly right now, but I know a lot of work went into making sure that Ctrl-C would cancel the underlying Node process (as a user would expect), not only the wrapper process.

[rcsilva83]

Hi Charles!

Thank you for the feedback.

So you prefer this change is changed only for Windows, right? We can make this condition, no problem.

About the volta run and .cmd idea, let me know if you prefer that approach and I'll try to implement it.

[charlespierce]

Yeah, if we move forward here, we should limit the scope to only for Windows, so that we have the smallest possible blast radius.

For the volta run idea, I want to do a quick prototype to see if it's feasible, then we can look at which approach makes the most sense.

And thank you again for persisting with this change, despite our busy schedules making the lead times really long! It will be a big improvement in Windows behavior when we don't need the "Developer Mode" warning 💥

[rcsilva83]

Great! So I'll wait for you before making both changes.