fix: invoking code-server in integrated terminal #5360

code-asher · 2022-07-20T21:20:39Z

This fixes the path to Node in upstream's scripts then links the right one based on the platform.

There is also some removal of duplicate code where we symlink the asar twice; turns out we do not need this since the standalone release runs yarn which will run the postinstall which will add the link.

If anyone wishes to test manually open up the integrated terminal and run code-server <file>. You can try things like code-server --wait <file> as well which should work.

Possibly in a separate PR we should think about using the same for external terminals then remove our bespoke code.

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

Fixes #2110

Follow-up TODOs

open issue for external terminal usage (aka --wait outside code-server)

codecov · 2022-07-20T21:34:32Z

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 · 2022-07-21T00:18:56Z

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 · 2022-07-21T04:07:39Z

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 · 2022-07-21T15:48:02Z

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 · 2022-07-21T17:02:23Z

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 · 2022-07-21T17:04:34Z

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

benz0li · 2022-07-21T17:36:27Z

@code-asher What about #5335 (comment)?

code-asher · 2022-07-21T18:56:39Z

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 · 2022-07-21T19:12:19Z

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 · 2022-07-21T19:32:24Z

IMHO there should be no file /opt/code-server/lib/vscode/bin/remote-cli/code-server. Reposting #5335 (comment) 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

/opt/code-server/lib/vscode/bin/remote-cli/code-oss
/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 · 2022-07-21T19:44:08Z

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 · 2022-07-22T04:18:59Z

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 · 2022-07-22T05:20:26Z

/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 · 2022-07-22T05:35:04Z

Also reposting #5335 (comment) 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 #1315.
ℹ️ In VS Code it is not possible to start another remote connection from within a remote connection.

code-asher · 2022-07-22T15:26:24Z

/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 · 2022-07-22T15:33:30Z

Sorry for being dense; how does this change anything with #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 · 2022-07-22T15:36:24Z

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 · 2022-07-22T15:39:54Z

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 · 2022-07-22T16:16:31Z

Sorry for being dense; how does this change anything with #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 · 2022-07-22T16:19:33Z

@code-asher Thank you for your explanations.

code-asher · 2022-07-22T16:48:27Z

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 · 2022-07-22T17:40:34Z

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: #2274, #4609

Removing it did not help. Still not sure if this is necessary though.

Otherwise if we change the script it will not rebuild Code.

The selector was timing out even though it matched more than one element but matching on the focused one appears to work. In addition add a loop so even it can keep trying to open the terminal if something goes wrong with the focus.

jsjoeio

We reviewed this together in person at the off-site. All looks good!

code-asher marked this pull request as ready for review July 21, 2022 00:01

code-asher requested a review from a team July 21, 2022 00:01

code-asher marked this pull request as draft July 21, 2022 00:18

tjcran mentioned this pull request Jul 25, 2022

[Bug]: Cannot invoke code-server from integrated terminal #5335

Closed

4 tasks

code-asher force-pushed the code-server-path branch 4 times, most recently from 5237b0a to ba4306f Compare August 1, 2022 16:59

Make sed work on macOS

a5d2200

code-asher force-pushed the code-server-path branch 3 times, most recently from ee9a637 to b7b2128 Compare August 2, 2022 15:53

code-asher added 4 commits August 2, 2022 10:55

Add back wait for load state

b732bf9

Removing it did not help. Still not sure if this is necessary though.

Fix Node path in bin scripts

43225a3

Disable shellcheck expansion error

8c2096f

Make scripts executable

44bd6ff

code-asher force-pushed the code-server-path branch from b7b2128 to 86db6e4 Compare August 2, 2022 15:55

code-asher added 2 commits August 2, 2022 11:40

Remove .bak files created by sed

4a00441

Include Code build script in cache hash

602f8d4

Otherwise if we change the script it will not rebuild Code.

code-asher force-pushed the code-server-path branch 8 times, most recently from 99e3167 to 7b2ddc7 Compare August 2, 2022 18:12

Make sure the terminal opens

b62191a

The selector was timing out even though it matched more than one element but matching on the focused one appears to work. In addition add a loop so even it can keep trying to open the terminal if something goes wrong with the focus.

