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

Fix typecheck foundations #167060

Merged
merged 23 commits into from
Sep 25, 2023
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
00c9138
chore(fix-typecheck): Add typecheck command, as a reference commit
delanni Sep 20, 2023
3b25ef5
chore(typecheck): Throw an error when a SIGABRT is seen - this happen…
delanni Sep 21, 2023
5bcd214
chore(typecheck): try increasing heap size to avoid OOM
delanni Sep 21, 2023
e1f2044
chore(typecheck): Set up alternative typecheck based on a flag: ci:ha…
delanni Sep 22, 2023
0c98ae9
Add watson/type_check_commits
Ikuni17 Sep 22, 2023
5e85fa7
Use script in bk
Ikuni17 Sep 22, 2023
f86af91
fix yaml whitespace
Ikuni17 Sep 22, 2023
0d039ee
Merge remote-tracking branch 'upstream/main' into fix-typecheck-found…
Ikuni17 Sep 22, 2023
39137b3
Temp disable flags
Ikuni17 Sep 22, 2023
b58f57b
Fix whitespace
Ikuni17 Sep 22, 2023
4dcf341
Add bootstrap
Ikuni17 Sep 22, 2023
e54c237
Only bootstrap in CI
Ikuni17 Sep 22, 2023
62ce147
Skip .buildkite dir
Ikuni17 Sep 22, 2023
db60557
Disable API docs type check
Ikuni17 Sep 22, 2023
4ef34f1
Rename typecheck package.json script
watson Sep 23, 2023
0ccc66b
Merge branch 'main' into fix-typecheck-foundations
Sep 23, 2023
a957e48
Update packages/kbn-dev-proc-runner/src/proc_runner.ts
delanni Sep 25, 2023
31e2ccc
Merge branch 'main' into fix-typecheck-foundations
Sep 25, 2023
d057ecf
chore(typecheck): Listen to subprocess termination by promise rejection
delanni Sep 25, 2023
f44ea24
Merge branch 'main' into fix-typecheck-foundations
kibanamachine Sep 25, 2023
7b06b81
Merge branch 'main' into fix-typecheck-foundations
delanni Sep 25, 2023
57ed7b6
Merge branch 'main' into fix-typecheck-foundations
kibanamachine Sep 25, 2023
0c66fc0
Merge branch 'main' into fix-typecheck-foundations
kibanamachine Sep 25, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions .buildkite/pipelines/pull_request/base.yml
Original file line number Diff line number Diff line change
Expand Up @@ -136,12 +136,13 @@ steps:
- exit_status: '-1'
limit: 3

- command: .buildkite/scripts/steps/check_types.sh
label: 'Check Types'
agents:
queue: n2-16-spot
timeout_in_minutes: 60
retry:
automatic:
- exit_status: '-1'
limit: 3
# TODO: Enable in #166813 after fixing types
# - command: .buildkite/scripts/steps/check_types.sh
# label: 'Check Types'
# agents:
# queue: n2-16-spot
# timeout_in_minutes: 60
# retry:
# automatic:
# - exit_status: '-1'
# limit: 3
10 changes: 10 additions & 0 deletions .buildkite/pipelines/pull_request/type_check.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
steps:
- command: .buildkite/scripts/steps/check_types.sh
label: 'Check Types'
agents:
queue: n2-16-spot
timeout_in_minutes: 60
retry:
automatic:
- exit_status: '-1'
limit: 3
10 changes: 10 additions & 0 deletions .buildkite/pipelines/pull_request/type_check_selective.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
steps:
- command: .buildkite/scripts/steps/check_types_commits.sh
label: 'Check Types Commit Diff'
agents:
queue: n2-16-spot
timeout_in_minutes: 60
retry:
automatic:
- exit_status: '-1'
limit: 3
6 changes: 6 additions & 0 deletions .buildkite/scripts/pipelines/pull_request/pipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,12 @@ const uploadPipeline = (pipelineContent: string | object) => {
pipeline.push(getPipeline('.buildkite/pipelines/pull_request/kbn_handlebars.yml'));
}

if (GITHUB_PR_LABELS.includes('ci:hard-typecheck')) {
pipeline.push(getPipeline('.buildkite/pipelines/pull_request/type_check.yml'));
} else {
pipeline.push(getPipeline('.buildkite/pipelines/pull_request/type_check_selective.yml'));
}

