fix: invoking code-server in integrated terminal

[code-asher]

Fixes #5335

I opted to point it to our own bin directory instead of Code's. We could probably makes theirs work but ours acts a bit differently so we would need to discuss so this seemed like the lower risk option.

Thanks to @fritterhoff for linking to where this happens, made this very easy to fix!

[codecov[bot]]

Codecov Report

Merging #5360 (8e03cbb) into main (0022473) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #5360   +/-   ##
=======================================
  Coverage   72.42%   72.42%           
=======================================
  Files          30       30           
  Lines        1672     1672           
  Branches      366      366           
=======================================
  Hits         1211     1211           
  Misses        398      398           
  Partials       63       63           

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0022473...8e03cbb. Read the comment docs.

[code-asher]

Decided to move this back to a draft. I am going to look into making their script work because that would be the more maintainable solution.

[benz0li]

I opted to point it to our own bin directory instead of Code's.

Doesn't this simply add the same path twice? I.e.

$ echo $PATH
/opt/code-server/bin:/opt/code-server/bin:...

ℹ️ Because I've already added code-server's bin directory to PATH: https://gitlab.b-data.ch/jupyterlab/r/docker-stack/-/blob/master/base/latest.Dockerfile#L165

[jsjoeio]

Untested, but maybe we could add an e2e test for this too. Something like:

// bin.test.ts

import * as cp from "child_process"
import * as path from "path"
import util from "util"
import { clean, tmpdir } from "../utils/helpers"
import { describe, expect, test } from "./baseFixture"

describe("Bin", true, [], {}, () => {
  const testName = "bin"
  test.beforeAll(async () => {
    await clean(testName)
  })

  test("should give access to code-server in terminal", async ({ codeServerPage }) => {
    const tmpFolderPath = await tmpdir(testName)
    const tmpFile = path.join(tmpFolderPath, "pipe")

    const command = `mkfifo '${tmpFile}' && cat '${tmpFile}'`
    const exec = util.promisify(cp.exec)
    const output = exec(command, { encoding: "utf8" })

    // Open terminal and type in value
    await codeServerPage.focusTerminal()

    await codeServerPage.page.waitForLoadState("load")
    await codeServerPage.page.keyboard.type(`code-server --help`)
    await codeServerPage.page.keyboard.press("Enter")
    // It may take a second to process
    await codeServerPage.page.waitForTimeout(1000)

    const { stdout } = await output
    expect(stdout).toMatch(/usage: code-server/)
  })
})

[code-asher]

Doesn't this simply add the same path twice?

Yup, for anyone that has already added it to the path it will appear twice. We could add a check to prevent it from being added if it already happens to exist but I think it would be more appropriate to send that upstream than patch it here since it affects upstream as well. (Plus there are no ill effects from having it twice, I think?)

[code-asher]

@jsjoeio that is awesome, I actually intended on adding a test for this so that code will be very helpful!

[benz0li]

@code-asher What about https://github.com/coder/code-server/issues/5335#issuecomment-1191075397?

[code-asher]

I think the ideal solution is going to be to fix upstream's code-server script so that it can find Node. This means invocations on the integrated terminal will use their script instead of ours but long-term that is probably a good move. For example things like --wait will just work. Thoughts?

[fritterhoff]

I think the ideal solution is going to be to fix upstream's code-server script so that it can find Node. This means invocations on the integrated terminal will use their script instead of ours but long-term that is probably a good move. For example things like --wait will just work. Thoughts?

From my point of view fixing/patching the script would make more sense and should be better maintanable than patching the actual source code of vs code?

[benz0li]

IMHO there should be no file /opt/code-server/lib/vscode/bin/remote-cli/code-server. Reposting https://github.com/coder/code-server/issues/5335#issuecomment-1191075397 here:

@code-asher In code-server v4.4.0/v4.3.0 there is /opt/code-server/lib/vscode/bin/remote-cli/code-oss.
👉 /opt/code-server/lib/vscode/bin/remote-cli is prepended to PATH, too; but there is code-oss instead of code-server at the said path.

