TS: Handle monorepos by rewriting package.json by asgerf · Pull Request #2619 · github/codeql

asgerf · 2020-01-10T17:06:19Z

Extends the dependency-installation support to handle monorepos, that is, repositories containing multiple packages. This also includes repos containing packages that haven't been published to NPM, such as our own vscode-codeql.

Update: it now works as follows:

Rewrite package.json files and save them under SCRATCH_DIR, removing references to packages that exist in the same repo.
Run yarn install for each package.json file in SCRATCH_DIR.
Override hooks in the TypeScript compiler host to resolve packages in SCRATCH_DIR or to a package that exists in the same repo.

Original (obsolete) PR description

~~It works as follows:~~

~~For each package.json file with name, install a symlink to it in <source_root>/node_modules.~~
~~Delete all mentions of the above package.json files from dependencies etc.~~
Rewrite the main field to ./src so TypeScript can find the contents of the package in terms of its original source files. Typically main refers to a non-existing file such as ./out/index.js which won't exist unless we run the full build. (Normally TypeScript would use the .d.ts files in the out directory)
~~Run yarn install for each package.json file, as before.~~
~~Extract TypeScript files.~~
~~Restore original package.json files. We don't clean up node_modules folders as there isn't a concrete reason for doing so at the moment.~~

~~It's a bit dirty to rewrite files on disk, but keep in mind that this is only intended to use on LGTM. Users of the codeql CLI should install dependencies themselves before extracting.~~

max-schaefer · 2020-01-13T09:03:33Z

javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java

