Add vscode via vendor package. #4135

GirlBossRush · 2021-09-10T16:36:57Z

While #4010 resolves some incompatibilities with 1.60.0, it's still in review and needs additional testing. This PR updates lib/vscode to use a yarn package pointing to our VS Code fork. Once this is merged, any remaining fork clean up branches will be ported over to the vendored .

Fixes #3917

Notable changes from upstream

The log service now defers initialization.
Several terminal channels were added.
Several type definitions were adjusted.

Our changes

Remove asar linking.
Remove module lint check.
Use yarn for vscode vendoring.

How to review this PR

This is a somewhat unusual PR since we don’t yet have a diff to start with, however, I’ve made a branch on our fork with the previous changes form lib/vscode. This allows you compare all the changes from upstream, as well as the adjustments to our code to fix them in 1.60.0

coder/vscode@1.57.1-initial...cdr:code-server

Follow up

#4136 can be merged after this. They are only separated to make reviewing the changes easier to follow.

github-actions · 2021-09-10T16:44:32Z

✨ Coder.com for PR #4135 deployed! It will be updated on every commit.

Host: https://codercom-3l84iss2t-codercom.vercel.app/docs/code-server
Last deploy status: success
Commit: 44a4ae5
Workflow status: https://github.com/cdr/code-server/actions/runs/1238743982

vendor/package.json

jsjoeio · 2021-09-10T17:55:09Z

Remove asar linking.

Happy we're doing this. Those npm changes upstream were not fun to debug so glad we can avoid that.

jsjoeio

This is awesome 🎉

Seems like it's mostly location changes (I'm not seeing a ton of changes related to VS Code itself).

The biggest question for me is what's the explanation behind vendor/modules? Is this a yarn naming convention or our own? And it doesn't have to happen here, but it would be nice for us to document how VS Code exists in code-server. i.e. how yarn plays into the story and how this all works (for instance is this still considered a git submodule).

(I wish we would have done this when we switched to a git subtree so that the reasoning behind the change + how it works would have been documented).

This could come in a followup PR as a docs addition though.

.github/codeql-config.yml

.github/workflows/ci.yaml

ci/dev/fmt.sh

ci/dev/lint.sh

ci/dev/update-vscode.sh

codecov · 2021-09-10T18:33:57Z

Codecov Report

Merging #4135 (d823571) into main (8e08775) will not change coverage.
The diff coverage is 33.33%.

❗ Current head d823571 differs from pull request most recent head 44a4ae5. Consider uploading reports for the commit 44a4ae5 to get more accurate results

@@           Coverage Diff           @@
##             main    #4135   +/-   ##
=======================================
  Coverage   64.22%   64.22%           
=======================================
  Files          36       36           
  Lines        1873     1873           
  Branches      379      379           
=======================================
  Hits         1203     1203           
  Misses        569      569           
  Partials      101      101

Impacted Files	Coverage Δ
src/browser/pages/vscode.ts	`100.00% <ø> (ø)`
src/node/entry.ts	`0.00% <ø> (ø)`
src/node/main.ts	`39.77% <0.00%> (ø)`
src/node/routes/vscode.ts	`29.47% <0.00%> (ø)`
src/node/vscode.ts	`19.48% <100.00%> (ø)`

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 8e08775...44a4ae5. Read the comment docs.

jsjoeio · 2021-09-10T18:35:37Z

Seems to be running just fine locally!

I do see this error but guessing extension related (not your changes):

Weird...a reload of code-server fixes it 🤷‍♂️

Search for files and the terminal aren't working :( I triedcd lib/vscode && yarn again but that didn't fix it.

Screen.Recording.2021-09-10.at.11.23.59.AM.mov

What am I missing? Do I need to manually delete node-modules?

jsjoeio · 2021-09-10T18:51:46Z

The terminal process failed to launch: A native exception occurred during launch (The module '/Users/jp/Dev/code-server/vendor/modules/code-oss-dev/node_modules/node-pty/build/Release/pty.node' was compiled against a different Node.js version using NODE_MODULE_VERSION 89. This version of Node.js requires NODE_MODULE_VERSION 83. Please try re-compiling or re-installing the module (for instance, using npm rebuild or npm install).).

