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

[type-summarizer] always use normalized paths, fix windows compat #127055

Merged
merged 3 commits into from
Mar 7, 2022

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Mar 7, 2022

There were a couple issues with type-summarizer that only showed up when running on windows:

  • TS uses normalized paths for sourceFile.fileName but Path.relative() and Path.resolve() use the system path separator causing a mix of / and \\ in the paths.
  • A bazel bug improperly escapes JSON passed in the args of the command line
  • Bazel doesn't create real sandboxes on windows, so test files were in the inputs that were built by TypeScript causing the project build to fail

To fix these issues a workaround for the JSON arg was implemented and @mistic is working on a fix for upstream. Additionally, a new Path wrapper was created which will always return normalized paths, only calling normalizePath when necessary and providing a few tweaks to the API to make sure that we're always dealing with normalized versions of paths. Additionally, the test directory was moved into the src and excludes were removed so that the build behaves the same on windows and other operating systems. It's quite unfortunate that we have to do this, but Windows is a tricky platform to target.

@spalger spalger added Team:Operations Team label for Operations Team release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.2.0 labels Mar 7, 2022
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
@kbn/type-summarizer 1 0 -1

Total ESLint disabled count

id before after diff
@kbn/type-summarizer 3 2 -1

History

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

@spalger spalger marked this pull request as ready for review March 7, 2022 21:24
@elasticmachine
Copy link
Contributor

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

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

Windows LGTM

@spalger spalger merged commit be7ff2c into elastic:main Mar 7, 2022
@spalger spalger deleted the fix/type-summarzer-windows-compat branch March 7, 2022 22:04
jloleysens added a commit to jloleysens/kibana that referenced this pull request Mar 8, 2022
…ed-unexpectedly-error

* 'main' of github.com:elastic/kibana: (46 commits)
  Fix copy and pasted renderer user_name test (elastic#126663)
  [Gauge] Vis editors gauge legacy percent mode. (elastic#126318)
  Remove all cases related code from timelines (elastic#127003)
  Hide Enterprise search panel when no nodes are present (elastic#127100)
  [Lens] Fixed flakiness on runtime fields' appearance on the list (elastic#126945)
  [Security Solution][Lists] - Add missing privileges callout to exception lists page (elastic#126874)
  [Security Solution][Lists] - Updates exception flyout edit error messages (elastic#126875)
  [Security Solution][Rules] - Remove rule selection for read only users (elastic#126827)
  Fix session cleanup test (elastic#126966)
  [ftr] implement support for accessing ES through CCS (elastic#126547)
  [type-summarizer] always use normalized paths, fix windows compat (elastic#127055)
  Revert "[ci] Configure hourly pipeline for a small spot instance trial (elastic#126824)"
  Revert "[CI] Expand spot instance trial a bit (elastic#126928)"
  [Alerting] Adding functional tests for alerting and actions telemetry (elastic#126528)
  [Telemetry] Check permissions when requesting telemetry (elastic#126238)
  Don't submit empty seed_urls or sitemap_urls when making a partial crawl request (elastic#126972)
  Remove License Requirement for Enterprise Search App Search Meta Engines (elastic#127046)
  [ML] Adding data recognizer module config cache (elastic#126338)
  skip flaky suite (elastic#126027)
  [Reporting] Improve error logging for rescheduled jobs (elastic#126737)
  ...

# Conflicts:
#	x-pack/plugins/reporting/server/core.ts
#	x-pack/plugins/reporting/server/lib/tasks/execute_report.ts
jloleysens added a commit to jloleysens/kibana that referenced this pull request Mar 8, 2022
…re-browser-errors

* 'main' of github.com:elastic/kibana: (46 commits)
  Fix copy and pasted renderer user_name test (elastic#126663)
  [Gauge] Vis editors gauge legacy percent mode. (elastic#126318)
  Remove all cases related code from timelines (elastic#127003)
  Hide Enterprise search panel when no nodes are present (elastic#127100)
  [Lens] Fixed flakiness on runtime fields' appearance on the list (elastic#126945)
  [Security Solution][Lists] - Add missing privileges callout to exception lists page (elastic#126874)
  [Security Solution][Lists] - Updates exception flyout edit error messages (elastic#126875)
  [Security Solution][Rules] - Remove rule selection for read only users (elastic#126827)
  Fix session cleanup test (elastic#126966)
  [ftr] implement support for accessing ES through CCS (elastic#126547)
  [type-summarizer] always use normalized paths, fix windows compat (elastic#127055)
  Revert "[ci] Configure hourly pipeline for a small spot instance trial (elastic#126824)"
  Revert "[CI] Expand spot instance trial a bit (elastic#126928)"
  [Alerting] Adding functional tests for alerting and actions telemetry (elastic#126528)
  [Telemetry] Check permissions when requesting telemetry (elastic#126238)
  Don't submit empty seed_urls or sitemap_urls when making a partial crawl request (elastic#126972)
  Remove License Requirement for Enterprise Search App Search Meta Engines (elastic#127046)
  [ML] Adding data recognizer module config cache (elastic#126338)
  skip flaky suite (elastic#126027)
  [Reporting] Improve error logging for rescheduled jobs (elastic#126737)
  ...

# Conflicts:
#	x-pack/plugins/reporting/server/lib/tasks/execute_report.ts
lucasfcosta pushed a commit to lucasfcosta/kibana that referenced this pull request Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Operations Team label for Operations Team v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants