Community
Participate
Working Groups
Windows CE, 9x, NT4, 2000 aren't relevant anymore, XP support is waning (Java SE 8 has dropped it). There's a noticeable amount of dead code dealing with those platforms' quirks and bugs. I propose to remove this code. The patch is being uploaded to Gerrit. Summary of the changes: * Always true and always false conditionals that depend on Windows version are flattened/removed. * ANSI versions of PI functions and structures are removed; Unicode versions are renamed to unprefixed form. * TCHAR and related wrappers are removed; TCHAR is converted to char[] according to the API conversion table; small helper class (WCHAR) is added to deal with String <-> null terminated char[] conversions similar to GTK Converter. * Most of the unused PI functions and structures are removed. The risk of new bugs is mitigated by strictly mechanical nature of the change. About 8500 lines of Java are removed, 3500 are modified. (Modified lines are unindents, renames and TCHAR->char[] replacements). Downstream users who rely on internal SWT PI classes might be affected, although Eclipse itself seems to work fine. PS. Vista is even more dead that XP, it would be nice to push the required Windows version to 7.
New Gerrit change created: https://git.eclipse.org/r/117254
Woh, that looks like a lot of work, thanks Nikita. SWT team, please review for M6.
(In reply to Nikita Nemkin from comment #0) > Windows CE, 9x, NT4, 2000 aren't relevant anymore, XP support is waning > (Java SE 8 has dropped it). I tried to confirm on Java8 support for Win XP, found below link: https://www.java.com/en/download/faq/winxp.xml It suggest that Java8 support is not explicitly dropped and it might still work on XP... but Java no longer provide complete guarantees. > There's a noticeable amount of dead code dealing with those platforms' > quirks and bugs. I propose to remove this code. The patch is being uploaded > to Gerrit. IMO, even if XP is not officially supported by Eclipse.. but it still works again we don't guarantee.[Yet to hear from community if Eclipse completely breaks on XP, hence moving out of M6] As of today, do you see any other benefits of dropping support for XP, other than cleaner code ?
> IMO, even if XP is not officially supported by Eclipse.. but it still works > again we don't guarantee.[Yet to hear from community if Eclipse completely > breaks on XP, hence moving out of M6] SWT master already doesn't work on XP, because it's built with VS2017/Win10 SDK.
(In reply to Nikita Nemkin from comment #5) > > IMO, even if XP is not officially supported by Eclipse.. but it still works > > again we don't guarantee.[Yet to hear from community if Eclipse completely > > breaks on XP, hence moving out of M6] > > SWT master already doesn't work on XP, because it's built with VS2017/Win10 > SDK. See, VS2017/Win10 SDKs are needed for SWT Windows native code compilation, which is something very specific to SWT and not a primary usage of Eclipse. Right thing to evaluate is: Whether latest Eclipse 4.8(M5 & on-wards) works or completely breaks on XP ? (In reply to Eclipse Genie from comment #3) > New Gerrit change created: https://git.eclipse.org/r/117254 Until we take a final call on this work, please try to break above patch, may be something like below: - One patch for removing the old OS version specific code. - One can be around WCHAR usage - Another can be dedicated for code refactoring purpose. Idea is to consume these changes in multiple manageable chunks.. if any change causes some issue then it should be easy to narrow down on the problem. Thanks!
> See, VS2017/Win10 SDKs are needed for SWT Windows native code compilation, > which is something very specific to SWT and not a primary usage of Eclipse. > > Right thing to evaluate is: > Whether latest Eclipse 4.8(M5 & on-wards) works or completely breaks on XP ? As of right now 1) Java 8 installers from Oracle don't work on XP, neither JRE nor JDK. 2) Photon M5 doesn't work on XP, at all. Apparently, nobody noticed. > Until we take a final call on this work, please try to break above patch, > may be something like below: > - One patch for removing the old OS version specific code. > - One can be around WCHAR usage > - Another can be dedicated for code refactoring purpose. OK I'll make two separate patches: old code removal and TCHAR removal.
(In reply to Nikita Nemkin from comment #7) > 1) Java 8 installers from Oracle don't work on XP, neither JRE nor JDK. While the Oracle Java 8 installers no longer work, an embedded JRE does quite well. > 2) Photon M5 doesn't work on XP, at all. Yes, 4.8 M4 still worked under XP, but M5 does not. My RCP application crashes at startup: org.eclipse.swt.SWTError: Item not added at org.eclipse.swt.SWT.error(SWT.java:4578) at org.eclipse.swt.SWT.error(SWT.java:4467) at org.eclipse.swt.SWT.error(SWT.java:4438) at org.eclipse.swt.widgets.Widget.error(Widget.java:482) at org.eclipse.swt.widgets.CoolBar.createItem(CoolBar.java:283) at org.eclipse.swt.widgets.CoolItem.<init>(CoolItem.java:76) [...] at org.eclipse.jface.window.Window.create(Window.java:426) [...]
New Gerrit change created: https://git.eclipse.org/r/117610
New Gerrit change created: https://git.eclipse.org/r/117613
New Gerrit change created: https://git.eclipse.org/r/117612
New Gerrit change created: https://git.eclipse.org/r/117611
New Gerrit change created: https://git.eclipse.org/r/117609
I ended up splitting the patch into 5 parts (on top of each other due to small interdependencies). Hopefully they will be easier to review. The contents is the same as the original patch. Each step (except the very small part 4) was tested with AllTests and ControlExample on Win10 x64. Final step was also (lightly) tested with child Eclipse.
(In reply to Stefan Mücke from comment #8) > (In reply to Nikita Nemkin from comment #7) > > 1) Java 8 installers from Oracle don't work on XP, neither JRE nor JDK. > > While the Oracle Java 8 installers no longer work, an embedded JRE does > quite well. Ok, Java8 JRE can be copied over to XP. > > 2) Photon M5 doesn't work on XP, at all. > > Yes, 4.8 M4 still worked under XP, but M5 does not. My RCP application > crashes at startup: > > org.eclipse.swt.SWTError: Item not added > at org.eclipse.swt.SWT.error(SWT.java:4578) > at org.eclipse.swt.SWT.error(SWT.java:4467) > at org.eclipse.swt.SWT.error(SWT.java:4438) > at org.eclipse.swt.widgets.Widget.error(Widget.java:482) > at org.eclipse.swt.widgets.CoolBar.createItem(CoolBar.java:283) > at org.eclipse.swt.widgets.CoolItem.<init>(CoolItem.java:76) > [...] > at org.eclipse.jface.window.Window.create(Window.java:426) > [...] With my quick testing on XP(64bit) VM: I find 4.8M5(64bit) Eclipse open up fine, Java work-space comes up fine and could install EGit plugin as well. Can you confirm the behavior of stand-alone Eclipse4.8M5 on WinXP ? (In reply to Nikita Nemkin from comment #14) > I ended up splitting the patch into 5 parts (on top of each other due to > small interdependencies). > > Hopefully they will be easier to review. The contents is the same as the > original patch. > > Each step (except the very small part 4) was tested with AllTests and > ControlExample on Win10 x64. Final step was also (lightly) tested with child > Eclipse. Thanks, we'll try to review them but push the changes post 4.8 only.
(In reply to Niraj Modi from comment #15) > Can you confirm the behavior of stand-alone Eclipse4.8M5 on WinXP ? I was wrong, it does launch on XP, tested it on 32-bit. (Apparently, eclipse.exe launcher is built with older toolset and DLL versions don't matter.) Doesn't change the fact that XP should be dropped. It's a burden to test every change on XP (in addition to 7 and 10). And XP lacks some modern APIs that SWT could use (WIX, D2D and DWrite in particular). > Thanks, we'll try to review them but push the changes post 4.8 only. So, postponed for at least 5 months? If the changes aren't acceptable, can't be improved and have no people willing to review them, let's just close this bug with WONTFIX.
Nikita, I will discuss dropping XP support in our next PMC call. Niraj, can you please summerize why you want to consider these patches only after 4.8? So that we know both sides for our next call.
For reference Microsoft ended support 2014, see https://www.microsoft.com/en-us/windowsforbusiness/end-of-xp-support
Target platform also does not include XP http://www.eclipse.org/projects/project-plan.php?projectid=eclipse#target_environments
(In reply to Nikita Nemkin from comment #16) > (In reply to Niraj Modi from comment #15) > > Can you confirm the behavior of stand-alone Eclipse4.8M5 on WinXP ? > > I was wrong, it does launch on XP, tested it on 32-bit. (Apparently, > eclipse.exe launcher is built with older toolset and DLL versions don't > matter.) > > Doesn't change the fact that XP should be dropped. > > It's a burden to test every change on XP (in addition to 7 and 10). And XP > lacks some modern APIs that SWT could use (WIX, D2D and DWrite in > particular). > > > Thanks, we'll try to review them but push the changes post 4.8 only. > > So, postponed for at least 5 months? > > If the changes aren't acceptable, can't be improved and have no people > willing to review them, let's just close this bug with WONTFIX. Hi Nikita, Thanks for all the work in preparing the patches! Windows XP is not a target platform for Photon and Eclipse doesn’t officially support it. That means, we don’t make fixes to XP specific bugs, don’t test our builds on it and don’t add new features for XP. But, it works as is and we have not explicitly removed the XP support. I agree that testing on multiple OS is time consuming, but as mentioned above we don’t test our fixes and builds on XP. Also, we don’t require the new features to work on XP, so that shouldn’t stop us from adding new features or using new APIs. I’m not aware of any feature or API in SWT being blocked by this. Please let me know if I’ve missed anything?
(In reply to Lars Vogel from comment #17) > Nikita, I will discuss dropping XP support in our next PMC call. > > Niraj, can you please summerize why you want to consider these patches only > after 4.8? So that we know both sides for our next call. Hi Lars, IMHO, it is too late to accommodate the change for 4.8. The changes involved are huge and touch almost all the files the win32 code. The changes are not just about removing the version checks, they also change the *types* used. I also noticed that 2 of the patches have more than 1000 lines of change and would require CQ approvals and we are past the date for CQ requests. We have less than 2 weeks of development cycle left for 4.8M6 and have our hands full with the 4.8 plan items (Hi-DPI work). I don’t think merging these changes in M7 (last milestone) will be a good idea due to the nature of changes involved. That said, we will start reviewing/testing the patches as soon as we can and merge the changes after 4.8.
(In reply to Lakshmi Shanmugam from comment #21) > just about removing the version checks, they also change the *types* used. I > also noticed that 2 of the patches have more than 1000 lines of change and > would require CQ approvals and we are past the date for CQ requests. Does Eclipse require CQ approval for whitespace changes and autogenerated code?
(In reply to Lakshmi Shanmugam from comment #21) > Hi Lars, > IMHO, it is too late to accommodate the change for 4.8. The changes involved > are huge and touch almost all the files the win32 code. The changes are not > just about removing the version checks, they also change the *types* used. I > also noticed that 2 of the patches have more than 1000 lines of change and > would require CQ approvals and we are past the date for CQ requests. > > We have less than 2 weeks of development cycle left for 4.8M6 and have our > hands full with the 4.8 plan items (Hi-DPI work). I don’t think merging > these changes in M7 (last milestone) will be a good idea due to the nature > of changes involved. Thanks for the explaination. I still bring it up the PMC that we should explicitely drop support for Windows XP. This should make it easier to merge these patches, once the opportunity arises. Nikita, this is IMHO one of the best contribution I have seen in years. Deletion so much outdated code, is very valuable for us, so ensure that SWT will remain active and maintainable in the future. Please continue providing patches, even if the review of this contribution will be a little bit deplayed. Work on 4.9 will start before the official 4.8 release, so the delay will not be 5 months.
FWIW, merging such changes in M7 IMHO is the best time as it will still be the same amount of time(actually more) we will have for every release after Photon .
Further more it's no API change and M7 is supposed to be our "polish" milestone which this works fits in just fine.
(In reply to Nikita Nemkin from comment #16) > (In reply to Niraj Modi from comment #15) > > Can you confirm the behavior of stand-alone Eclipse4.8M5 on WinXP ? > > I was wrong, it does launch on XP, tested it on 32-bit. (Apparently, > eclipse.exe launcher is built with older toolset and DLL versions don't > matter.) Thanks for confirming. (In reply to Lars Vogel from comment #19) > Target platform also does not include XP > http://www.eclipse.org/projects/project-plan. > php?projectid=eclipse#target_environments Hi Lars, It's also important to consider what Java8's stand is w.r.t. XP As of today Eclipse stand is very similar to Java8. We also don't support XP but Eclipse still works(https://www.java.com/en/download/faq/winxp.xml)
(In reply to Niraj Modi from comment #26) > It's also important to consider what Java8's stand is w.r.t. XP > As of today Eclipse stand is very similar to Java8. We also don't support XP > but Eclipse still works(https://www.java.com/en/download/faq/winxp.xml) Thanks, interesting to see that Oracle also does not support Java anymore for XP. This is IMHO another good point to remove the code with Nikita. Old and outdated code makes maintenance and contributions harder for future Eclipse versions.
This was approaved by the PMC see meeting minutes February 27, 2028 https://wiki.eclipse.org/Eclipse/PMC The note from Mike to cross did not result in feedback so we can proceed. @Niraj, we need CQ for some of the changes. The PMC discussed that if we ask the IP team politely and explain that these are only removals and reorganization of exisitng code that they still might be able to review them. Can you create the CQs or shall I do it for you?
Nikita, can you move the type changes to another bug? Dani raised concerns that this is a different activity rather then the deletion. AFAICS the type changes are encapsolated into the last two Gerrits, please open a new bug for this and move them to the new bug.
(In reply to Lars Vogel from comment #29) > Nikita, can you move the type changes to another bug? Dani raised concerns > that this is a different activity rather then the deletion. AFAICS the type > changes are encapsolated into the last two Gerrits, please open a new bug > for this and move them to the new bug. To clarify: We discussed this in the PMC and agreed that changing the types and other changes that are not purely for the removal of Windows XP code are considered too dangerous for Photon, but we definitely want them in 4.9 M1. Alex volunteered to test and review the removal part. If he is happy, he will file the CQ and ask the IP team for a late approval.
I've abandoned the patches that are too dangerous for 4.8. When 4.9 window opens, I'll resubmit them. The remaining three patches should be safe. Thanks for volunteering to review them.
(In reply to Nikita Nemkin from comment #31) > I've abandoned the patches that are too dangerous for 4.8. When 4.9 window > opens, I'll resubmit them. Can you restore them and point them to Bug 532830? > The remaining three patches should be safe. Thanks for volunteering to > review them. Thanks again for providing them.
(In reply to Nikita Nemkin from comment #31) > I've abandoned the patches that are too dangerous for 4.8. When 4.9 window > opens, I'll resubmit them. > > The remaining three patches should be safe. Thanks for volunteering to > review them. I setup a Windows 10 (64bit) machine to test the 3 patches. The 3 patches seem to work well. - Natives compile without problem. - Child Eclipse fires up well. - AllTests pass (5 fail, but these are not related to these patches). - ControlExample works well - SnippetLauncher fires up snippets well, (haven't tested all snippets, but the first 50 or so seemed to work well). Patches look good to me. But I'm a Gtk/SWT developer, a Windows/SWT developer should do the final review & merge. These are great patches to improve code maintainability.
(In reply to Leo Ufimtsev from comment #33) Great news, thanks. > Patches look good to me. But I'm a Gtk/SWT developer, a Windows/SWT > developer should do the final review & merge. AFAIK is Niraj and the rest of the existing team to busy for the review. > These are great patches to improve code maintainability. Did you try it also with an Eclipse? Do we have instructions how to use the self-compiled SWT binaries in an Eclpise?
(In reply to Lars Vogel from comment #34) > (In reply to Leo Ufimtsev from comment #33) > Great news, thanks. > > > Patches look good to me. But I'm a Gtk/SWT developer, a Windows/SWT > > developer should do the final review & merge. > > AFAIK is Niraj and the rest of the existing team to busy for the review. > > > These are great patches to improve code maintainability. > > Did you try it also with an Eclipse? > Do we have instructions how to use the self-compiled SWT binaries in an > Eclpise? I've fired a child eclipse. What do you mean try with an eclipse? Do you mean like build the swt.jar file and try that with an actual child eclipse? (I have a hacky way of doing this on Gtk, haven't tried windows). Or you mean like build all of eclipse? (I've never actually done that yet).
(In reply to Leo Ufimtsev from comment #35) > I've fired a child eclipse. What do you mean try with an eclipse? OK, so this just works? Didn't know. If that works fine, I think we can proceed. I got also a windows machine setup to help with this review, I will try to compile them also next week and do another round of review.
(In reply to Lars Vogel from comment #36) > (In reply to Leo Ufimtsev from comment #35) > > I've fired a child eclipse. What do you mean try with an eclipse? > > OK, so this just works? Didn't know. Yep, a child eclipse with the newly compiled natives fires up and seems to work well. I've used a pretty plain eclipse with just a single java file. I went through options and tried some dialogues. I didn't observe any issues.
(In reply to Leo Ufimtsev from comment #33) > Patches look good to me. But I'm a Gtk/SWT developer, a Windows/SWT > developer should do the final review & merge. > AFAIK is Niraj and the rest of the existing team to busy for the review. Correct. In the PMC call Alex said he will do the review. Or some other Windows committer has to step forward.
(In reply to Dani Megert from comment #38) > (In reply to Leo Ufimtsev from comment #33) > > Patches look good to me. But I'm a Gtk/SWT developer, a Windows/SWT > > developer should do the final review & merge. > > > AFAIK is Niraj and the rest of the existing team to busy for the review. > > Correct. In the PMC call Alex said he will do the review. Or some other > Windows committer has to step forward. Well, I hope by having ran the patch through the usual tests, it would cost a Windows SWT contributor less time to review the patch and hopefully encourage them to review sooner. @Win/SWT folks, if there is something else to test, please let me know. Otherwise looking forward to having these merged.
Create CQ for the first patch: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=16015
(In reply to Lars Vogel from comment #40) > Create CQ for the first patch: > https://dev.eclipse.org/ipzilla/show_bug.cgi?id=16015 CQ got approved. I will now merge the first patch.
Gerrit change https://git.eclipse.org/r/117609 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=84c9e305cb087110cb300a5e58f86583cf80914d
Gerrit change https://git.eclipse.org/r/117610 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=36a2cde49563bc13e65b5b03e811641de522f240
Is there some further work planned (beyond bug 532830), or can this be closed?
(In reply to Dani Megert from comment #44) > Is there some further work planned (beyond bug 532830), or can this be > closed? We have one more patch, testing it currently.
Gerrit change https://git.eclipse.org/r/117612 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=b33183ce0dec18efde020735ad580baed707c824
Thanks Nikita for this great clean-up work. Looking forward to see more contributions from you in the future.
This change introduces https://bugs.eclipse.org/bugs/show_bug.cgi?id=538381 I added the explanations there. I can fix it if original author is unavailable / unwilling, if someone vouches to take care of my patches.
The bug is introduced by this commit: Bug 531097 - [Win32] Remove unnecessary string manipulation