Yeah I tried deleting node_modules in lib/vscode and running yarn install but that didn't help.

Check the Node.js version that's used by Electron

I see the docs say this but I'm not sure how to check that

jsjoeio · 2021-09-10T19:01:01Z

Oh no...I think I broke something. So I ran cd vendor && yarn install --modules-folder modules --ignore-scripts and now when I go back to the root and run yarn watch I get this error:

yarn run v1.22.10
$ VSCODE_IPC_HOOK_CLI= NODE_OPTIONS='--max_old_space_size=32384 --trace-warnings' ts-node ./ci/dev/watch.ts
[tsc] [11:59:26 AM] Starting compilation in watch mode...
[vscode] $ npm-run-all -lp watch-client watch-extensions
/bin/sh: npm-run-all: command not found
error Command failed with exit code 127.
[vscode] info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
vs code watcher terminated unexpectedly
killing vs code watcher
killing tsc
killing watch
error Command failed with exit code 127.

I'm wondering if I should not use --ignore-scripts when developing locally (i.e. running yarn watch).

GirlBossRush · 2021-09-10T19:07:44Z

@jsjoeio,

I recommend doing yarn clean to completely erase your modules, then yarn from the project directory. Vendor modules will automatically be installed and yarn watch can be run. However, if you encounter that terminal error, your node version differs from a built package and you’ll have to do the following:

 cd vendor/modules/code-oss-dev
 npm rebuild

From there, yarn watch should behave correctly.

jsjoeio · 2021-09-10T21:02:08Z

Oh man... yarn clean didn't work. Going to try the second approach

argh, i'm getting this error now saying I need to use yarn 🤔

yarn rebuild only exists in Yarn 2 I think?

Gonna try running yarn in that directory instead of npm rebuild 🤷‍♂️

Nope, that didn't work either.

Node/yarn/VS Code why are you making my life so hard 😢

jsjoeio · 2021-09-10T21:16:36Z

I'm using Node v14.17.0 if that helps. I wish I knew how to check which Node version corresponded to version 83 vs 89

jsjoeio · 2021-09-10T21:19:26Z

I'm going to delete the code-server repo, reclone and try again

jsjoeio · 2021-09-10T21:32:06Z

Still no luck. I'm going to blame my Node version. What version are you using @TeffenEllis ?

code-asher

I have not looked at the fork at all but the changes here look great! Everything seems to run as expected.

Nice work!

ci/dev/fmt.sh

.gitignore

code-asher · 2021-09-10T21:16:05Z

.github/workflows/ci.yaml

        run: yarn --frozen-lockfile

      - name: Build code-server
        run: yarn build

-      # Parse the hash of the latest commit inside lib/vscode
+      # Parse the hash of the latest commit inside vendor/modules/code-oss-dev


Thoughts on keeping it in lib? (So lib/modules). Reason being we download other dependencies there (link agent and Node).

code-asher · 2021-09-10T21:32:54Z

ci/lib.sh

