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

build: add check for lib size #24712

Merged
merged 7 commits into from
Jun 8, 2018
Merged

build: add check for lib size #24712

merged 7 commits into from
Jun 8, 2018

Conversation

styfle
Copy link
Contributor

@styfle styfle commented Jun 6, 2018

This adds a check to make sure the lib size doesn't increase more than 10% after the "LKG" step.

This change was suggested by mhegazy in this comment.

Related #24282

@msftclas
Copy link

msftclas commented Jun 6, 2018

CLA assistant check
All CLA requirements met.

const { lstatSync, readdirSync } = require("fs");
const { join } = require("path");
const { promisify } = require("util");
const execFile = promisify(require("child_process").execFile);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if is needed, but promisify not exists in node 6.

Copy link
Member

Choose a reason for hiding this comment

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

Also, execFile isn't used?

Copy link
Contributor Author

@styfle styfle Jun 7, 2018

Choose a reason for hiding this comment

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

Oops, this was a bad copypasta on my part 😟
I removed it 🔧

Gulpfile.js Outdated
@@ -12,6 +12,7 @@ const clone = require("gulp-clone");
const newer = require("gulp-newer");
const tsc = require("gulp-typescript");
const tsc_oop = require("./scripts/build/gulp-typescript-oop");
const { getDirSize } = require("./scripts/build/get-dir-size");
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, do not use kebab-case, instead use camel case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I changed it 🔧

@mhegazy
Copy link
Contributor

mhegazy commented Jun 6, 2018

we also need to make a similar change to jakefile.js

Gulpfile.js Outdated
const seq = runSequence("LKGInternal", "VerifyLKG");
const sizeAfter = getDirSize(lib);
if (sizeAfter > (sizeBefore * 1.10)) {
throw new Error("The lib folder increased by 10% or more. This likely indicates a bug.");
Copy link
Member

Choose a reason for hiding this comment

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

How do we want to confirm "no, this really is what we meant to do"?

Copy link
Contributor

Choose a reason for hiding this comment

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

u can manually check in the LKG. this happens after the LKG has been built and updated.

@styfle
Copy link
Contributor Author

styfle commented Jun 7, 2018

@mhegazy Which line number in the Jakefile.js?
I'm guessing here line L659 but I'm not sure.

https:/Microsoft/TypeScript/blob/8b77b13166df82f32d1edbc121ba42c46006a3a0/Jakefile.js#L657-L660

@mhegazy
Copy link
Contributor

mhegazy commented Jun 7, 2018

I would do it after we have copied all expected files.

@styfle
Copy link
Contributor Author

styfle commented Jun 8, 2018

@mhegazy How does that look?

@mhegazy mhegazy merged commit 7b2e092 into microsoft:master Jun 8, 2018
@mhegazy
Copy link
Contributor

mhegazy commented Jun 8, 2018

thanks!

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.

5 participants