-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Fix broken user and docker tests #42431
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
Changes from all commits
9978171
c63c118
28f25ab
da62a37
a83a17e
8824950
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,31 +68,35 @@ namespace Harness { | |
|
||
cwd = config.path ? path.join(cwd, config.path) : submoduleDir; | ||
} | ||
const npmVersionText = exec("npm", ["--version"], { cwd, stdio: "pipe" })?.trim(); | ||
const npmVersion = npmVersionText ? ts.Version.tryParse(npmVersionText.trim()) : undefined; | ||
const isV7OrLater = !!npmVersion && npmVersion.major >= 7; | ||
if (fs.existsSync(path.join(cwd, "package.json"))) { | ||
if (fs.existsSync(path.join(cwd, "package-lock.json"))) { | ||
fs.unlinkSync(path.join(cwd, "package-lock.json")); | ||
} | ||
if (fs.existsSync(path.join(cwd, "node_modules"))) { | ||
del.sync(path.join(cwd, "node_modules"), { force: true }); | ||
} | ||
exec("npm", ["i", "--ignore-scripts"], { cwd, timeout: timeout / 2 }); // NPM shouldn't take the entire timeout - if it takes a long time, it should be terminated and we should log the failure | ||
exec("npm", ["i", "--ignore-scripts", ...(isV7OrLater ? ["--legacy-peer-deps"] : [])], { cwd, timeout: timeout / 2 }); // NPM shouldn't take the entire timeout - if it takes a long time, it should be terminated and we should log the failure | ||
} | ||
const args = [path.join(IO.getWorkspaceRoot(), "built/local/tsc.js")]; | ||
if (types) { | ||
args.push("--types", types.join(",")); | ||
// Also actually install those types (for, eg, the js projects which need node) | ||
if (types.length) { | ||
exec("npm", ["i", ...types.map(t => `@types/${t}`), "--no-save", "--ignore-scripts"], { cwd: originalCwd, timeout: timeout / 2 }); // NPM shouldn't take the entire timeout - if it takes a long time, it should be terminated and we should log the failure | ||
exec("npm", ["i", ...types.map(t => `@types/${t}`), "--no-save", "--ignore-scripts", ...(isV7OrLater ? ["--legacy-peer-deps"] : [])], { cwd: originalCwd, timeout: timeout / 2 }); // NPM shouldn't take the entire timeout - if it takes a long time, it should be terminated and we should log the failure | ||
} | ||
} | ||
args.push("--noEmit"); | ||
Baseline.runBaseline(`${cls.kind()}/${directoryName}.log`, cls.report(cp.spawnSync(`node`, args, { cwd, timeout, shell: true }), cwd)); | ||
|
||
function exec(command: string, args: string[], options: { cwd: string, timeout?: number }): void { | ||
function exec(command: string, args: string[], options: { cwd: string, timeout?: number, stdio?: import("child_process").StdioOptions }): string | undefined { | ||
const res = cp.spawnSync(isWorker ? `${command} 2>&1` : command, args, { shell: true, stdio, ...options }); | ||
if (res.status !== 0) { | ||
throw new Error(`${command} ${args.join(" ")} for ${directoryName} failed: ${res.stdout && res.stdout.toString()}`); | ||
} | ||
return options.stdio === "pipe" ? res.stdout.toString("utf8") : undefined; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't actually use this returned value anywhere, do we? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm using it to test the npm version, since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahhh, ok |
||
} | ||
}); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
FROM node:current | ||
RUN npm i -g yarn --force | ||
RUN git clone https://github.com/prettier/prettier.git /prettier | ||
WORKDIR /prettier | ||
RUN git pull | ||
COPY --from=typescript/typescript /typescript/typescript-*.tgz /typescript.tgz | ||
RUN mkdir /typescript | ||
RUN tar -xzvf /typescript.tgz -C /typescript | ||
RUN yarn add typescript@/typescript.tgz | ||
RUN yarn | ||
ENTRYPOINT [ "yarn" ] | ||
# Build | ||
CMD [ "build" ] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, so the user test previously just applied a custom tsconfig to the contents of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd probably use their build, since that's what they're running. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ COPY --from=typescript/typescript /typescript/typescript-*.tgz /typescript.tgz | |
WORKDIR /vscode/build | ||
RUN yarn add typescript@/typescript.tgz | ||
WORKDIR /vscode/extensions | ||
RUN yarn add rimraf | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this working around a bug in vscode's own build? Since we clearly don't use rimraf here in the dockerfile. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does yarn have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, not according to the docs for |
||
RUN yarn add typescript@/typescript.tgz | ||
WORKDIR /vscode | ||
RUN yarn add typescript@/typescript.tgz | ||
|
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a compiler test for this crash fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm honestly not sure how to trigger it. It was triggered by something using typescript to parse just.config.ts in office-ui-fabric, and I'm not sure how deep the rabbit hole goes. At least its verified by the fact that office-ui-fabric is compiling again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Off the top of my head, I know pattern template literals are returned as their own constraint by computeBaseConstraint, and are.... Kinda recent? So maybe there's a situation with a template triggering it?