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

Getting rid of the x-pack tsconfig refs #90537

Merged
merged 2 commits into from
Feb 9, 2021

Conversation

kobelb
Copy link
Contributor

@kobelb kobelb commented Feb 6, 2021

This will yield no performance improvement. Based on my one-off test, prior to this change, it took 310 seconds to build the TS projects; and after the change, it took 310 seconds to build the TS projects.

@kobelb
Copy link
Contributor Author

kobelb commented Feb 6, 2021

/cc @tylersmalley @restrry

@tylersmalley
Copy link
Contributor

tylersmalley commented Feb 6, 2021

I put up #90529 after coming to the same conclusion.

@mshustov
Copy link
Contributor

mshustov commented Feb 8, 2021

hm... How did you test the changes? My testing algorithm:
master

  1. check out master branch, clean cache: time ./node_modules/.bin/tsc -b x-pack/tsconfig.refs.json tsconfig.refs.json --clean
  2. run build for both projects: time ./node_modules/.bin/tsc -b x-pack/tsconfig.refs.json tsconfig.refs.json
    time: real 9m7s
  3. check out this branch.
  4. merge master
  5. resolve conflict, list infra plugin in tsconfig.refs.json.
  6. run build for the single project: time ./node_modules/.bin/tsc -b tsconfig.refs.json
    time: real 4m40s

@tylersmalley
Copy link
Contributor

@restrry, not sure how you're seeing such an improvement. It's possible that is correct, or that there is additional caching we're missing like with Babel.

When I was testing, I was running git clean -fdx && yarn kbn bootstrap. I tried separating build_ts_refs from the rest of bootstrap, but everything else was extremely consistent.

@kobelb
Copy link
Contributor Author

kobelb commented Feb 9, 2021

@restrry on both branches, I did the following:

  1. Delete this line: https://github.com/elastic/kibana/blob/master/package.json#L60
  2. yarn kbn clean
  3. yarn kbn bootstrap
  4. time node ./scripts/build_ts_refs

@tylersmalley
Copy link
Contributor

@kobelb, I believe you removed kbn:bootstrap from package.json during your test?

@kobelb
Copy link
Contributor Author

kobelb commented Feb 9, 2021

@tylersmalley yup!

@mshustov
Copy link
Contributor

mshustov commented Feb 9, 2021

@tylersmalley does yarn kbn clean remove target folders?

@tylersmalley
Copy link
Contributor

@restrry, yeah. It should output those directory locations when you run it.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@kobelb kobelb marked this pull request as ready for review February 9, 2021 23:33
@kobelb kobelb requested a review from a team as a code owner February 9, 2021 23:33
@kobelb kobelb added release_note:skip Skip the PR/issue when compiling release notes v8.0.0 v7.12.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Feb 9, 2021
@kobelb kobelb merged commit 7627d35 into elastic:master Feb 9, 2021
@kobelb kobelb deleted the one-tsconfig-refs branch February 9, 2021 23:35
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 9, 2021
@kibanamachine
Copy link
Contributor

Backport result

{"level":"info","message":"POST https://api.github.com/graphql (status: 200)"}
{"level":"info","message":"POST https://api.github.com/graphql (status: 200)"}
{"meta":{"labels":["auto-backport","release_note:skip","v7.12.0","v8.0.0"],"branchLabelMapping":{"^v8.0.0$":"master","^v7.12.0$":"7.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"},"existingTargetPullRequests":[]},"level":"info","message":"Inputs when calculating target branches:"}
{"meta":["7.x"],"level":"info","message":"Target branches inferred from labels:"}
{"meta":{"killed":false,"code":2,"signal":null,"cmd":"git remote rm kibanamachine","stdout":"","stderr":"error: No such remote: 'kibanamachine'\n"},"level":"info","message":"exec error 'git remote rm kibanamachine':"}
{"meta":{"killed":false,"code":2,"signal":null,"cmd":"git remote rm elastic","stdout":"","stderr":"error: No such remote: 'elastic'\n"},"level":"info","message":"exec error 'git remote rm elastic':"}
{"level":"info","message":"Backporting [{\"sourceBranch\":\"master\",\"targetBranchesFromLabels\":[\"7.x\"],\"sha\":\"7627d35443a548037ec5cceb92763be9005e2590\",\"formattedMessage\":\"Getting rid of the x-pack tsconfig refs (#90537)\",\"originalMessage\":\"Getting rid of the x-pack tsconfig refs (#90537)\",\"pullNumber\":90537,\"existingTargetPullRequests\":[]}] to 7.x"}

Backporting to 7.x:
{"level":"info","message":"Backporting via filesystem"}
{"level":"info","message":"Creating PR with title: \"[7.x] Getting rid of the x-pack tsconfig refs (#90537)\". kibanamachine:backport/7.x/pr-90537 -> 7.x"}
{"level":"info","message":"POST /repos/elastic/kibana/pulls - 201 in 1126ms"}
{"level":"info","message":"Adding assignees to #90875: kobelb"}
{"level":"info","message":"POST /repos/elastic/kibana/issues/90875/assignees - 201 in 538ms"}
{"level":"info","message":"Adding labels: backport"}
{"level":"info","message":"POST /repos/elastic/kibana/issues/90875/labels - 200 in 401ms"}
View pull request: https://github.com/elastic/kibana/pull/90875

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants