nix-community / nixos-vscode-server

Visual Studio Code Server support in NixOS
MIT License
419 stars 76 forks source link

Auto-fix script fails often #45

Closed hypevhs closed 1 year ago

hypevhs commented 1 year ago

Getting these on latest master, cb48580bb58d28e6fbf0f5f032f57727dff3bb9c.

1. Tries to auto-fix tarball mid-download

ln: failed to access '/home/\<SNIP>/.vscode-server/bin/441438abd1ac652551dbe4d408dfcec8a499b8bf-1676144611.tar.gz/node': Not a directory

On a fresh bin dir, it seems to want to try to patch the tarball as it's being downloaded. The service dies 5 times before the download completes, exceeds the restart counter, and so stays broken. We already check if it's a directory in the inotifywait loop, but we should also limit to directories in the initial find run.

2. Tries to auto-fix before node_modules are ready

Because inotifywait only waits for the node trigger to get removed, the node_modules (where spawn-helper lives) may not be populated in time, so it'll find nothing to patch. After replacing node with the symlink, we should wait an additional 5 seconds to let node_modules become ready.

3. Wrong comparison

The script has [[ $interp != "$node_rpath" ]]. If this is meant to check for already-patched binaries, I think this is a typo, and should be [[ $interp != "$node_interp" ]].

4. Shellcheck advises against A && B || C syntax

Shellcheck says this, which may or may not be a problem:

In /nix/store/lzwvk3zxh5j9xfcspbnq0qmgyls4dlk9-auto-fix-vscode-server.sh line 13:
    interp=$(patchelf --print-interpreter "$bin" 2>/dev/null) && [[ $interp != "$node_rpath" ]] || continue
                                                              ^-- SC2015 (info): Note that A && B || C is not if-then-else. C may run when A is true.

For more information:
  https://www.shellcheck.net/wiki/SC2015 -- Note that A && B || C is not if-t...

It's probably justified usage here, but it's confusing, and I think it would be better to use two separate checks here.

I'll make a PR for all of these issues in a few minutes.

hypevhs commented 1 year ago

Issues 2 3 4 have been fixed by #47. Issue 1 only affects Remote-WSL for whatever reason. Remote-WSL has a host of other problems, which we will tackle later, so closing.