Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor (vscode): migrate from submodule to subtree #2476

Merged
merged 16 commits into from
Dec 17, 2020

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Dec 15, 2020

This PR changes our current approach of using vscode as a submodule to using it as a subtree.

Changes

  • remove submodule
  • add subtree
  • apply patch
  • remove old files and scripts
    • ci/dev/vscode.patch
    • ci/dev/patch-vscode.sh and vscode:patch from package.json
    • ci/dev/diff-vscode.sh and vscode:diff from package.json
  • remove vscode.sh and add postinstall script to package.json

Remaining Todos

  • update ci/dev scripts
  • update ci/build scripts
  • update docs
  • update contributing docs
  • address feedback
  • add | grep -v "lib/vscode" to all the git ls-files invocations

Screenshots

2020-12-16 11 57 16

Checklist

  • tested locally
  • updated the docs

Fixes #1587

Many thanks to @SPGoding for testing out this approach and outlining the steps to start this effort! 🎉

@jsjoeio jsjoeio self-assigned this Dec 15, 2020
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Dec 15, 2020

Okay made some initial progress. The migration piece was the easy part. The harder part is understanding how to update our vscode.sh script.

How do we keep our subtree up to date with vscode?

Thinking a bit out loud here...

  • when someone is making a change to our codebase, they don't need to run any funky commands because the vscode code is already there (thanks to the subtree)
  • when we (maintainers) need to update the subtree (as in update vscode from 1.5.1 to 1.5.2, how do we do it?

If it were a normal repository, like a fork I'd probably fetch the latest changes then rebase my changes on top of those. But subtree's don't support rebase, only merge strategies. So we have to figure that out.

Resources that may help:

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Dec 16, 2020

Maybe something like this?

  # Add vscode as a new remote
  # TODO only do once
  git remote add -f vscode https://github.com/microsoft/vscode.git

  # Prep to merge with version we're currently on
  git merge -s ours --no-commit --allow-unrelated-histories vscode/release/1.51

  # Read changes to our sub directory
  git read-tree --prefix=lib/vscode -u vscode/release/1.51

  # Commit changes
  # git commit -m "chore: update vscode"

@code-asher
Copy link
Member

code-asher commented Dec 16, 2020

I think all we need to do in vscode.sh is install VS Code's Node modules. Actually maybe we should just move that to postinstall in package.json so you don't need to run a separate yarn vscode.

That way development would just be:

$ yarn
$ yarn watch

As for updating VS Code once the remote is added I think all we need is something like:

$ git subtree pull --prefix lib/vscode vscode release/1.52 --squash --message "Update VS Code to 1.52.1"

I'm not sure we need a script for that, maybe we just add it to the contributing docs.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Dec 16, 2020

Actually maybe we should just move that to postinstall in package.json so you don't need to run a separate yarn vscode

Great idea! Didn't think of that.

I'm not sure we need a script for that, maybe we just add it to the contributing docs.

Ah, if it's only one line, I agree, we probably don't need that. I'll add to contributing. Thanks @code-asher

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Dec 16, 2020

Alright a few errors I'm seeing.

Error in fmt script

In our ci / fmt workflow, it's upset with the let declaration inside the for loop. Maybe when that gets called $1 is undefined?

lib/vscode/src/vs/base/node/cpuUsage.sh:9:11: = must follow a name

The code there reads:

#!/bin/bash

function get_total_cpu_time() {
  # Read the first line of /proc/stat and remove the cpu prefix
  CPU=(`sed -n 's/^cpu\s//p' /proc/stat`)

  # Sum all of the values in CPU to get total time
  for VALUE in "${CPU[@]}"; do
    let $1=$1+$VALUE
  done
}

(see file on remote)

Error in lint script

It seems like it can't find our .eslint.yaml or rather the node_modules where these rule definitions come from.

/github/workspace/lib/vscode/test/automation/src/explorer.ts
  1:1  error  Definition for rule 'code-no-unused-expressions' was not found        code-no-unused-expressions
  1:1  error  Definition for rule 'code-translation-remind' was not found           code-translation-remind
  1:1  error  Definition for rule 'code-no-nls-in-standalone-editor' was not found  code-no-nls-in-standalone-editor
  1:1  error  Definition for rule 'code-no-standalone-editor' was not found         code-no-standalone-editor
  1:1  error  Definition for rule 'code-no-unexternalized-strings' was not found    code-no-unexternalized-strings

Error on linux-arm64

It's out of space.

cp: error writing './release-gcp/3.7.4/linux-arm64.tar.gz': No space left on device

doc/CONTRIBUTING.md Outdated Show resolved Hide resolved
doc/CONTRIBUTING.md Outdated Show resolved Hide resolved
@jsjoeio jsjoeio marked this pull request as ready for review December 16, 2020 19:25
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very exciting! A couple other areas we should update:

  • tour/contributing.tour: Change "file": "ci/dev/vscode.patch" to "directory": "lib/vscode" and update the wording to not reference the patch.
  • There are some references to applying the patch in ci/README.md which we should remove.

I haven't looked at the lint/fmt errors or the space issue yet.

.tours/start-development.tour Show resolved Hide resolved
doc/CONTRIBUTING.md Outdated Show resolved Hide resolved
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Dec 16, 2020

Re: Feedback in Slack from @code-asher

we might need to use something other than git ls-files for determining what to run our formatter and linter on
we can do this by adding | grep -v "lib/vscode" to all the git ls-files invocations

ci/dev/fmt.sh Outdated Show resolved Hide resolved
@code-asher
Copy link
Member

code-asher commented Dec 17, 2020

For the space issue it seems to run out when copying to the GCP directory (build-packages.sh). I think we just remove that for now since we don't currently upload automatically to GCP anyway and I think it might make sense to add a separate step that uploads the releases from GitHub to GCP instead of doing it this way anyway.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Dec 17, 2020

For the space issue it seems to run out when copying to the GCP directory (build-packages.sh). I think we just remove that for now since we don't currently upload automatically to GCP anyway

Good call. I removed it in 96576bd

and I think it might make sense to add a separate step that uploads the releases from GitHub to GCP instead of doing it this way anyway.

Do you want me to open an issue for us to do that in the future?

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Dec 17, 2020

$ ./ci/build/build-packages.sh
done (release-packages/code-server-3.7.4-linux-arm64)
guessing packager from target file extension...
using deb packager...
nfpm: error: cannot add data.tar.gz to deb: write release-packages/code-server_3.7.4_arm64.deb: no space left on device

That's interesting...It's still trying to add it. I must have removed the wrong step.

@code-asher
Copy link
Member

Do you want me to open an issue for us to do that in the future?

No need, we're tracking it internally.

That's interesting...It's still trying to add it. I must have removed the wrong step.

Nah you got it right, it's just running out of space later down the line now.

@jsjoeio jsjoeio force-pushed the issue-1587-submodule-to-subtree branch from 4203bfc to 74d6d5e Compare December 17, 2020 19:17
@jsjoeio jsjoeio removed the request for review from nhooyr December 17, 2020 19:33
@jsjoeio jsjoeio merged commit 619ab45 into master Dec 17, 2020
@jsjoeio jsjoeio deleted the issue-1587-submodule-to-subtree branch December 17, 2020 22:29
@nhooyr
Copy link
Contributor

nhooyr commented Dec 18, 2020

May submodules burn in hell!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants