Skip to content

Move to migrated platform terminal #1233

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

laeubi
Copy link
Contributor

@laeubi laeubi commented Jul 4, 2025

@laeubi
Copy link
Contributor Author

laeubi commented Jul 18, 2025

@ruspl-afed as discussed in the IDE call yesterday, this is the PR for adjustments in CDT to migrate to the new terminal view, would be great if you can help with this as well.

@ruspl-afed
Copy link
Member

Software being installed: org.eclipse.cdt.debug.application.product 12.2.0.qualifier

Please have a look here

@laeubi
Copy link
Contributor Author

laeubi commented Jul 18, 2025

The feature is still there! So this should not be the problem, it currently fails for some org.bouncycastle stuff, maybe I should try first a PR that uses ibuilds repo to not change too much things at once....

@jld01
Copy link
Contributor

jld01 commented Jul 18, 2025

The feature is still there! So this should not be the problem, it currently fails for some org.bouncycastle stuff, maybe I should try first a PR that uses ibuilds repo to not change too much things at once....

The bouncycastle version issue seems unrelated to this PR. I have been experimenting on #1241. It would be preferable to use a separate PR for releng changes, but I won't have time to look at this again today. Please feel free to work on a solution yourselves.

@ruspl-afed
Copy link
Member

@laeubi I plan to continue looking at your changes today, please let me know about your plans.
I also agree that we should split PRs

@laeubi
Copy link
Contributor Author

laeubi commented Jul 18, 2025

I think

needs investigation first and if that works I'll rebase my PR to see what else fails (maybe).

@ruspl-afed
Copy link
Member

needs investigation first

looking into it. may be just an infra failure

@ruspl-afed
Copy link
Member

may be just an infra failure

at least license check converged after restart (was ClearlyDefined failure)
restarted main build action as well to see what is wrong

@ruspl-afed
Copy link
Member

restarted main build action as well to see what is wrong

I think org.eclipse.cdt.ui.tests.buildconsole.BuildConsoleTests is not happy @laeubi

@ruspl-afed
Copy link
Member

2025-07-18T08:23:21.2437933Z Running org.eclipse.cdt.ui.tests.buildconsole.BuildConsoleTests
2025-07-18T10:02:34.8750427Z ##[error]The operation was canceled.

@laeubi
Copy link
Contributor Author

laeubi commented Jul 18, 2025

See here: #1241 the test seem to timeout currently :-\

@jld01
Copy link
Contributor

jld01 commented Jul 18, 2025

@laeubi
Copy link
Contributor Author

laeubi commented Jul 18, 2025

Renaming a private (!) field of an internal class can merely be seen as breaking change .. I'm not sure why CDT do the same bad hacks as Platform did, but it has to do the same adaption than here then:

eclipse-platform/eclipse.platform.ui@0e4942b#diff-54c81a16536eba9489797a9d665c195f0908163e2c7b14ad055d78cc62579fdaL193

@ruspl-afed
Copy link
Member

@laeubi now CDT uses the later Platform I build to simplify Terminal-related changes
Please let me know regarding your further plans

@laeubi laeubi force-pushed the move_to_migrated_platform_terminal branch from 84d70a7 to 49ebebd Compare July 18, 2025 17:22
@laeubi
Copy link
Contributor Author

laeubi commented Jul 18, 2025

Rebase done, lets see if it works out better :-)

I think I should probably squash all commits into one if everything works.

@ruspl-afed
Copy link
Member

Please have a look at product manifest of org.eclipse.cdt.debug.application.product @laeubi

@laeubi
Copy link
Contributor Author

laeubi commented Jul 18, 2025

Please have a look at product manifest of org.eclipse.cdt.debug.application.product @laeubi

It was actually the feature using a wrong version here...

@laeubi
Copy link
Contributor Author

laeubi commented Jul 18, 2025

Some more feature cleanups ...

@ruspl-afed
Copy link
Member

It was actually the feature using a wrong version here...

Sorry for the wrong pointer, I hope that at least direction was right :)

@laeubi
Copy link
Contributor Author

laeubi commented Jul 18, 2025

Yes no problem, now the build actually starts and resolves it gives better errors to work on... code cleaniness check seem needs adjustments at least I see some exceptional handling from @jonahgraham last time something was moved to platform... so we are getting closer!

@ruspl-afed
Copy link
Member

for code cleanliness failure the message is

Missing about.ini entry in org.eclipse.tm.terminal.connector.ssh.feature/build.properties

