Skip to content

Getting rid of the x-pack tsconfig refs#90537

Merged
kobelb merged 2 commits intoelastic:masterfrom
kobelb:one-tsconfig-refs
Feb 9, 2021
Merged

Getting rid of the x-pack tsconfig refs#90537
kobelb merged 2 commits intoelastic:masterfrom
kobelb:one-tsconfig-refs

Conversation

@kobelb
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

kobelb commented Feb 6, 2021

/cc @tylersmalley @restrry

@tylersmalley
Copy link
Copy Markdown
Member

tylersmalley commented Feb 6, 2021 •
edited
Loading

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

@tylersmalley tylersmalley mentioned this pull request Feb 6, 2021
Closed
@mshustov
Copy link
Copy Markdown
Contributor

mshustov commented Feb 8, 2021 •
edited
Loading

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
Copy Markdown
Member

tylersmalley commented Feb 9, 2021

@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
Copy Markdown
Contributor Author

kobelb commented Feb 9, 2021 •
edited
Loading

@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
Copy Markdown
Member

tylersmalley commented Feb 9, 2021

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

@kobelb
Copy link
Copy Markdown
Contributor Author

kobelb commented Feb 9, 2021

@tylersmalley yup!

@mshustov
Copy link
Copy Markdown
Contributor

mshustov commented Feb 9, 2021

@tylersmalley does yarn kbn clean remove target folders?

@tylersmalley
Copy link
Copy Markdown
Member

tylersmalley commented Feb 9, 2021

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

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Feb 9, 2021

💚 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
@kobelb @kibanamachine
Getting rid of the x-pack tsconfig refs (elastic#90537)
78b6d30
@kibanamachine kibanamachine mentioned this pull request Feb 9, 2021
Merged
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Feb 9, 2021

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

@kobelb kobelb mentioned this pull request Feb 10, 2021
Merged
kobelb added a commit that referenced this pull request Feb 10, 2021
@kibanamachine @kobelb
Getting rid of the x-pack tsconfig refs (#90537) (#90875)
c6535a8 
Co-authored-by: Brandon Kobel <brandon.kobel@elastic.co>
@tylersmalley tylersmalley mentioned this pull request Feb 10, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tylersmalley tylersmalley tylersmalley approved these changes

+1 more reviewer

@mshustov mshustov mshustov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kobelb @tylersmalley @mshustov @kibanamachine