feat: compile CI to a binary with bun #3101
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| @@ -63,61 +64,61 @@ | |||
| }, | |||
| "author": "genkit", | |||
| "license": "Apache-2.0", | |||
| "types": "./lib/types/types/index.d.ts", | |||
| "types": "./lib/types/src/types/index.d.ts", | |||
There was a problem hiding this comment.
this is required as the package.json is included in the build now, which lives above src
cabljac
force-pushed
the
@invertase/cli-binary
branch
3 times, most recently
from
e72b3b3 to
a22d785
Compare
cabljac
force-pushed
the
@invertase/cli-binary
branch
2 times, most recently
from
036207a to
b2afcc7
Compare
cabljac
force-pushed
the
@invertase/cli-binary
branch
from
b2afcc7 to
9e8229c
Compare
There was a problem hiding this comment.
Pull Request Overview
This draft PR introduces changes to enable compiling the CLI into binaries using Bun and integrates a new CI workflow to build and test these binaries on multiple platforms. Key changes include:
- Updates to tsconfig and package.json files to support Bun-specific compiler options and type definitions.
- Refactoring of error handling utilities and removal of legacy argument parsing in favor of Commander commands.
- Addition of a new GitHub Actions workflow for building and testing CLI binaries.
Reviewed Changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| genkit-tools/common/tsconfig.json | Added "resolveJsonModule" and a trailing comma for proper JSON configuration. |
| genkit-tools/common/src/utils/utils.ts | Refactored error handling to use the new isConnectionRefusedError helper. |
| genkit-tools/common/src/utils/package.ts | Simplified package import by using direct JSON import instead of file system reads. |
| genkit-tools/common/src/utils/errors.ts | Added utility functions to normalize error handling across Node.js and Bun environments. |
| genkit-tools/common/package.json | Updated types paths in exports and added "bun-types" as a dependency. |
| genkit-tools/cli/tsconfig.json | Included "bun-types" and enabled "resolveJsonModule" for improved module resolution. |
| genkit-tools/cli/src/utils/server-harness.ts | Replaced legacy argument parsing with a Commander command for starting the server and redirecting output. |
| genkit-tools/cli/src/commands/ui-start.ts | Updated to spawn the process using process.execPath along with the new CLI command interface. |
| genkit-tools/cli/src/cli.ts | Integrated the new uiStartServer command based on process arguments. |
| genkit-tools/cli/package.json | Introduced a new "compile:bun" script and updated Bun-related dependencies. |
| .github/workflows/build-cli-binaries.yml | Added a CI workflow to compile CLI binaries across various OS and architecture targets using Bun. |
Files not reviewed (1)
- genkit-tools/pnpm-lock.yaml: Language not supported
cabljac
force-pushed
the
@invertase/cli-binary
branch
from
648d085 to
8394ced
Compare
| on: | ||
| push: | ||
| branches: | ||
| - '@invertase/cli-binary' |
Co-authored-by: Elliot Hesp <[email protected]>
Co-authored-by: Elliot Hesp <[email protected]>
Co-authored-by: Elliot Hesp <[email protected]>
Co-authored-by: Elliot Hesp <[email protected]>
| const child = spawn( | ||
| 'node', | ||
| [serverPath, port.toString(), serversDir + '/devui.log'], | ||
| process.execPath, |
There was a problem hiding this comment.
does this end up being the bundled bun binary?
There was a problem hiding this comment.
Oh, I see the other comment... I need to think about how I feel about it... probably fine....
There was a problem hiding this comment.
I honestly don't remember why we start the web-server in a separate process...
It might've had something to do with logging... being able to write dev ui server logs into a separate log file.
Which may not be actually working at the moment.
There might be an opportunity to change this and just start the server as part of the cli process itself. But it's a bigger/riskier change.
I'm leaning towards saying that this approach is OK.
There was a problem hiding this comment.
ui:start was an async command that would start the server (in a separate process so that it would continue running) and then the command would end. We've since moved back to a sync command but this function is reused.
There was a problem hiding this comment.
I'm not referring the command itself, but the underlying web-server being run from the harness in a separate process.
There was a problem hiding this comment.
I don't think I understand what you mean. The harness is started in a separate process for the reasons I explained and startServer() runs the web server in that process. There's no way to spawn a process without an entry point hence the harness's existence but without ui:start it could be simplified, having the server be started in a separate thread in the command's process.
There was a problem hiding this comment.
Ah, now I get it. Sorry, took me a sec to connect the dots. Yes, the refactoring is unblocked for sure.
| @@ -0,0 +1,349 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
@pavelgj this has been copied and modified from the firebase.tools source - could you let us know if this needs correct attribution to anyone and if so, we can add them to the commit/pr?
There was a problem hiding this comment.
No, no attribution is required. It's all "copyright google". If it were 3P it'd be different.
This is a draft PR to:
bunbunChecklist (if applicable):