---

My workaround is renaming /opt/code-server/lib/vscode/bin/remote-cli/code-server to /opt/code-server/lib/vscode/bin/remote-cli/code-oss and replacing all occurrences of code-server with code-oss in

1. /opt/code-server/lib/vscode/bin/remote-cli/code-oss
2. /opt/code-server/lib/vscode/bin/helpers/browser.sh

---

code-server v4.4.0:

$ tree /opt/code-server/lib/vscode/bin
/opt/code-server/lib/vscode/bin
├── code-server-oss
├── helpers
│   └── browser.sh
└── remote-cli
    └── code-oss

2 directories, 3 files

$ cat /opt/code-server/lib/vscode/bin/code-server-oss 
#!/usr/bin/env sh
#
# Copyright (c) Microsoft Corporation. All rights reserved.
#

case "$1" in
        --inspect*) INSPECT="$1"; shift;;
esac

ROOT="$(dirname "$(dirname "$(readlink -f "$0")")")"

"$ROOT/node" ${INSPECT:-} "$ROOT/out/server-main.js" "$@"

$ cat /opt/code-server/lib/vscode/bin/helpers/browser.sh 
#!/usr/bin/env sh
#
# Copyright (c) Microsoft Corporation. All rights reserved.
#
ROOT=$(dirname "$(dirname "$(dirname "$0")")")

APP_NAME="code-oss"
VERSION="1.66.2"
COMMIT=","
EXEC_NAME="code-oss"
CLI_SCRIPT="$ROOT/out/server-cli.js"
"$ROOT/node" "$CLI_SCRIPT" "$APP_NAME" "$VERSION" "$COMMIT" "$EXEC_NAME" "--openExternal" "$@"

$ cat /opt/code-server/lib/vscode/bin/remote-cli/code-oss 
#!/usr/bin/env sh
#
# Copyright (c) Microsoft Corporation. All rights reserved.
#
ROOT=$(dirname "$(dirname "$(dirname "$0")")")

APP_NAME="code-oss"
VERSION="1.66.2"
COMMIT=","
EXEC_NAME="code-oss"
CLI_SCRIPT="$ROOT/out/server-cli.js"
"$ROOT/node" "$CLI_SCRIPT" "$APP_NAME" "$VERSION" "$COMMIT" "$EXEC_NAME" "$@"

---