+                Path symlinkPath = rootNodeModules.resolve(name);
+                if (!Files.exists(symlinkPath)) {
+                  Files.createDirectories(symlinkPath.getParent());
+                  Files.createSymbolicLink(symlinkPath, file.getParent());


Is this going to work on Windows? StackOverflow suggests that one needs special permissions to create symbolic links on Windows. Not an issue for LGTM.com, of course, but still relevant for LGTM-E.

That's a good question. I'm having some difficulty getting Java to run on my Windows VM today but I'll try to test it this week.

FWIW yarn workspaces use symlinks and yarn manages to create them just fine on Windows, so maybe can use that as a fallback.

Turns out yarn workspaces use "junctions", a type of symlink on NTFS which doesn't require admin. The Java API doesn't have a method for creating them.

Before going any further with the symlink approach, I'll experiment a bit with providing in a virtual file system in the compiler host we pass to the TypeScript compiler.

max-schaefer · 2020-01-13T09:05:10Z

but keep in mind that this is only intended to use on LGTM

Could you briefly explain how this is enforced?

asgerf · 2020-01-13T16:35:35Z

but keep in mind that this is only intended to use on LGTM

Could you briefly explain how this is enforced?

By having an undocumented environment variable you need to set (LGTM_INDEX_TYPESCRIPT_INSTALL_DEPS=true)

asgerf · 2020-01-23T10:10:02Z

I've pushed a revised version that no longer relies on symlinks, and doesn't pollute the original source checkout. We now install dependencies in SCRATCH_DIR and override various hooks in the TypeScript compiler host.

asgerf · 2020-01-23T12:42:23Z

@erik-krogh do you think you could help review this?

@max-schaefer I'd still be interested in your take on this, if you have time, though the PR has grown quite a bit now.

erik-krogh

I have not tried to run your code, but your idea of hooking into the TypeScript compiler looks good.
(Your comments really helped me understand the code).

This review was mainly me trying to understand all the parts of your code.
I'll take a second look later.

javascript/extractor/lib/typescript/src/common.ts

javascript/extractor/lib/typescript/src/main.ts

javascript/extractor/lib/typescript/src/common.ts

javascript/extractor/lib/typescript/src/main.ts

javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java

Co-Authored-By: Erik Krogh Kristensen <erik-krogh@github.com>

…-monorepo-deps

erik-krogh

LGTM, but I still haven't run the code.

Before I approve: Could you post a few links to some monorepos, then I'll confirm that it works on them.

We should probably get another pair of eyes on this (@esbena?).

erik-krogh · 2020-01-27T11:55:06Z

javascript/extractor/lib/typescript/src/common.ts

+      let redirected = this.redirectModuleName(moduleName, containingFile, options);
+      if (redirected != null) return redirected;
+      return ts.resolveModuleName(moduleName, containingFile, options,  host, resolutionCache).resolvedModule;


So, ts.resolveModuleName uses a cache, and this.redirectModuleName doesn't (sometimes).

I'm just wondering whether there is a reason for ts.resolveModuleName to use a cache beyond performance.
Because if there is, then we should probably add a similar cache.

From my own look into how resolveModuleNames is implemented in TypeScript it seems like the cache is used purely for performance.

Yes, I'm pretty sure the cache is just for performance.

The cache makes sense, because it's a lot of work to locate a package in the general case. The node_modules folder can be in a bunch of places, and there's both the actual package and its companion in @types to look for, and TypeScript also have to parse the package.json file to look at its typings field, and I'm sure there's more.

In our case, the packageEntryPoints map makes it a lot easier though, as it mostly does what the cache would do anyway. We do have to a try a few file extensions when looking for the file on disk, which I suppose we could cache as well, but I'm not sure it matters.

asgerf · 2020-01-28T12:44:03Z

Could you post a few links to some monorepos, then I'll confirm that it works on them.

I've been testing on these three, though it would be great if you can find more:

max-schaefer

Fantastic work! Looks great overall. I have a few very minor suggestions and comments, but none of it is blocking.

max-schaefer · 2020-01-29T12:02:04Z

javascript/extractor/lib/typescript/src/main.ts

+    console.warn('TypeScript: reported ' + diagnostics.length + ' semantic errors.');
+    for (let diagnostic of diagnostics) {
+        let text = diagnostic.messageText;
+        if (typeof text !== 'string') {


Would it be worth adding a text && check? I'm sure it won't happen in practice, but since all we do with text is to log it the console it doesn't seem worth crashing the extractor if it should happen somehow.

javascript/extractor/lib/typescript/src/virtual_source_root.ts

max-schaefer · 2020-01-29T12:05:28Z

javascript/extractor/lib/typescript/src/virtual_source_root.ts

+import * as ts from "./typescript";
+
+/**
+ * Mapping from the source root to the virtual source root.


Perhaps say "real source root" here, since that's the terminology used below? Also, would it perhaps be worth briefly outlining what these two roots are and how they relate to each other? (Perhaps something like the comment on line 12 below.)

javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java

max-schaefer · 2020-01-29T12:12:31Z

javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java

+                propsToRemove.add(packageName);
+              } else {
+                // Remove file dependency on a package that don't exist in the checkout.
+                String dependecy = getChildAsString(dependencyObj, packageName);


Suggested change

String dependecy = getChildAsString(dependencyObj, packageName);

String dependency = getChildAsString(dependencyObj, packageName);

(And uses below.)

max-schaefer · 2020-01-29T12:22:03Z

javascript/extractor/src/com/semmle/js/extractor/EnvironmentVariables.java

+  public static String getScratchDir() {
+    String env = Env.systemEnv().get(CODEQL_EXTRACTOR_JAVASCRIPT_SCRATCH_DIR_ENV_VAR);
+    if (env == null) {
+      throw new UserError(CODEQL_EXTRACTOR_JAVASCRIPT_SCRATCH_DIR_ENV_VAR + " must be set");


Will this work on LGTM? I don't think it sets that environment variable, does it? However, LGTM_WORKSPACE looks like a suitable alternative and is used by a few of the other autobuilders.

Looking at the other AutoBuild scripts, it seems that LGTM_WORKSPACE is set to whatever value is already in CODEQL_EXTRACTOR_LANG_SCRATCH_DIR.

I'm not sure I understand. Do you mean that LGTM sets LGTM_WORKSPACE to CODEQL_EXTRACTOR_LANG_SCRATCH_DIR? But who sets the latter? LGTM doesn't use CodeQL.

max-schaefer · 2020-01-29T12:22:44Z

javascript/extractor/lib/typescript/src/common.ts

+/**
+ * Extracts the package name from the prefix of an import string.
+ */
+const packageNameRex = /^(?:@[\w.-]+[/\\])?\w[\w.-]*(?=[/\\]|$)/;


Can import paths contain double slashes?

Indeed they can and it seems TypeScript will happily resolve them. We should handle it now.

Co-Authored-By: Max Schaefer <54907921+max-schaefer@users.noreply.github.com>

erik-krogh · 2020-01-30T14:12:29Z

NodeJS ran out of memory trying to extract angular/angular.

But I got it to work on github/vscode-codeql, and it seems to work, so 👍 from here.

asgerf · 2020-02-05T10:03:21Z

@erik-krogh could you review the last two commits?

NodeJS ran out of memory trying to extract angular/angular.

Hm, that's unfortunate. The default memory limit of 1G might not be enough for this project when dependencies are included, but I doubt it's an issue on LGTM.com where the Node.js process has 4G. Maybe we should raise the default limit anyway, but that can be done in a separate PR.

erik-krogh

@erik-krogh could you review the last two commits?

👍 from here.

NodeJS ran out of memory trying to extract angular/angular.

Hm, that's unfortunate. The default memory limit of 1G might not be enough for this project when dependencies are included, but I doubt it's an issue on LGTM.com where the Node.js process has 4G. Maybe we should raise the default limit anyway, but that can be done in a separate PR.

Could you do a local test that angular/angular works with 4GB before you merge?

asgerf · 2020-02-05T10:50:44Z

Could you do a local test that angular/angular works with 4GB before you merge?

Yep, works for me, installation+extraction took 19 minutes.

asgerf added JS WIP This is a work-in-progress, do not merge yet! labels Jan 10, 2020

max-schaefer reviewed Jan 13, 2020

View reviewed changes

asgerf added 7 commits January 22, 2020 10:52

TS: Handle monorepos by rewriting package.json

dde0f86

TS: Print TypeScript semantic errors in log

71b5407

JS: Make return type class for installDependencies()

21eecc4

TS: Guess main file location

303bac9

TS: Install deps under scratch dir

a220268

TS: Add some documentation

5719b44

TS: Support tsconfig.json extending from ./node_modules

7e8fb14

asgerf force-pushed the ts-monorepo-deps branch from 542e338 to 7e8fb14 Compare January 22, 2020 17:30

TS: Only require SCRATCH_DIR when installing dependencies

dc30dcf

erik-krogh reviewed Jan 23, 2020

View reviewed changes

asgerf and others added 9 commits January 23, 2020 16:13

TS: Be compatible with odasa/qltest

852b90a

TS: Allow .js extensions in cross package imports

fc04e06

TS: Simplify string equality check

542ce81

TS: Remove unneeded alias PackageLocationMap

804aef5

TS: Document nodeModulesRex

3ca5a3d

Update javascript/extractor/lib/typescript/src/main.ts

5448bff

Co-Authored-By: Erik Krogh Kristensen <erik-krogh@github.com>

Merge branch 'ts-monorepo-deps' of github.com:asger-semmle/ql into ts…

9ed7758

…-monorepo-deps

TS: Move definition of mainStr

1f64722

TS: Address comments in guessMainFile

7fa0fea

asgerf marked this pull request as ready for review January 24, 2020 10:15

asgerf requested a review from a team as a code owner January 24, 2020 10:15

erik-krogh reviewed Jan 27, 2020

View reviewed changes

max-schaefer reviewed Jan 29, 2020

View reviewed changes

asgerf and others added 4 commits January 30, 2020 12:28

TS: Handle multiple slashes in scope name

141d4bf

JS: Handle LGTM_WORKSPACE and fix emptiness check

92dbfb2

TS: Other review comments

1bf8165

Apply suggestions from code review

b88cc50

Co-Authored-By: Max Schaefer <54907921+max-schaefer@users.noreply.github.com>

max-schaefer previously approved these changes Jan 30, 2020

View reviewed changes

asgerf dismissed max-schaefer’s stale review via 915bb0f February 3, 2020 10:25

TS: Pass virtual source root explicitly to Node.js process

3c1cbce

asgerf force-pushed the ts-monorepo-deps branch from 915bb0f to 3c1cbce Compare February 3, 2020 10:36

asgerf removed the WIP This is a work-in-progress, do not merge yet! label Feb 4, 2020

TS: Only print number of errors if there were any

db2212e

erik-krogh approved these changes Feb 5, 2020

View reviewed changes

max-schaefer approved these changes Feb 5, 2020

View reviewed changes

semmle-qlci merged commit a5e183b into github:master Feb 5, 2020

	String dependecy = getChildAsString(dependencyObj, packageName);
	String dependency = getChildAsString(dependencyObj, packageName);

Conversation

asgerf commented Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Original (obsolete) PR description

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!

max-schaefer commented Jan 13, 2020

Uh oh!

asgerf commented Jan 13, 2020

Uh oh!

asgerf commented Jan 23, 2020

Uh oh!

asgerf commented Jan 23, 2020

Uh oh!

erik-krogh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erik-krogh left a comment

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!

asgerf commented Jan 28, 2020

Uh oh!

max-schaefer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

asgerf Jan 30, 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!

erik-krogh commented Jan 30, 2020

Uh oh!

asgerf commented Feb 5, 2020

Uh oh!

erik-krogh left a comment

Choose a reason for hiding this comment

Uh oh!

asgerf commented Feb 5, 2020

Uh oh!

Reviewers

Assignees

asgerf commented Jan 10, 2020 •

edited

Loading

asgerf Jan 30, 2020 •

edited

Loading