if (
(await doAnyChangesMatch([
/^src\/plugins\/controls/,
Expand Down
5 changes: 3 additions & 2 deletions .buildkite/scripts/steps/build_api_docs.sh
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@ set -euo pipefail

.buildkite/scripts/bootstrap.sh

echo "--- Run scripts/type_check to ensure that all build available"
node scripts/type_check
# TODO: Enable in #166813 after fixing types
# echo "--- Run scripts/type_check to ensure that all build available"
# node scripts/type_check

echo "--- Build API Docs"
node --max-old-space-size=12000 scripts/build_api_docs
Expand Down
114 changes: 114 additions & 0 deletions .buildkite/scripts/steps/check_types_commits.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
#!/usr/bin/env bash

set -euo pipefail


if [[ "${CI-}" == "true" ]]; then
.buildkite/scripts/bootstrap.sh

sha1="${GITHUB_PR_TARGET_BRANCH-}"
sha2="${GITHUB_PR_TRIGGERED_SHA-}"
else
# Script take between 0 and 2 arguments representing two commit SHA's:
# If 0, it will diff HEAD and HEAD^
# If 1 (SHA1), it will diff SHA1 and SHA1^
# If 2 (SHA1, SHA2), it will diff SHA1 and SHA2
sha1="${1-HEAD}"
sha2="${2-$sha1^}"
fi

uniq_dirs=()
uniq_tsconfigs=()

echo "Detecting files changed between $sha1 and $sha2..."

files=($(git diff --name-only $sha1 $sha2))

add_dir () {
new_dir=$1

if [ ${#uniq_dirs[@]} -gt 0 ]; then
for dir in "${uniq_dirs[@]}"
do
if [[ "$new_dir" == "$dir" ]]; then
return
fi
done
fi

uniq_dirs+=($new_dir)
}

add_tsconfig () {
new_tsconfig=$1

if [ ${#uniq_tsconfigs[@]} -gt 0 ]; then
for tsconfig in "${uniq_tsconfigs[@]}"
do
if [[ "$new_tsconfig" == "$tsconfig" ]]; then
return
fi
done
fi

echo " $new_tsconfig"
uniq_tsconfigs+=($new_tsconfig)
}

contains_tsconfig () {
dir=$1
tsconfig="$dir/tsconfig.json"
if [ -f "$tsconfig" ]; then
true
else
false
fi
}

find_tsconfig () {
dir=$1

if [[ "$dir" == "." ]]; then
return
fi

if contains_tsconfig $dir; then
add_tsconfig "$dir/tsconfig.json"
else
find_tsconfig $(dirname -- "$dir")
fi
}

if [ ${#files[@]} -eq 0 ]; then
echo "No files found!"
exit
fi

for file in "${files[@]}"
do
dir=$(dirname -- "$file")

# Ignore buildkite dir because it traverses many kbn packages and emits incorrect results
if [[ "$dir" != .buildkite* ]]; then
add_dir $dir
fi
done

echo "Looking for related tsconfig.json files..."

for dir in "${uniq_dirs[@]}"
do
find_tsconfig $dir
done

if [ ${#uniq_tsconfigs[@]} -eq 0 ]; then
echo "No tsconfig.json files found for changes in $sha1 $sha2"
exit
fi

echo "Running scripts/type_check for each found tsconfig.json file..."

for tsconfig in "${uniq_tsconfigs[@]}"
do
node scripts/type_check --project $tsconfig
done
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@
"test:ftr:runner": "node scripts/functional_test_runner",
"test:ftr:server": "node scripts/functional_tests_server",
"test:jest": "node scripts/jest",
"test:jest_integration": "node scripts/jest_integration"
"test:jest_integration": "node scripts/jest_integration",
"test:type_check": "node scripts/type_check"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

why did you rename the script?
For me test:type_check would mean run typechecks on the test code, or similar - this is not test-related, but typechecking any code

},
"repository": {
"type": "git",
Expand Down
15 changes: 11 additions & 4 deletions packages/kbn-dev-proc-runner/src/proc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,19 +87,26 @@ export function startProc(name: string, options: ProcOptions, log: ToolingLog) {

const outcome$: Rx.Observable<number | null> = Rx.race(
// observe first exit event
Rx.fromEvent<[number]>(childProcess, 'exit').pipe(
Rx.fromEvent<[number, string]>(childProcess, 'exit').pipe(
take(1),
map(([code]) => {
map(([code, signal]) => {
if (stopCalled) {
return null;
}
// JVM exits with 143 on SIGTERM and 130 on SIGINT, dont' treat then as errors

// JVM exits with 143 on SIGTERM and 130 on SIGINT, don't treat them as errors
if (code > 0 && !(code === 143 || code === 130)) {
throw createFailError(`[${name}] exited with code ${code}`, {
exitCode: code,
});
}

// A stop signal of SIGABRT indicates a self-inflicted app crash,
// then we can't rely on an exit code, because it's not passed
if (signal === 'SIGABRT') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% confident we can trust that there will not be other signals that might indicate a crash. We don't have to do that in this PR, but in the future I'd like to simplify this code so that it just listens for the rejected promise instead of trying to be so clever and possibly missing some things

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, here's a fix: d057ecf

throw createFailError(`[${name}] aborted ${code ? `with code: ${code}` : ''}`);
}

return code;
})
),
Expand Down Expand Up @@ -132,7 +139,7 @@ export function startProc(name: string, options: ProcOptions, log: ToolingLog) {
share()
);

const outcomePromise = Rx.merge(lines$.pipe(ignoreElements()), outcome$).toPromise();
const outcomePromise = Rx.firstValueFrom(Rx.merge(lines$.pipe(ignoreElements()), outcome$));

async function stop(signal: NodeJS.Signals) {
if (stopCalled) {
Expand Down
5 changes: 4 additions & 1 deletion packages/kbn-dev-proc-runner/src/proc_runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,13 @@ export class ProcRunner {
);
}

if (wait === true) {
if (wait) {
delanni marked this conversation as resolved.
Show resolved Hide resolved
// wait for process to complete
await proc.outcomePromise;
}
} catch (e) {
this.log.error(e);
throw e;
} finally {
// while the procRunner closes promises will resolve/reject because
// processes and stopping, but consumers of run() shouldn't have to
Expand Down
3 changes: 3 additions & 0 deletions packages/kbn-ts-type-check-cli/run_type_check_cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,9 @@ run(
'--pretty',
...(flagsReader.boolean('verbose') ? ['--verbose'] : []),
],
env: {
NODE_OPTIONS: '--max-old-space-size=8192',
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

},
cwd: REPO_ROOT,
wait: true,
});
Expand Down