@laeubi laeubi force-pushed the move_to_migrated_platform_terminal branch from b529ac7 to ca2fd97 Compare July 19, 2025 06:38
@ruspl-afed
Copy link
Member

I'm thinking how we can converge on this. May be we can split this PR and first transfer CDT-DSF to a new Platform Terminal?

@laeubi
Copy link
Contributor Author

laeubi commented Jul 19, 2025

Some thing might be able to be extracted, but I think there is not much benefit and will possibly result in conflicts in the build. I think I'm almost done with that and then I can try to came up with trying it in smaller chunks, e.g. currently the actual deletion of stuff is the far biggest change here, so one might simply let linger around the deleted stuff and then have one PR that actually delete it (but nothing else changes).

I also bumped the major version for the both CDT connectors from 4 > 5, since they are using new API in their own API it seems not useful to not do it.

One might think about after doing the migration to also clean up these remaining connects and maybe even change their namespace as well to prevent confusion with the old ones. The also their version can be reset and maybe not export everything as API...

e.g. org.eclipse.tm.terminal.connector.cdtserial -> org.eclipse.cdt.terminal.connector.serial (and move it into the core or launch or debug or ... subfolder)
and org.eclipse.tm.terminal.connector.remote voud probabbly better go to the remote and be renamed org.eclipse.remote.terminal.connector.

But I would leave this aspects to the CDT maintainer to decide!

@ruspl-afed
Copy link
Member

My congrats with the first fully green build @laeubi !

There are two kind of things that I cannot merge without visa from CDT PL @jonahgraham

  1. major version bumps, because it raises a lot of further questions
  2. removal of anything, because CDT codebase is very conservative

What I think I can merge is switch of CDT itself to the new Platform Terminal
I'm not sure how useful this partial change could be, but at least it may reduce the volume of further discussions.

@laeubi
Copy link
Contributor Author

laeubi commented Jul 19, 2025

major version bumps, because it raises a lot of further questions

I think that is the cleanest solution here, as it depends on new API that is leaked into the bundles own API. An alternative would be the rename / reversion of the whole thing. In the end I think no one should actually API wise depend on a connector anyways, but it is API at the moment sadly.

removal of anything, because CDT codebase is very conservative

If one depends on anything from the old world, it is still there in the old releases an/or branches, it does not seem useful to retain the sources here if they have evolved/moved to another project I think. Also I think usual users/extenders of CDT should never notice that particular thing:

  • Everything from CDT is only referenced internally and do not surface to API
  • The Connectors are actually extensions to the terminal view and nothing tied to CDT itself

@laeubi laeubi force-pushed the move_to_migrated_platform_terminal branch from 8d13390 to 7c24da0 Compare July 19, 2025 13:11
@laeubi
Copy link
Contributor Author

laeubi commented Jul 19, 2025

I now squashed everything, so this is ready for review, will try to extract some aspects into separate PRs now.

@laeubi laeubi marked this pull request as ready for review July 19, 2025 13:11
@laeubi
Copy link
Contributor Author

laeubi commented Jul 19, 2025

GDB Migration:

@laeubi
Copy link
Contributor Author

laeubi commented Jul 19, 2025

remote migration:

@laeubi laeubi force-pushed the move_to_migrated_platform_terminal branch from 7c24da0 to d49db2d Compare July 19, 2025 15:12
@laeubi
Copy link
Contributor Author

laeubi commented Jul 19, 2025

Migration of remaining connectors to new API including major version bump:

@laeubi
Copy link
Contributor Author

laeubi commented Jul 19, 2025

Using of the new bundle names from platform in feature files:

@laeubi laeubi force-pushed the move_to_migrated_platform_terminal branch from d49db2d to 90d2f50 Compare July 19, 2025 17:42
@ruspl-afed
Copy link
Member

Thank you for supporting this PR in actual state @laeubi !
I suggest to remove added .project manifests from it to make it even more clear

@jonahgraham
Copy link
Member

@ruspl-afed are you able to merge this? You have done the work to review so far.

thanks @laeubi for this PR.

@laeubi
Copy link
Contributor Author

laeubi commented Jul 23, 2025

@jonahgraham I think I should rebase the PR and have already creates smaller chunks that should be merged first:

so I will convert this to draft, @ruspl-afed has had some concerns but these are maybe better discussed on the smaller PRs.

@laeubi laeubi force-pushed the move_to_migrated_platform_terminal branch from 90d2f50 to 61ad699 Compare July 24, 2025 06:34
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.

4 participants