Skip to content

Add PTY support to SWT #2025

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

Closed
wants to merge 1 commit into from
Closed

Conversation

laeubi
Copy link
Contributor

@laeubi laeubi commented Apr 16, 2025

Currently there is no way to execute a process inside a PTY, the Program class only allows to execute a native program (what theoretically could be a Terminal Shell) but not access to a real terminal session.

This now migrated the terminal support from CDT in a way that it could be used via SWT as an alternative to the Program class to execute a process in a terminal session.

The only new public API would be the PTY and the Spawner (what maybe better is renamed to PTYProcess).

FYI @akurtakov @jonahgraham I have only migrated the relevant java parts, remove deprecated code and made some adjustments, omitted the conpty part for now (what could be provided as a separate fragment as it requires JNA and is only relevant for windows). The java code might also be more compacted but I first want to get general agreement on that we can actually integrate it here.

I also just committed the native sources as-is just to show how it looks like these need to be integrated into the native build and I could probably need some help here.

Currently there is no way to execute a process inside a PTY, the Program
class only allows to execute a native program (what theoretically could
be a Terminal Shell) but not access to a real terminal session.

This now migrated the terminal support from CDT in a way that it could
be used via SWT as an alternative to the Program class to execute a
process in a terminal session.
Copy link
Contributor

Test Results

   402 files   -   143     402 suites   - 143   25m 45s ⏱️ - 2m 7s
 4 299 tests  -    74   4 291 ✅  -    70   7 💤  -  5  1 ❌ +1 
12 492 runs   - 4 142  12 401 ✅  - 4 129  90 💤  - 14  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 9919870. ± Comparison against base commit f468eca.

This pull request removes 74 tests.
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_dollarSign
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_emptyString
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_letterA
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_letters
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16LE_null
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_AsciiLetters
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_Asciiletter
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_LotsOfLetters
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_letter
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_letters
…

@akurtakov
Copy link
Member

No MacOS implementation?

@laeubi
Copy link
Contributor Author

laeubi commented Apr 16, 2025

No MacOS implementation?

I see max os in the cdt repo so I would assume it is the same source like for linux there as it has no dedicated folder here ... (@jonahgraham might can confirm)

@jonahgraham
Copy link
Contributor

Linux and macOS both use the unix directory in CDT: https://github.com/eclipse-cdt/cdt/tree/main/core/org.eclipse.cdt.core.native/native_src/unix with some ifdefs such as this

@laeubi
Copy link
Contributor Author

laeubi commented Apr 17, 2025

@jonahgraham thanks for the insights, it is then the right time to drop old windows support.
@akurtakov do you think it would be okay to have (optional?) dependency to jna? Or should we rethink the idea to add it to SWT at all?

@akurtakov
Copy link
Member

Optional dependencies to swt are quite nasty - as they have to be handled in non-osgi env, fragments and etc. As the jsvg example shows we would better stay away from that adventure again. Maybe go for eclipse.platform repo now.
IMO it would be fine to have the code changed to jffm (aka Java 22 prereq or Java 21 with --enable-preview) and have this functionality available only for people on newer JVMs, it's better than dragging jna to swt.

@laeubi
Copy link
Contributor Author

laeubi commented Apr 17, 2025

@akurtakov yes sadly that means we need to rewrite all native code something I'd like to avoid actually.

@laeubi
Copy link
Contributor Author

laeubi commented Apr 19, 2025

I have now checked adding this to platform but this would mean we need to push new binaries, new fragments and so on for other supported architectures as well and setup builds.

I therefore thing it might be better to go for a longer term solution here

  1. Once Add accessor for included pty instance eclipse-cdt/cdt#1109 is there, it would be possible to use CDT bundles for now (that already have the binaries, fragments and infrastructure to build) as the org.eclipse.tm.terminal.control.feature is already part of (all?) EPPs this will not really pull in something new.
  2. I probably would suggest some smaller cleanups at CDT then, maybe a dedicated feature for only the native stuff would be good as well.
  3. once we can officially use Java 24 it would be a good time to migrate the code finally to platform and rewrite the binary access to FFM.

this would bring the most benefit in the shortest time from my point of view with less effort and intermediate solutions.

@akurtakov
Copy link
Member

Should this one be closed now as using FFM is not on the table any time soon?

@laeubi
Copy link
Contributor Author

laeubi commented May 3, 2025

Yes this can be closed in the meanwhile.

@laeubi laeubi closed this May 3, 2025
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