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

Provides way to exclude TS project references build from bootstrap #91209

Closed
wants to merge 4 commits into from

Conversation

tylersmalley
Copy link
Contributor

@tylersmalley tylersmalley commented Feb 11, 2021

Not everyone needs/wants the Typescript project references to be built, resulting in a lot of time for needless waiting.

This provides an environment variable they could set, and if needed manually run node script/build_ts_refs

No longer building TS references in bootstrap on CI will reduce overall CI time by ~8 minutes.

Not everyone needs/wants the Typescript project references to be build,
resulting in a lot of time for needless waiting.

This provides an environment variable they could set, and if needed
manually run `node script/build_ts_refs`

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
@tylersmalley tylersmalley added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team v8.0.0 backport:skip This commit does not require backporting v7.12.0 labels Feb 11, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@tylersmalley tylersmalley added release_note:skip Skip the PR/issue when compiling release notes and removed backport:skip This commit does not require backporting labels Feb 11, 2021
@tylersmalley
Copy link
Contributor Author

Actually might just move this check to the kbn:bootstrap script in package.json

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
@mistic
Copy link
Member

mistic commented Feb 11, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

kibanamachine commented Feb 11, 2021

💔 Build Failed

Failed CI Steps


Test Failures

Kibana Pipeline / jest / Jest Tests.packages/kbn-utils/src/path.Default path finder should find a data directory

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches


Stack Trace

Error: expect(received).not.toThrow()

Error name:    "Error"
Error message: "ENOENT: no such file or directory, access '/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/data'"

      18 |   it('should find a data directory', () => {
      19 |     const dataPath = getDataPath();
    > 20 |     expect(() => accessSync(dataPath, constants.R_OK)).not.toThrow();
         |                  ^
      21 |   });
      22 | 
      23 |   it('should find a config directory', () => {

      at packages/kbn-utils/src/path/index.test.ts:20:18
      at Object.<anonymous> (node_modules/expect/build/toThrowMatchers.js:83:11)
      at Object.toThrow (node_modules/expect/build/index.js:338:21)
      at Object.<anonymous> (packages/kbn-utils/src/path/index.test.ts:20:60)
    at Object.<anonymous> (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/packages/kbn-utils/src/path/index.test.ts:20:60)
    at Promise.then.completed (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/utils.js:276:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/utils.js:216:10)
    at _callCircusTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:212:40)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at _runTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:149:3)
    at _runTestsForDescribeBlock (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:63:9)
    at _runTestsForDescribeBlock (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:57:9)
    at run (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:25:3)
    at runAndTransformResultsToJestFormat (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:176:21)
    at jestAdapter (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:109:19)
    at runTestInternal (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/runTest.js:380:16)
    at runTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/runTest.js:472:34)
    at Object.worker (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/testWorker.js:133:12)

Metrics [docs]

✅ unchanged

History

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

@Zacqary
Copy link
Contributor

Zacqary commented Feb 11, 2021

Is there a reason we'd want to build TS references in bootstrap in a context outside of the CI? I imagine most Kibana devs would need to enable this environment variable to speed up dev time (i.e. working on something with unfinished type definitions, need to merge master, have to run bootstrap to get Kibana to run).

Does it make more sense to have the CI opt-in to building references with an environment variable, rather than having an end user opt-out? Or is there a use case I'm not understanding?

@tylersmalley
Copy link
Contributor Author

tylersmalley commented Feb 11, 2021

@Zacqary, the building of TS references is to address Typescript being otherwise unusable in any IDE, not in CI. The time this task has taken has increased greatly over the past couple of weeks as teams have migrated to project references. I agree that not everyone needs or wants these types to be generated every time, and we need to reach a consensus on that.

Some options:

  • Make it opt-out, which is what we are doing here.
  • No longer build Typescript references by default during bootstrap, and instead let the developer run node scripts/build_ts_refs when they feel it's necessary.
  • Move to building the references asynchronously, where the bootstrap will kick off this task. This is not completely strait-forward, as we need to figure out how to communicate this to the user and ensure we're not running multiple of these.
  • Attempt to improve caching (Spencer has an experimental PR [ts/build-refs] implement experimental remote cache #91012)
  • Build projects using Bazel (minimum 2+ months out from being able to move forward with this Migrate Kibana plugins from TS project references into Bazel #91056)
  • Address performance issues with current Typescript usage. This is hard, and not currently documented. But you can get a trace:
    • rm -rf src/core/target && ./node_modules/.bin/tsc -b src/core/tsconfig.json --extendedDiagnostics --generateTrace trace_output
    • Open chrome://tracing/ in Chrome
    • Load trace.1.json
    • The pos references a line in types.1.json

More information on project references: https://github.com/elastic/kibana/blob/master/docs/developer/best-practices/typescript.asciidoc#project-references

@tylersmalley
Copy link
Contributor Author

tylersmalley commented Feb 12, 2021

Closing this in favor of not tying the IDE optimization to the bootstrap task by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants