-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix(tui): worker path resolution in dev mode #3778
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
Conversation
|
This works as compiled standalone executable and for dev: diff --git a/packages/opencode/script/build.ts b/packages/opencode/script/build.ts
index 1d3a3fac..4ce8bfba 100755
--- a/packages/opencode/script/build.ts
+++ b/packages/opencode/script/build.ts
@@ -41,7 +41,9 @@ for (const [os, arch] of targets) {
const opentui = `@opentui/core-${os === "windows" ? "win32" : os}-${arch.replace("-baseline", "")}`
await $`mkdir -p ../../node_modules/${opentui}`
- await $`npm pack ${opentui}@${pkg.dependencies["@opentui/core"]}`.cwd(path.join(dir, "../../node_modules"))
+ await $`npm pack ${opentui}@${pkg.dependencies["@opentui/core"]}`.cwd(
+ path.join(dir, "../../node_modules"),
+ )
await $`tar -xf ../../node_modules/${opentui.replace("@opentui/", "opentui-")}-*.tgz -C ../../node_modules/${opentui} --strip-components=1`
const watcher = `@parcel/watcher-${os === "windows" ? "win32" : os}-${arch.replace("-baseline", "")}${os === "linux" ? "-glibc" : ""}`
@@ -49,7 +51,11 @@ for (const [os, arch] of targets) {
await $`npm pack ${watcher}`.cwd(path.join(dir, "../../node_modules")).quiet()
await $`tar -xf ../../node_modules/${watcher.replace("@parcel/", "parcel-")}-*.tgz -C ../../node_modules/${watcher} --strip-components=1`
- const parserWorker = fs.realpathSync(path.resolve(dir, "./node_modules/@opentui/core/parser.worker.js"))
+ const parserWorker = fs.realpathSync(
+ path.resolve(dir, "./node_modules/@opentui/core/parser.worker.js"),
+ )
+ const workerPath = "./src/cli/cmd/tui/worker.ts"
+
await Bun.build({
conditions: ["browser"],
tsconfig: "./tsconfig.json",
@@ -61,10 +67,11 @@ for (const [os, arch] of targets) {
execArgv: [`--user-agent=opencode/${Script.version}`, `--env-file=""`, `--`],
windows: {},
},
- entrypoints: ["./src/index.ts", parserWorker, "./src/cli/cmd/tui/worker.ts"],
+ entrypoints: ["./src/index.ts", parserWorker, workerPath],
define: {
OPENCODE_VERSION: `'${Script.version}'`,
OTUI_TREE_SITTER_WORKER_PATH: "/$bunfs/root/" + path.relative(dir, parserWorker),
+ OPENCODE_WORKER_PATH: workerPath,
OPENCODE_CHANNEL: `'${Script.channel}'`,
},
})
diff --git a/packages/opencode/src/cli/cmd/tui/thread.ts b/packages/opencode/src/cli/cmd/tui/thread.ts
index 3fd998bc..05b4e9a4 100644
--- a/packages/opencode/src/cli/cmd/tui/thread.ts
+++ b/packages/opencode/src/cli/cmd/tui/thread.ts
@@ -8,6 +8,10 @@ import { bootstrap } from "@/cli/bootstrap"
import path from "path"
import { UI } from "@/cli/ui"
+declare global {
+ const OPENCODE_WORKER_PATH: string
+}
+
export const TuiThreadCommand = cmd({
command: "$0 [project]",
describe: "start opencode tui",
@@ -55,7 +59,11 @@ export const TuiThreadCommand = cmd({
// Resolve relative paths against PWD to preserve behavior when using --cwd flag
const baseCwd = process.env.PWD ?? process.cwd()
const cwd = args.project ? path.resolve(baseCwd, args.project) : process.cwd()
- const workerPath = new URL("./worker.ts", import.meta.url).href
+ let workerPath = new URL("./worker.ts", import.meta.url).href
+
+ if (typeof OPENCODE_WORKER_PATH !== "undefined") {
+ workerPath = OPENCODE_WORKER_PATH
+ }
try {
process.chdir(cwd)
@@ -63,6 +71,7 @@ export const TuiThreadCommand = cmd({
UI.error("Failed to change directory to " + cwd)
return
}
+
await bootstrap(cwd, async () => {
upgrade()
|
07858eb to
b2476a9
Compare
|
@kommander thanks I updated the PR. it works for me, I tested :) |
b2476a9 to
beb22cb
Compare
6af2fa7 to
d3566d3
Compare
33efc8d to
1352970
Compare
| // Resolve relative paths against PWD to preserve behavior when using --cwd flag | ||
| const baseCwd = process.env.PWD ?? process.cwd() | ||
| const cwd = args.project ? path.resolve(baseCwd, args.project) : process.cwd() | ||
| let workerPath = new URL("./worker.ts", import.meta.url).href |
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.
Theoretically, Worker class is able to handle plain URL object. But I guess you hit a bug in bun that is not yet fixed.
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.
I think I have fixed this with 646f4b6
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.
Since you're on it, you should really mention the bun bug URI to explain why that workaround to resolve cwd manually is currently needed
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.
@jerome-benoit squashed it & added the link + explanation
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.
Sorry, I mean in the code comment: so people do not remove it. That code should not be here in normal circumstance and people might remove it because of that. Keeping track of why it's here will reduce that probability ;)
|
It's a bun bug Easier to delete 2 lines when bun will fix the issue. |
But it does not address the main concern here. |
|
Ive just tested, it works in dev mode against my own repo and build is working PS: anyway tested the other command, executing being in repo |
Some edge cases are not properly handled by your PR. The code here is trying the resolve cwd to handle most of them (while also consolidating worker files path definition). |
|
Can you provide that edge cases? So I could learn |
|
76400b4 to
ebb0914
Compare
Uses sst#3778 Signed-off-by: Christian Stewart <[email protected]>
Previous versions allowed:
```
bun install
bun dev ../path/to/my/project
```
But now I get:
```
% bun dev ../path/to/my/project
$ bun run --cwd packages/opencode --conditions=browser src/index.ts ../path/to/my/project
Error: Failed to change directory to /Users/cjs/repos/opencode/packages/path/to/my/project
```
If I try to add more ../ I get:
```
% bun dev ../../../path/to/my/project
$ bun run --cwd packages/opencode --conditions=browser src/index.ts ../../../path/to/my/project
ErrorEvent {
type: "error",
message: "BuildMessage: ModuleNotFound resolving \"./src/cli/cmd/tui/worker.ts\" (entry point)",
error: null,
}
```
This commit fixes this by using relative path resolution.
There are actually two bugs fixed here:
Bun bug: oven-sh/bun#15981
`bun run --cwd packages/opencode`, the `process.cwd()` returns the `--cwd` value
`packages/opencode`, not the directory the user ran "bun" within. This causes
`path.resolve(args.project)` to resolve relative paths from the wrong base dir.
We can use the URL object directly for worker path in dev mode.
Fixes: sst#3777
Co-authored-by: Sebastian Herrlinger <[email protected]>
Signed-off-by: Christian Stewart <[email protected]>
ebb0914 to
2eba385
Compare
Uses sst#3778 Signed-off-by: Christian Stewart <[email protected]>
It's a workaround for a code interpreter issue. We do not know what is the root cause in the interpreter: import code path resolution? worker code path resolution? worker env init code? generic path resolution issue (unlikely), ... Since you do not know, enforcing a robust cwd path resolution before is the safest way. |
|
I prefer to focus on an optimistic future. Imagine one day Bun fixes the issue, and all you have to do is remove two lines of code or much more in multiple files. It’s quite debatable whether sharing a variable between the build and the code actually makes things better - in both cases, you still need to search for the source. (It’s shared via define and ts will point you to the globals section.)
We’re just implementing a bug workaround. Both changes are the same, except here the path is shared through define. In a pessimistic scenario, it’s still easy to detect the issue in any situation I can imagine. The code above doesn’t produce TypeScript or linting errors if the worker is moved. I absolutely don’t mean to offend - that’s just how I see it. ;-) |
Tunables single source of truth is a best practice. I prefer to focus on building reliable software following documented best practices elaborated by people smarter than me since computer science has begun. |
|
Thanks for both of your contributions! Keep it coming, appreciated. |
Uses sst#3778 Signed-off-by: Christian Stewart <[email protected]>
Uses sst#3778 Signed-off-by: Christian Stewart <[email protected]>
Uses sst#3778 Signed-off-by: Christian Stewart <[email protected]>
Uses sst#3778 Signed-off-by: Christian Stewart <[email protected]>
Uses sst#3778 Signed-off-by: Christian Stewart <[email protected]>
Uses sst#3778 Signed-off-by: Christian Stewart <[email protected]>
Uses sst#3778 Signed-off-by: Christian Stewart <[email protected]>
Uses sst#3778 Signed-off-by: Christian Stewart <[email protected]>
Uses sst#3778 Signed-off-by: Christian Stewart <[email protected]>
Previous versions allowed:
But now I get:
If I try to add more ../ I get:
This commit fixes this by using relative path resolution.
Fixes: #3777