Bugzilla – Full Text Bug Listing
| Summary: | [terminal][local] Confusing error when trying to install Local Terminal on Windows from Helios | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] Target Management | Reporter: | Martin Oberhuber <mober.at+eclipse> | ||||||
| Component: | Terminal | Assignee: | Mirko Raner <mirko> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | Martin Oberhuber <mober.at+eclipse> | ||||||
| Severity: | major | ||||||||
| Priority: | P2 | CC: | eclipse, mirko, uwe.st | ||||||
| Version: | 3.2 | Flags: | mober.at+eclipse:
pmc_approved+
uwe.st: review+ |
||||||
| Target Milestone: | 3.2 RC4 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows XP | ||||||||
| Whiteboard: | |||||||||
| Bug Depends on: | 302196, 314193 | ||||||||
| Bug Blocks: | |||||||||
| Attachments: |
|
||||||||
|
Description
Martin Oberhuber
I think we need to take a totally different approach on this. My point is that the only reason why Local Terminal doesn't work on Windows is that the CDT's PTY doesn't work as expected on Windows. I expect the very same issue to happen on other architectures where CDT reports PTY.isSupported() == false Therefore, I think that we should enable / disable Terminal support more dynamically. Rather than putting a hard-coded restriction on the MANIFEST.MF (and feature.xml), we should always allow installing the bundle but when it comes to creating a new "Local Terminal" connection, inspect whether PTY.isSupported() returns true and if not, issue an error message and disable the connector. We have been using exactly the same strategy for the Serial Terminal Connector. Its RXTX dependency is optional, and when RXTX cannot be loaded, we issue the error message (by catching an Exception). Mirko, can you contribute a patch which provides this? - Quite frankly, given the Build Problems we had (and have) with putting the bundle restriction in, I think that this is the one and only way how we can possibly get local.terminal into Helios. Yes, I can put together a patch later today. It might be a close call for RC4, though. When exactly will the RC4 build start? Created attachment 171072 [details] Patch to disable the Local Terminal Connector if PTYs are not supported I followed the example that is set in SerialConnector, and the unavailability of the LocalTerminalConnector on certain platforms is now handled in the same fashion. As a minor improvement, I also replaced Format.format(...) with NLS.bind(...) as mentioned in bug 314195 comment 11. Created attachment 171095 [details]
Patch slightly improved
Great work Mirko! I have only slightly improved the patch:
- Shortened the error message to be brief and readable, yet informative.
- Using Platform.getOS() instead of System.getProperty()... because what's
relevant in terms of interpreting the error is avaialability of the CDT
fragment which uses the OSGi specs (org.eclipse.cdt.core.win32.x86).
- Capitalized the connector name ("Local Program")
- Fixed a spelling error in terminal.view ("Connector... is not available")
Oh, yes, and of course I removed the offending Eclipse-PlatformFilter in MANIFEST.MF. The important point here is that it's not the local terminal connector's fault if a connector is not available... that solely depends on availability of the native PTY library in the org.eclipse.cdt.core.os.arch fragment. I think that we cover this really nicely now. I need one committer's "review+" before I can commit this. Uwe? Michael? Comment on attachment 171072 [details]
Patch to disable the Local Terminal Connector if PTYs are not supported
Thanks for the quick review, Uwe - I have released the fix.
Released, please verify with the next I-build. *** Bug 315751 has been marked as a duplicate of this bug. *** (In reply to comment #4) > - Shortened the error message to be brief and readable, yet informative. Thanks, Martin. Writing short, concise, and properly spelled error or warning messages doesn't seem to be one of my strong points ;-) > - Using Platform.getOS() instead of System.getProperty()... because what's I actually used that initially, but I didn't like that it returns a short "moniker" instead of the real OS name as humans would write it. For example, on my Mac, the error message would say "macosx (x86_64) does not provide..." when I used Platform.getOS(), but "Mac OS X (x86_64) does not provide..." when I used the os.name property. I liked the latter option better, but it obviously doesn't make a big difference. Thanks for all the work! I'm glad we could finally resolve this. I'm glad you are glad ... but remember, we are only done when it's tested and verified. I want each 3.2RC4 fix to be verified with an actual install off the Helios repository and/or our download site. An interim TM site is ready for testing: http://download.eclipse.org/dsdp/tm/updates/3.2interim I'm right now preparing the download bits and contributing to Helios. |