Skip to content

Add Windows ConPTY support and tar.xz release packaging#108

Open
kiennq wants to merge 1 commit into
dakra:mainfrom
kiennq:pr/windows-conpty
Open

Add Windows ConPTY support and tar.xz release packaging#108
kiennq wants to merge 1 commit into
dakra:mainfrom
kiennq:pr/windows-conpty

Conversation

@kiennq
Copy link
Copy Markdown
Contributor

@kiennq kiennq commented Apr 15, 2026

  • ConPTY pseudoconsole backend for Windows terminal I/O
  • Transport abstractions (ghostel--process-send/live-p/set-window-size) that dispatch to ConPTY or PTY depending on the active backend
  • Windows-specific process startup skipping the shell wrapper
  • ConPTY module build via emacs-util-mods dependency
  • Release artifacts packaged as per-platform .tar.xz archives containing ghostel-module + conpty-module (Windows)
  • Download/extract flow for .tar.xz release archives
  • Windows entries in CI build and test matrix
  • 7 new ConPTY tests + extract-module-archive test

@kiennq kiennq force-pushed the pr/windows-conpty branch 6 times, most recently from a89709b to 72a67ef Compare April 15, 2026 07:58
@dakra
Copy link
Copy Markdown
Owner

dakra commented Apr 15, 2026

Thank you very much for this. Imho looks already really good.
I let claude review it and also added that I would like a good separation between the existing and the windows code so I can revert it if there should be something major wrong/buggy with the windows code and I'm not able to maintain it (hopefully not ;) )

So here is the claude review after chatting a bit.
Don't take it too serious. Make changes as you see it makes sense.
I'm not too worried about the separation as looking at it now is already pretty clean imho,
but having it separate and in one place as much as possible is preferred.
Just use your judgement ;)

Here the review:

Review

Thanks for this — the ConPTY work is well-structured overall and the transport abstraction is the right shape. Before merging I'd like a few changes to make the Windows path more isolated (so it's easy to revert as a unit if Windows-specific bugs surface), plus a few smaller fixes from the review. Detailed list below; an LLM should be able to crank through these mechanically.

Required: Windows isolation

The goal is that reverting Windows support is "delete one section + one dispatch line + one bootstrap block", with no diff churn in shared Unix code paths.

1. Cluster all Windows-only elisp under one section

