-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(ci): skip oclif shell probe in egg-bin #6014
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,13 @@ | ||
| #!/usr/bin/env node | ||
|
|
||
| import { execute } from '@oclif/core'; | ||
| function presetWindowsShell() { | ||
| if (process.platform !== 'win32' || process.env.SHELL) return; | ||
|
|
||
| const comspec = process.env.ComSpec ?? process.env.COMSPEC; | ||
| process.env.SHELL = comspec?.split(/[\\/]/).at(-1) || 'cmd.exe'; | ||
| } | ||
|
|
||
| presetWindowsShell(); | ||
| const { execute } = await import('@oclif/core'); | ||
|
|
||
| await execute({ dir: import.meta.dirname }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,13 @@ | ||
| #!/usr/bin/env node | ||
|
|
||
| import { execute } from '@oclif/core'; | ||
| function presetWindowsShell() { | ||
| if (process.platform !== 'win32' || process.env.SHELL) return; | ||
|
|
||
| const comspec = process.env.ComSpec ?? process.env.COMSPEC; | ||
| process.env.SHELL = comspec?.split(/[\\/]/).at(-1) || 'cmd.exe'; | ||
| } | ||
|
|
||
| presetWindowsShell(); | ||
| const { execute } = await import('@oclif/core'); | ||
|
|
||
| await execute({ dir: import.meta.url }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| --- | ||
| title: Egg-bin Windows shell probe hotspot | ||
| type: workflow | ||
| summary: How PR #6014 diagnosed hosted-Windows egg-bin startup slowness and why the final fix only presets SHELL. | ||
| source_files: | ||
| - tools/egg-bin/bin/run.js | ||
| - tools/egg-bin/test/fixtures/my-egg-bin/bin/run.js | ||
| - https://github.com/eggjs/egg/pull/6014 | ||
| - https://github.com/eggjs/egg/actions/runs/28316987317 | ||
| - https://github.com/eggjs/egg/actions/runs/28317525249 | ||
| updated_at: 2026-06-28 | ||
| status: active | ||
| --- | ||
|
|
||
| ## Finding | ||
|
|
||
| The Windows `test-egg-bin` hotspot in PR #6014 was repeated child-process CLI | ||
| startup, not Vitest sharding, globby, or `egg-bin test` config construction. | ||
| Hosted Windows runners started spawned `egg-bin` children without `SHELL`, which | ||
| made oclif synchronously probe the parent process shell through PowerShell/CIM. | ||
| Files such as `tools/egg-bin/test/commands/test.test.ts` amplify that cost | ||
| because they spawn many child `egg-bin` processes. | ||
|
|
||
| Inference: the hosted-runner slowdown looked like a long test file, but the | ||
| dominant repeated cost happened before the command's own test work. Temporary | ||
| CI-only timing showed normal `test` command globby/config work was tiny, while | ||
| pre-command oclif startup dominated. | ||
|
|
||
| ## Final Fix | ||
|
|
||
| The final code fix is deliberately small: | ||
|
|
||
| - in `tools/egg-bin/bin/run.js`, preset `process.env.SHELL` on Windows when it is | ||
| missing, deriving the shell name from `COMSPEC`/`ComSpec` or falling back to | ||
| `cmd.exe` | ||
| - do the same in `tools/egg-bin/test/fixtures/my-egg-bin/bin/run.js`, because the | ||
| custom CLI fixture has its own oclif entrypoint | ||
| - dynamically import `@oclif/core` after the preset, so oclif observes `SHELL` | ||
| before it initializes | ||
|
|
||
| The diagnostic instrumentation used during the investigation was removed from | ||
| the final PR. The CI workflow stays as one Windows `test-egg-bin` leg; sharding | ||
| is not needed after the shell-probe fix. | ||
|
|
||
| ## Evidence | ||
|
|
||
| Observed on CI while diagnosing PR #6014: | ||
|
|
||
| - Before the fix, `test/commands/test.test.ts` on Windows took about 1,077s. | ||
| - After the main entrypoint preset, `test/commands/test.test.ts` dropped to about | ||
| 42-48s. | ||
| - After applying the same preset to the custom CLI fixture, `test/my-egg-bin.test.ts` | ||
| dropped from about 299s to about 7.6s. | ||
| - After removing the diagnostic code and keeping the minimal fix, the single | ||
| Windows `test-egg-bin` job passed in about 3m06s; file-level timings included | ||
| `test/commands/test.test.ts` at 48.8s, `test/my-egg-bin.test.ts` at 8.5s, and | ||
| `test/commands/cov.test.ts` at 30.0s. | ||
|
|
||
| Remaining slow cases are real test/app startup work, mainly TypeScript fixture | ||
| cases around 9-10s, not repeated oclif shell probing. | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个差距也太大了吧。。。