Does /opt/code-server/lib/vscode/bin/* come into play with code-server at all?

And yes, "$ROOT/node" does not work; but "$ROOT/../../node" would.

[code-asher]

Yeah renaming the upstream script would cause code-server invocations to use our script instead (for users that have it on their path) but since upstream's script does everything ours does and more it might make sense to just use theirs.

The only thing theirs does not support is launching a new instance of code-server (it only supports interacting with the current instance of code-server) but I have no idea if that is a common use in the integrated terminal.

[benz0li]

Yeah renaming the upstream script would cause code-server invocations to use our script instead (for users that have it on their path) but since upstream's script does everything ours does and more it might make sense to just use theirs.

/opt/code-server/lib/vscode/bin/code-server-oss is actually meant to start a new server.

The only thing theirs does not support is launching a new instance of code-server (it only supports interacting with the current instance of code-server) but I have no idea if that is a common use in the integrated terminal.

I have never started a new instance of code-server using the integrated terminal.

[benz0li]

/opt/code-server/bin/code-server is the equivalent to /opt/code-server/lib/vscode/bin/code-server-oss and not /opt/code-server/lib/vscode/bin/remote-cli/code-oss.

The latter named /opt/code-server/lib/vscode/bin/remote-cli/code-server in code-server, creating a name conflict.

[benz0li]

Also reposting https://github.com/coder/code-server/issues/5335#issuecomment-1189936620 here:

The way code-server is released(/run?), it behaves like a remote installation. This ~might be~ is the reason, why /opt/code-server/lib/vscode/bin/remote-cli is prepended to PATH.

This will also generate a settings conflict should you ever consider resolving https://github.com/coder/code-server/issues/1315.
ℹ️ In VS Code it is not possible to start another remote connection from within a remote connection.

[code-asher]

/opt/code-server/bin/code-server is the equivalent to /opt/code-server/lib/vscode/bin/code-server-oss and not /opt/code-server/lib/vscode/bin/remote-cli/code-oss.

./bin/code-server is like ./lib/vscode/bin/code-server-oss when ran from an external terminal (spawns a server) but it is like ./lib/vscode/bin/remote-cli/code-oss when ran in an integrated terminal (interacts with the existing code-server). code-server switches behavior based on the VSCODE_IPC_HOOK_CLI environment variable which is set in the integrated terminal.

For example here is a test with ./lib/vscode/bin/remote-cli/code-oss:

$ code-server ~/test
/var/tmp/coder/code-server/lib/vscode/bin/remote-cli/code-server: 12: /var/tmp/coder/code-server/lib/vscode/node: not found
$ sed -i 's/ROOT\/node/ROOT\/..\/..\/node/' /var/tmp/coder/code-server/lib/vscode/bin/remote-cli/code-server
$ code-server ~/test

This opened the file ~/test in my current instance of code-server, which is what ./bin/code-server would have done and is what I think we want the code-server invocation to always do in the integrated terminal.

[code-asher]

Sorry for being dense; how does this change anything with https://github.com/coder/code-server/issues/1315? Our script and upstream's are more or less doing the same thing (run Node and interact with code-server via the socket stored in VSCODE_IPC_HOOK_CLI) so at the very least I think we would be maintaining the status quo. The extensions should work regardless of what is in the path right?

[benz0li]

The extensions should work regardless of what is in the path right?

@code-asher Not to have a misunderstanding here: Which extensions are you talking about?

[code-asher]

The VS Code remote extensions. Remote - SSH, remote - containers, and remote WSL I think. Although that issue (#1315) is only about implementing open-source versions of the first two (SSH and containers).

[benz0li]

Sorry for being dense; how does this change anything with https://github.com/coder/code-server/issues/1315?

Not because of /opt/code-server/lib/vscode/bin/remote-cli but Settings; This looks somehow confusing:

But it seems to work correctly. Workspace Settings override Remote Settings (~/.local/share/code-server/Machine/settings.json) override User Settings (~/.local/share/code-server/User/settings.json). 👉 User Settings being the settings of the local Machine – in VS Code terms.

If I remember correctly: When changing User Settings in older versions of code-server it always printed Also modified in: Remote, displaying the same value in Remote Settings as in User Setting.
ℹ️ This might have changed as soon as you have moved to upstream microsoft/vscode.

The VS Code remote extensions. Remote - SSH, remote - containers, and remote WSL I think.

👍

Although that issue (#1315) is only about implementing open-source versions of the first two (SSH and containers).

Yes, of course.

[benz0li]

@code-asher Thank you for your explanations.

[code-asher]

Ahh right I see what you mean! Yeah the settings situation has been confusing. Upstream User settings were changed to be stored in the browser so now they are local to the client and separate from remote but we patch them to use ~/.local/share/code-server/User/settings.json which makes them no longer local...definitely a weird situation. We did that for backwards compatibility but I wonder if we should remove that patch and recommend ~/.local/share/code-server/Machine/settings.json instead for seeding settings on the server. Definitely something to keep in mind as we try to align ourselves with upstream.

[benz0li]

Ahh right I see what you mean!

🙇

We did that for backwards compatibility but I wonder if we should remove that patch and recommend ~/.local/share/code-server/Machine/settings.json instead for seeding settings on the server.

For me the current patch is fine.

---

Cross references: https://github.com/coder/code-server/issues/2274, https://github.com/coder/code-server/issues/4609