Group the following under a single ;;; Windows ConPTY backend section header in ghostel.el (currently they're scattered):

  • conpty--* declare-function block
  • ghostel--conpty-notify-pipe defvar
  • ghostel--conpty-module-file-path
  • ghostel--ensure-conpty-loaded
  • ghostel--native-runtime-ready-p
  • ghostel--conpty-active-p, ghostel--process-live-p, ghostel--process-send, ghostel--process-set-window-size, ghostel--conpty-filter
  • ghostel--start-process-windows (see Fix build.sh to find deps in CI zig cache directories #2)
  • ghostel--extract-module-archive, ghostel--publish-downloaded-module-archive, ghostel--replace-module-file if they're Windows-only — if they're shared with the new tar.xz flow, leave them in the download section.

2. Factor Windows process startup out of ghostel--start-process

Right now ghostel--start-process is a fork inside one big let*: env got split into env-overrides + process-environment, the shell-command form has an if windows-nt, and set-process-coding-system / set-process-window-size / query-flag are wrapped in unless windows-nt. The Unix body no longer matches its pre-PR shape, so a revert isn't a clean delete.

Please restructure so:

  • ghostel--start-process-windows (new defun, in the Windows section) contains all Windows-specific setup: make-pipe-process, process-put, conpty--init, query-flag, the Windows-specific shell-command (just (list shell)), and the env construction it needs.
  • ghostel--start-process becomes:
    (if (eq system-type 'windows-nt)
        (ghostel--start-process-windows ...)
      <original Unix body, byte-identical to pre-PR>)
  • The Unix body should not have unless windows-nt wrappers around set-process-coding-system / set-process-window-size / set-process-query-on-exit-flag / process-put — those guards go away because that whole branch only runs on Unix.
  • Drop the redundant inner (setq ghostel--process proc) and inner set-process-query-on-exit-flag in the Windows branch — :noquery t already covers the latter and the outer setq covers the former.

3. Keep transport helpers as permanent thin wrappers

ghostel--process-send, ghostel--process-live-p, ghostel--process-set-window-size should stay even if ConPTY is later reverted — that way the ~10 call-site rewrites in ghostel.el (process-send-stringghostel--process-send, process-live-pghostel--process-live-p) don't have to be undone. On non-Windows, the helpers are one-line forwarders. Please add a brief comment on each helper noting they're permanent wrappers around the underlying primitive, not a Windows-only abstraction.

4. Bootstrap: avoid nesting the ConPTY load inside the existing condition-case

Currently the new (when (eq system-type 'windows-nt) (condition-case err (ghostel--ensure-conpty-loaded dir) ...)) is nested inside the existing condition-case err for module-load, which shadows err. Please move it to a separate top-level (when (eq system-type 'windows-nt) ...) block immediately after the existing bootstrap so it's a clean append.

In ghostel-toggle, the swap from (fboundp 'ghostel--new) to (ghostel--native-runtime-ready-p) is fine — keep it. ghostel--native-runtime-ready-p already short-circuits the conpty check to t on non-Windows.

Required: Restore macOS strip step

The release workflow rewrite dropped strip -x on macOS without replacement. strip -x removes non-exported symbols from the dylib and meaningfully shrinks the artifact. It's orthogonal to the tar.xz packaging change.

Please restore it as a pre-tar step in .github/workflows/release.yml:

- name: Strip non-exported symbols (macOS)
  if: runner.os == 'macOS'
  run: strip -x zig-out/bin/ghostel-module${{ matrix.suffix }}

(Place it before the "Package release artifact" step.)

The universal .tar.xz packaging is good — keep it for all platforms; xz compresses .so/.dylib well and saves bandwidth. The conpty bundling logic for Windows-only is correct.

Smaller fixes

5. Resize ordering in the ConPTY branch

In ghostel--window-adjust-process-window-size, the new (ghostel--process-set-window-size process height width) runs after (ghostel--delayed-redraw buffer), so the buffer is redrawn against the new term size before the ConPTY child has been told to resize. Please call ghostel--process-set-window-size before the redraw — or, ideally, before ghostel--set-size — so the child knows the new size first.

6. Simplify ghostel--conpty-module-file-path

The current construction wraps expand-file-name around file-name-as-directory around another expand-file-name. Simpler:

(defun ghostel--conpty-module-file-path (&optional dir)
  "Return the stable conpty-module path in DIR."
  (expand-file-name (concat "conpty-module" module-file-suffix)
                    (or dir (file-name-directory (or load-file-name
                                                     (locate-library "ghostel")
                                                     buffer-file-name)))))

7. addWindowsRuntimeLibraries — clarify or drop the MSVC branch

The SDK lookup only runs when abi == .msvc, but the entire CI matrix builds with *-windows-gnu. Either:

  • Drop the if (rt.abi != .msvc) return; block and everything below it (simpler, matches what's actually exercised), or
  • Leave it but add a comment explaining it's there for future MSVC builds.

8. Live ConPTY test gated off CI

ghostel-test-conpty-init-keeps-shell-alive-on-windows has (skip-unless (not (getenv "GITHUB_ACTIONS"))), so the only end-to-end ConPTY assertion never runs in CI — the Windows job currently just smoke-tests that the module loads. Please investigate the underlying reason (cmd.exe startup timing? sleep/poll race?) and either fix it or document in a comment why it has to be skipped, with a TODO.

9. Tar extraction note

tar xJf … -C dest doesn't block path traversal in archive entries. The source is your own GitHub release so this is acceptable, but please add a one-line comment in ghostel--extract-module-archive noting the trust assumption, so future maintainers don't repurpose it for untrusted archives.

Nice-to-have (optional)

  • Consider strip --strip-unneeded for Linux release artifacts symmetrically with macOS — wasn't in the original workflow either, so up to you.
  • The ghostel-test-extract-module-archive-uses-tar-xf test is great; consider adding a complementary test that verifies ghostel--publish-downloaded-module-archive only copies conpty-module.dll on Windows, not on Linux/macOS.

Summary of the revert story after these changes

If Windows ever needs to be ripped out:

  1. Delete the ;;; Windows ConPTY backend section in ghostel.el.
  2. Replace the (if (eq system-type 'windows-nt) (ghostel--start-process-windows ...) <unix>) dispatch with just <unix>.
  3. Delete the (when (eq system-type 'windows-nt) ...) bootstrap block.
  4. Delete the if (target_os == .windows) blocks in build.zig and the emacs_util_mods dep in build.zig.zon.
  5. Remove Windows rows from CI matrices.

Transport helpers stay; tar.xz packaging stays; no shared code paths get touched. That's the goal.

@kiennq kiennq force-pushed the pr/windows-conpty branch 2 times, most recently from fa8ae17 to 02ed6f2 Compare April 15, 2026 19:03
@kiennq
Copy link
Copy Markdown
Contributor Author

kiennq commented Apr 15, 2026

Thanks, the review is helpful. I will try to address most of them. However for the strip, the Zig build system already handles stripping — build.zig sets .strip = true for release builds, plus enables link_gc_sections, link_function_sections, link_data_sections, and dead_strip_dylibs. This produces the same result as strip -x (removes non-exported symbols) without needing an external tool step. The original strip -x in the workflow was compensating for a build that didn't strip — with the Zig flags it's redundant.

@dakra
Copy link
Copy Markdown
Owner

dakra commented Apr 15, 2026

However for the strip, the Zig build system already handles stripping

At least for MacOS that's not completely true.
I build with zig build -Doptimize=ReleaseFast -Dcpu=baseline
and then did strip -x:

State Size Defined symbols Total symbol table
Zig build only 1,556,536 bytes (1,520 KB) 3,879 4,084
After strip -x 1,381,000 bytes (1,349 KB) 389 594
Savings 175,536 bytes (171 KB, 11.3%) -3,490 -3,490

.strip = true in Zig strips debug info but leaves the symbol table mostly intact, so it's not equivalent to strip -x.

@kiennq kiennq force-pushed the pr/windows-conpty branch 3 times, most recently from e601474 to 4150e0e Compare April 16, 2026 00:50
- ConPTY pseudoconsole backend for Windows terminal I/O
- Transport abstractions (ghostel--process-send/live-p/set-window-size)
  that dispatch to ConPTY or PTY depending on the active backend
- Shared process-start state extraction for Unix/Windows code sharing
- Windows-specific process startup factored into ghostel--start-process-windows
- ConPTY module build via emacs-util-mods dependency
- Lazy ConPTY loading when user invokes ghostel on Windows
- Release artifacts packaged as per-platform .tar.xz archives
  containing ghostel-module + conpty-module (Windows)
- Download/extract flow for .tar.xz release archives
- Strip step for all platforms in release workflow
- Windows entries in CI build and test matrix
- .gitattributes enforcing LF line endings

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@kiennq kiennq force-pushed the pr/windows-conpty branch from 4150e0e to 4e44fd9 Compare April 16, 2026 01:04
@kiennq
Copy link
Copy Markdown
Contributor Author

kiennq commented Apr 16, 2026

@dakra, please let me know if you want me to fix anything else

@dakra
Copy link
Copy Markdown
Owner

dakra commented Apr 16, 2026

@kiennq Thank you very much. Looks already really good.

3 quick thoughts (without consulting AI ;) ):

Iirc I did the strip -x only for Mac because it was only needed there.
On Linux they where already stripped, you added it for all systems and with strip -x .. || strip ...
not sure if that change is needed (and relevant for the windows build).

There are a few unrelated indent changes

What do you think if we just copy your conpty.zig in here? It's only 560 lines and probably doesn't change very often. would have the advantage to have 1 less external dependency and also we would not need a few workarounds (e.g. tar) because we have 2 .dll files for windows.

I would have a closer look at this at the weekend.

Thanks again ❤️

@kiennq
Copy link
Copy Markdown
Contributor Author

kiennq commented Apr 16, 2026

What do you think if we just copy your conpty.zig in here? It's only 560 lines and probably doesn't change very often. would have the advantage to have 1 less external dependency and also we would not need a few workarounds (e.g. tar) because we have 2 .dll files for windows.

The conpty is a module on its own and independent from ghostel so I would like to keep it separated. It can be used by emacs-vterm as well, so pretty versatile.

@dakra
Copy link
Copy Markdown
Owner

dakra commented Apr 16, 2026

The conpty is a module on its own and independent from ghostel so I would like to keep it separated. It can be used by emacs-vterm as well, so pretty versatile.

Sure, I don't want to move the emacs-utils repo (/conpty) in here. Just as it is one small file, kinda vendor it here similar to emacs.h or I'll soon add another small PNG decoder for the kitty graphics support.
That way we would save a few extra steps that comes with having to deal with 2 .dll files on windows.
Not sure what the disadvantages would be by vendoring it in vs having it as extra dep?
(And just to be clear, I don't have too strong of an opinion, just discussing..)

@kiennq
Copy link
Copy Markdown
Contributor Author

kiennq commented Apr 17, 2026

Sure, I don't want to move the emacs-utils repo (/conpty) in here. Just as it is one small file, kinda vendor it here similar to emacs.h or I'll soon add another small PNG decoder for the kitty graphics support.

Since the conpty will expose some module functions, would it be okay for them to clobber the emacs_module_init in ghostel-module?
I don't have much concern about one or multiple files on Windows since we will still treat them all the same, unpack -> move the currently in-used binaries to backup -> deploy the new one. That's the only way we can download the new module while the current ghostel is running

@dakra
Copy link
Copy Markdown
Owner

dakra commented Apr 17, 2026

After thinking and talking with Claude a bit more about it, I personally would favour to put the conpty stuff just under src/conpty or so. I couldn't see any downside and only a few improvements (one dll download, no tar calls, no ghostel--ensure-conpty-loaded (because if ghostel is loaded, so is contpy on windows), etc)

Since the conpty will expose some module functions, would it be okay for them to clobber the emacs_module_init in ghostel-module?

sure, you can make the changes necessary.

Also sorry for always changing the codebase under you. ghostel is still in heavy development,
so hopefully we can get this merged rather sooner than later so we don't always have to rewrite the rebase on top of a changed main :)

@ISouthRain
Copy link
Copy Markdown

@dakra @kiennq

Hello, every one.

What happened?

Making ghostel support Windows seems to be going smoothly, where kiennq/ghostel is merged and updated daily from dakra/ghostel, and runs normally on Windows.

Not sure why this PR stalled?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants