Fix auto-patch script

[hypevhs]

• Only run patch_bin on directories
• Wait 5s for spawn-helper to appear before attempting patch
• Detect already-patched binaries correctly
• Fix shellcheck warning by using less-ambiguous syntax
• Increase logging

Fixes #45

[msteen]

@libjared Thanks for your feedback and PR, much appreciated! The fact that I made such a typo is already an indicator I should have refactored that check, so that is definitely going in.

I don't like the fact we are back to sleeps though, and what if spawn-helper exists, but rg (the binary from ripgrep) doesn't? Also, that dir check should not be necessary right? If you update find with -type d like you did, $bin_dir will always be a directory in both call sites.

I think I have a decent workaround though. So I am currently putting in your changes together with that fix.

[hypevhs]

The sleeps are only for a maximum of 5 seconds. inotifywaiting here didn't seem like the right fit because the node_modules aren't all added to the directory at the same time, they just sorta filter in slowly, and I worry that using inotifywait then immediately starting the patching process would be jumping the gun. I wish vscode left better hints as to what state the installation is in.

Now that I think about it, the best thing to do would be to, after patching node, run inotifywait in monitor mode for around 10s and modify each binary that gets added one by one.

You're right that the [[ ! -d check isn't necessary currently. I'm on the fence, but I'm leaning towards keeping it as "defensive programming" because of how different both call sites do filtering for directories.

[Atry]

Why not use inotifywait in recursive mode? https://linux.die.net/man/1/inotifywait#:~:text=rather%20than%20stderr.-,%2Dr%2C%20%2D%2Drecursive,-Watch%20all%20subdirectories

[msteen]

@Atry Recursive mode on the scale of node_modules is extremely expensive and might even run into kernel resource problems on some configurations. Another problem with it, is that there is no point to continue watching except for the root directory like I am already doing, and I would not know the stop condition.

@libjared My workaround in my sleep branch is to replace node with a script, since node is run only when all files have been unpacked.

I would normally agree with defensive programming, but on a program (well script) of this size, and both -type d and ISDIR leaving no chance, less is more.

Currently I am looking into getting the RPATH for the node supplied by Microsoft without using a NixOS version of Node.js. This way you can just patch everything and not replace anything, but it is a binary of 77M so I will leave it as an option.

[hypevhs]

@msteen

1. What's the benefit of elfpatching the MSFT-provided node vs replacing it with a NixOS-provided node? elfpatch seems less guaranteed to work, in comparison to using the node from nixpkgs, something we already know works on NixOS.
2. I see we switched to naming the trigger server.sh. Is that better than naming it node?

Anyway I like the idea of replacing node or server.sh with a wrapper script and it seems to have been implemented quickly and very well. I'll close or rebase this PR after testing if it solves all the issues.

[msteen]

Closed in favor of #47.