fix: fix node-pty packaging #664
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| "electron": "^38.2.1" | ||
| "electron": "^38.2.1", | ||
| "@lydell/node-pty": "1.1.0", | ||
| "node-pty": "1.1.0-beta39" |
There was a problem hiding this comment.
This should not be needed at all anymore.
There was a problem hiding this comment.
i need regular node-pty for electron
sreya
force-pushed
the
ptyfix
branch
2 times, most recently
from
dc816a9 to
97a28e4
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
2 participants
I'm going to try to articulate the problem as best as I understand it at the moment. Web terminals are currently used in the current cases:
In order to make web terminals work we need to have access to a pty handle aka we have to use
node-pty. The problem is that we basically distribute two versions of mux: an electron variant for desktop usage, and a server variant.node-ptydoesn't bundle any linux prebuilds so when you runnpm install muxit has to build node-pty from source but it inevitably fails for a lot of cases because the environment lacks the build tools. To fix this you can use something like@lydell/node-ptybut then you break the electron use case because the prebuilds it bundles for mac aren't for the electron ABI and it doesn't bundle source files so it can't build from source.The fucked solution I came up with is to basically include both with
@lydell/node-ptyas an optional dependency. What happens at runtime is it tries to use@lydell/node-ptyfirst, (which should succeed for the linux use case) and if not falls back to regular olenode-ptywhich should be fine for desktop applications.Developers have to run
make devmoving forward if developing for the desktop use case (which they have to anyway?)