code-asher force-pushed the code-server-path branch from 7b2ddc7 to b62191a Compare August 2, 2022 18:40

code-asher marked this pull request as ready for review August 2, 2022 19:43

code-asher requested a review from jsjoeio August 2, 2022 19:43

jsjoeio approved these changes Aug 3, 2022

View reviewed changes

Merge branch 'main' into code-server-path

8e03cbb

code-asher enabled auto-merge (squash) August 4, 2022 15:40

code-asher merged commit 9087e0c into coder:main Aug 4, 2022

fritterhoff mentioned this pull request Aug 11, 2022

[Feat]: Upgrade vscode to 1.70 #5417

Closed

benz0li mentioned this pull request Aug 29, 2022

Coder/code-server conflicts with the startup command of visual studio code server[Bug]: #5502

Closed

4 tasks

benz0li mentioned this pull request Oct 5, 2022

[Bug]: code-server (run with docker) will always overwrite PATH variable #5621

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: invoking code-server in integrated terminal #5360

fix: invoking code-server in integrated terminal #5360

code-asher commented Jul 20, 2022 •

edited

Loading

codecov bot commented Jul 20, 2022 •

edited

Loading

code-asher commented Jul 21, 2022

benz0li commented Jul 21, 2022 •

edited

Loading

jsjoeio commented Jul 21, 2022

code-asher commented Jul 21, 2022 •

edited

Loading

code-asher commented Jul 21, 2022

benz0li commented Jul 21, 2022

code-asher commented Jul 21, 2022 •

edited

Loading

fritterhoff commented Jul 21, 2022

benz0li commented Jul 21, 2022 •

edited

Loading

code-asher commented Jul 21, 2022

benz0li commented Jul 22, 2022

benz0li commented Jul 22, 2022

benz0li commented Jul 22, 2022

code-asher commented Jul 22, 2022

code-asher commented Jul 22, 2022 •

edited

Loading

benz0li commented Jul 22, 2022

code-asher commented Jul 22, 2022 •

edited

Loading

benz0li commented Jul 22, 2022

benz0li commented Jul 22, 2022

code-asher commented Jul 22, 2022 •

edited

Loading

benz0li commented Jul 22, 2022

jsjoeio left a comment

fix: invoking code-server in integrated terminal #5360

fix: invoking code-server in integrated terminal #5360

Conversation

code-asher commented Jul 20, 2022 • edited Loading

codecov bot commented Jul 20, 2022 • edited Loading

Codecov Report

code-asher commented Jul 21, 2022

benz0li commented Jul 21, 2022 • edited Loading

jsjoeio commented Jul 21, 2022

code-asher commented Jul 21, 2022 • edited Loading

code-asher commented Jul 21, 2022

benz0li commented Jul 21, 2022

code-asher commented Jul 21, 2022 • edited Loading

fritterhoff commented Jul 21, 2022

benz0li commented Jul 21, 2022 • edited Loading

code-asher commented Jul 21, 2022

benz0li commented Jul 22, 2022

benz0li commented Jul 22, 2022

benz0li commented Jul 22, 2022

code-asher commented Jul 22, 2022

code-asher commented Jul 22, 2022 • edited Loading

benz0li commented Jul 22, 2022

code-asher commented Jul 22, 2022 • edited Loading

benz0li commented Jul 22, 2022

benz0li commented Jul 22, 2022

code-asher commented Jul 22, 2022 • edited Loading

benz0li commented Jul 22, 2022

jsjoeio left a comment

Choose a reason for hiding this comment

code-asher commented Jul 20, 2022 •

edited

Loading

codecov bot commented Jul 20, 2022 •

edited

Loading

benz0li commented Jul 21, 2022 •

edited

Loading

code-asher commented Jul 21, 2022 •

edited

Loading

code-asher commented Jul 21, 2022 •

edited

Loading

benz0li commented Jul 21, 2022 •

edited

Loading

code-asher commented Jul 22, 2022 •

edited

Loading

code-asher commented Jul 22, 2022 •

edited

Loading

code-asher commented Jul 22, 2022 •

edited

Loading