-# exist in node_modules so just symlink it. We have to do this since not only VS
-# Code itself but also extensions will look specifically in this directory for
-# files (like the ripgrep binary or the oniguruma wasm).
-symlink_asar() {


We likely need to keep these symlinks. When I built a release there was no vendor/modules/code-oss-dev/node_modules.asar directory which some extensions rely on.

To reproduce, install the Bracket Pair Colorizer 2 extension. You will see an error in the browser console and the brackets will not be colored.

I think VS Code does something where they duplicate some modules into this directory so maybe eventually we can use their build process instead although it seems like a waste of space. 👨‍🚀

ci/dev/postinstall.sh

.github/workflows/ci.yaml

code-asher · 2021-09-10T21:53:41Z

With regards to the Node module version errors, that might be because the code-oss-dev module we are pulling in has .yarnrc files? They tell Node to target Electron 13.1.8 instead of whatever version of Node you are running.

jsjoeio · 2021-09-10T22:25:43Z

Okay I downgraded to v14.15.2 then did cd vendor/modules/code-oss-dev && npm rebuild and it magically worked.

How I figured out Node 83? nvm ls-remote then counted backwards from 89 😅

       v14.15.0   (LTS: Fermium)
       v14.15.1   (LTS: Fermium)
       v14.15.2   (LTS: Fermium) (83?)
       v14.15.3   (LTS: Fermium)
       v14.15.4   (LTS: Fermium)
       v14.15.5   (LTS: Fermium)
       v14.16.0   (LTS: Fermium)
       v14.16.1   (LTS: Fermium)
->     v14.17.0   (LTS: Fermium) (89)
       v14.17.1   (LTS: Fermium)
       v14.17.2   (LTS: Fermium)
       v14.17.3   (LTS: Fermium)
       v14.17.4   (LTS: Fermium)
       v14.17.5   (LTS: Fermium)
       v14.17.6   (Latest LTS: Fermium)

P.S. @TeffenEllis yarn.lock under vendor might need to be updated? Or at least it shows changed when I run yarn watch locally.

jsjoeio · 2021-09-10T23:03:10Z

Video

✅ Besides Asher's requested changes, everything looks good on my side

Screen.Recording.2021-09-10.at.4.01.59.PM.mov

jsjoeio · 2021-09-13T20:55:34Z

codecov/patch Failing after 1s — 33.33% of diff hit (target 64.12%)

I thought I disabled this in a previous PR but apparently I didn't 😅 Here is PR to fix that.

Feel free to rebase after that gets merged in.

#4159

- Use yarn for vscode vendoring. - Grab hash from package.

code-asher

Fantastic!!

jsjoeio

Woohoo!!!

ci/dev/postinstall.sh

GirlBossRush added enhancement Some improvement that isn't a feature chore Related to maintenance or clean up labels Sep 10, 2021

GirlBossRush added this to the 3.12.0 milestone Sep 10, 2021

GirlBossRush requested review from jsjoeio and code-asher September 10, 2021 16:36

GirlBossRush self-assigned this Sep 10, 2021

GirlBossRush requested a review from a team as a code owner September 10, 2021 16:36

GirlBossRush mentioned this pull request Sep 10, 2021

Remove subtree #4136

Closed

GirlBossRush force-pushed the vscode-1.60.0-yarn branch 5 times, most recently from 39c8ad1 to 8c7b2b7 Compare September 10, 2021 17:31

kylecarbs reviewed Sep 10, 2021

View reviewed changes

vendor/package.json Show resolved Hide resolved

GirlBossRush force-pushed the vscode-1.60.0-yarn branch from 8c7b2b7 to 9d8bec1 Compare September 10, 2021 18:04

jsjoeio reviewed Sep 10, 2021

View reviewed changes

.github/codeql-config.yml Show resolved Hide resolved

.github/workflows/ci.yaml Outdated Show resolved Hide resolved

ci/dev/fmt.sh Show resolved Hide resolved

ci/dev/lint.sh Show resolved Hide resolved

ci/dev/update-vscode.sh Show resolved Hide resolved

GirlBossRush requested a review from kylecarbs September 10, 2021 18:12

GirlBossRush modified the milestones: 3.12.0, 3.12.1 Sep 10, 2021

GirlBossRush requested a review from jsjoeio September 10, 2021 20:26

code-asher requested changes Sep 10, 2021

View reviewed changes

GirlBossRush force-pushed the vscode-1.60.0-yarn branch from b33cdf8 to afb1419 Compare September 15, 2021 15:47

Add vscode via vendor package.

b9d79dc

- Use yarn for vscode vendoring. - Grab hash from package.

GirlBossRush force-pushed the vscode-1.60.0-yarn branch from afb1419 to b9d79dc Compare September 15, 2021 15:53

Disable cache.

699f78c

GirlBossRush force-pushed the vscode-1.60.0-yarn branch from f66a71e to d366690 Compare September 15, 2021 16:25

Enable cache on vendor assets.

d4dafd8

GirlBossRush force-pushed the vscode-1.60.0-yarn branch from d366690 to d4dafd8 Compare September 15, 2021 16:32

code-asher approved these changes Sep 15, 2021

View reviewed changes

jsjoeio approved these changes Sep 15, 2021

View reviewed changes

ci/dev/postinstall.sh Show resolved Hide resolved

GirlBossRush added 2 commits September 15, 2021 13:55

Fix outdated selector. Add debug info.

565dbec

Disable CI caching during build.

44a4ae5

GirlBossRush enabled auto-merge (rebase) September 15, 2021 18:12

GirlBossRush merged commit f937849 into main Sep 15, 2021

GirlBossRush deleted the vscode-1.60.0-yarn branch September 15, 2021 18:54

GirlBossRush mentioned this pull request Sep 15, 2021

Remove subtree. #4169

Merged

Jose-Matsuda mentioned this pull request Sep 28, 2021

Fix failing builds (code-server and sql-language-server) StatCan/aaw-kubeflow-containers#293

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vscode via vendor package. #4135

Add vscode via vendor package. #4135

GirlBossRush commented Sep 10, 2021 •

edited by jsjoeio

Loading

github-actions bot commented Sep 10, 2021 •

edited

Loading

jsjoeio commented Sep 10, 2021

jsjoeio left a comment

codecov bot commented Sep 10, 2021 •

edited

Loading

jsjoeio commented Sep 10, 2021

jsjoeio commented Sep 10, 2021

jsjoeio commented Sep 10, 2021

GirlBossRush commented Sep 10, 2021 •

edited by jsjoeio

Loading

jsjoeio commented Sep 10, 2021 •

edited

Loading

jsjoeio commented Sep 10, 2021

jsjoeio commented Sep 10, 2021

jsjoeio commented Sep 10, 2021

code-asher left a comment

code-asher Sep 10, 2021 •

edited

Loading

code-asher Sep 10, 2021 •

edited

Loading

code-asher commented Sep 10, 2021

jsjoeio commented Sep 10, 2021 •

edited

Loading

jsjoeio commented Sep 10, 2021

jsjoeio commented Sep 13, 2021

code-asher left a comment

jsjoeio left a comment

Add vscode via vendor package. #4135

Add vscode via vendor package. #4135

Conversation

GirlBossRush commented Sep 10, 2021 • edited by jsjoeio Loading

Notable changes from upstream

Our changes

How to review this PR

Follow up

github-actions bot commented Sep 10, 2021 • edited Loading

jsjoeio commented Sep 10, 2021

jsjoeio left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 10, 2021 • edited Loading

Codecov Report

jsjoeio commented Sep 10, 2021

jsjoeio commented Sep 10, 2021

jsjoeio commented Sep 10, 2021

GirlBossRush commented Sep 10, 2021 • edited by jsjoeio Loading

jsjoeio commented Sep 10, 2021 • edited Loading

jsjoeio commented Sep 10, 2021

jsjoeio commented Sep 10, 2021

jsjoeio commented Sep 10, 2021

code-asher left a comment

Choose a reason for hiding this comment

code-asher Sep 10, 2021 • edited Loading

Choose a reason for hiding this comment

code-asher Sep 10, 2021 • edited Loading

Choose a reason for hiding this comment

code-asher commented Sep 10, 2021

jsjoeio commented Sep 10, 2021 • edited Loading

jsjoeio commented Sep 10, 2021

Video

jsjoeio commented Sep 13, 2021

code-asher left a comment

Choose a reason for hiding this comment

jsjoeio left a comment

Choose a reason for hiding this comment

GirlBossRush commented Sep 10, 2021 •

edited by jsjoeio

Loading

github-actions bot commented Sep 10, 2021 •

edited

Loading

codecov bot commented Sep 10, 2021 •

edited

Loading

GirlBossRush commented Sep 10, 2021 •

edited by jsjoeio

Loading

jsjoeio commented Sep 10, 2021 •

edited

Loading

code-asher Sep 10, 2021 •

edited

Loading

code-asher Sep 10, 2021 •

edited

Loading

jsjoeio commented Sep 10, 2021 •

edited

Loading