Try moving build to just gulp by jcreedcmu · Pull Request #503 · github/vscode-codeql

jcreedcmu · 2020-07-16T13:34:03Z

This PR is a proposal for how to fulfill #498. All build configuration is moved into extensions/ql-vscode and now only one buildable module exists, so we don't use rush anymore. The Actions workflow files call gulp directly. Building the package with vsce seems to remove devDependencies just fine by itself, so we no longer have to do any traversal of our dependencies, just copy node_modules to the dist directory and let vsce do its thing.

In fact, the only reason we have to create a distribution directory and copy anything at all is because we fiddle with the version number in package.json. But this seems fine --- it lets us have exact control over which files go in the package. But maybe vsce pays attention to the "files" section in package.json, I haven't checked that.

jcreedcmu · 2020-07-16T13:34:54Z

(for a sense of scale, this diff is +196 loc, -3116 loc excluding the shrinkwrap/lock files)

aeisenberg

At some point, we should move the extension out of the extensions/ql folder to the top level. Is there any reason not to do this?

We can wait for a different PR since this one is so large.

jcreedcmu · 2020-07-17T12:18:26Z

absolutely; that seems like the immediate next step. I only postponed it because this is plenty big and it seemed less deeply consequential (although it requires changing some relative paths in a few files iirc)

aeisenberg · 2020-07-21T15:19:32Z

        "isDefault": true
      },
-      "command": "node common/scripts/install-run-rush.js build --verbose",
+      "command": "npx gulp buildWithoutPackage --verbose",


I dont think we need npx here. We can assume users have already run npm run build -- buildWithoutPackage --verbose before running in vs code. So, just do the same, but remove npx. You may need to add a full path here, so ${workspaceRoot}/extensions/ql-vscode/node_modules/.bin/gulp.

I sort of like npx as a terse idiom here, but I don't mind being more explicit if you think it's better to avoid the magic of npx auto-downloading packages. I'm pretty sure we already need to be in the correct cwd to have gulp work, so a path relative to extensions/ql-vscode should work.

My concern with npx is with versioning. I've had situations before where people ran into different behaviour because npx had installed different versions. If we are running from the node_modules directory, this won't happen.

aeisenberg · 2020-07-21T15:23:05Z

-        run: node common/scripts/install-run-rush.js build
+        run: |
+          cd extensions/ql-vscode
+          gulp


Can we change this to npm run build? This will ensure $PATH is correct.

Sure, and I agree with similar comments below that confining any call to gulp into the package.json scripts is generally a better approach.

aeisenberg · 2020-07-21T15:23:18Z

-        run: node common/scripts/install-run-rush.js build
+        run: |
+          cd extensions/ql-vscode
+          gulp


aeisenberg · 2020-07-21T15:23:47Z

-        run: node common/scripts/install-run-rush.js build --release
+        run: |
+          cd extensions/ql-vscode
+          gulp --release


Can we add a npm run release:build script?

Or, npm run build -- --release should work.

aeisenberg · 2020-07-21T15:28:02Z

    '--out', path.resolve(deployedPackage.distPath, '..', `${deployedPackage.name}-${deployedPackage.version}.vsix`)
  ];
-  const proc = child_process.spawn('vsce', args, {
+  const proc = childProcess.spawn('npx', ['vsce'].concat(args), {


I'd like to avoid npx and use the package-installed version of vsce.

Agree as above.

aeisenberg · 2020-07-21T15:29:39Z

    "format-staged": "lint-staged"
  },
  "dependencies": {
+    "@types/semver": "~7.2.0",


Can you move this to devDependencies?

Oops, good catch.

aeisenberg · 2020-07-21T15:30:14Z

@@ -1,3 +1,32 @@
 {
-  "extends": "./node_modules/typescript-config/extension.tsconfig.json"
+  "$schema": "http://json.schemastore.org/tsconfig",


Is this line important?

I don't presently know of anything that requires it but it occurred in other tsconfig files in the previous state of this repo, and it seemed like fine documentation. I have no overwhelmingly strong opinion about including or eliminating it.

I'm fine with keeping it. I've just never seen it explicitly before. I think vscode adds it automatically, but other tools may not.

aeisenberg · 2020-07-21T16:53:21Z

NIce.

jcreedcmu · 2020-07-21T16:57:07Z

Still need to fix CONTRIBUTING.md now.

jcreedcmu added 16 commits July 14, 2020 08:00

Remove now-unused libraries.

e5e8548

Copy semmle-vscode-utils into extension.

ac941eb

Remove now unused library.

53d5c24

inline tsconfig inheritance

657df5e

Remove typescript-config package

f005591

Make stub typescript gulpfile

2c5b672

Move more config into local typescript gulpfile

528cbc8

Replace javascript gulpfile with typescript

49c0d39

Reconcile vscode-engine and api versions

9083c5d

Fix package deploy to not depend on rush

56b62ff

Update action to not depend on rush

19a76dc

Fix warning

2b53396

Run npm-installed copy of vsce

bce3413

Fix test section of workflow file

d213890

Remove now unused rush configuration

b7a72b9

Relax version constraints in package.json

258f431

jcreedcmu requested a review from aeisenberg July 16, 2020 13:34

aeisenberg reviewed Jul 16, 2020

View reviewed changes

jcreedcmu added 2 commits July 21, 2020 09:21

Extract rush from vscode tasks

2671414

Fix source maps

cd329ee

jcreedcmu marked this pull request as ready for review July 21, 2020 14:10

aeisenberg reviewed Jul 21, 2020

View reviewed changes

jcreedcmu added 3 commits July 21, 2020 12:13

Move type dependency to devDependencies

3c07be5

Use explicit path for vsce

f8804f9

Call into package.json scripts from actions workflow

9ad0bf6

aeisenberg approved these changes Jul 21, 2020

View reviewed changes

jcreedcmu merged commit b3dc7d7 into github:main Jul 21, 2020

jcreedcmu mentioned this pull request Jul 27, 2020

Fix dependency of queryserver client on compiled code #268

Closed

Conversation

jcreedcmu commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcreedcmu commented Jul 16, 2020

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

jcreedcmu commented Jul 17, 2020

Uh oh!

aeisenberg Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeisenberg commented Jul 21, 2020

Uh oh!

jcreedcmu commented Jul 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jcreedcmu commented Jul 16, 2020 •

edited

Loading

aeisenberg Jul 21, 2020 •

edited

Loading