Bugzilla – Full Text Bug Listing
| Summary: | [Scanner Discovery] The CygpathTranslator should check if the path that it is translating is in fact a Cygwin path | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Michael Lindo <mlindo> | ||||||||
| Component: | cdt-build | Assignee: | Andrew Gvozdev <angvoz.dev> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | Andrew Gvozdev <angvoz.dev> | ||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | cdtdoug, recoskie, vivkong | ||||||||
| Version: | 8.0 | ||||||||||
| Target Milestone: | 7.0.3 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Michael Lindo
Created attachment 198735 [details]
Only translate paths that contain cygwin, mingw and cygdrive
Can you give an example of the paths that are not showing? Could you also give more details on what you are trying to do? (In reply to comment #2) > Can you give an example of the paths that are not showing? Could you also give > more details on what you are trying to do? The translator would prefix the remote paths with the local drive letter, i.e. "C:/" and make them invalid. Invalid paths are filtered out. This patch checks to make sure it's actually dealing with local Cygwin paths before doing any translation. Are you doing scanner discovery or what is it what you are doing? How are you making the remote paths valid on the local system? (In reply to comment #4) > Are you doing scanner discovery or what is it what you are doing? How are you > making the remote paths valid on the local system? Yes, I'm implementing the Remote GNU Toolchain in PTP (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=344348 for more information) and it was noticed that Include Paths were not showing. This item fixes that problem. (In reply to comment #3) > The translator would prefix the remote paths with the local drive letter, i.e. > "C:/" and make them invalid. Invalid paths are filtered out. > This patch checks to make sure it's actually dealing with local Cygwin paths > before doing any translation. First, your patch will break Cygwin translation. You are assuming that all Cygwin paths are having "cygwin" or "cygdrive" but it is not the only case. Cygwin also maps common unix paths, for example /usr/include -> C:/cygwin/usr/include. Second, if CygwinPathTranslator does not find a path and does not prepend the path with drive letter it gets removed from the list in the default Win SI collector (which is opposite of what you are saying). Are you using different collector? My dreams for cygwin support revolve around using the Eclipse File System to provide the translation between the paths. It's a half baked idea, but I hate having cygpath handling sprinkled in somewhat random places in the CDT. (In reply to comment #7) > My dreams for cygwin support revolve around using the Eclipse File System to > provide the translation between the paths. It's a half baked idea, but I hate > having cygpath handling sprinkled in somewhat random places in the CDT. I think we got infrastructure to do this. We have EFSExtensionProvider extension point which purpose is to translate EFS paths so we could inject cygwin translation logic to file:// schema or maybe introduce cygwin:/. This would require to sprinkle EFS in those places to replace cygwin hacks. (In reply to comment #8) > (In reply to comment #7) > > My dreams for cygwin support revolve around using the Eclipse File System to > > provide the translation between the paths. It's a half baked idea, but I hate > > having cygpath handling sprinkled in somewhat random places in the CDT. > I think we got infrastructure to do this. We have EFSExtensionProvider > extension point which purpose is to translate EFS paths so we could inject > cygwin translation logic to file:// schema or maybe introduce cygwin:/. This > would require to sprinkle EFS in those places to replace cygwin hacks. Having a cygwin:/ filesystem is an interesting idea... but I'd have to think about it, and it's kind of too big a change for point releases. Overriding behaviour for file:/ basically puts you back to where you started... assuming that because you're running a Windows client, that you must be running CygWin. Since the translator is constructed with an IProject, can we not ask the build system what toolchain is being used for the active configuration, and use that to determine whether or not to do CygWin stuff? I realize that you might not be building the active configuration, but right now the API doesn't have a way of specifying a configuration to translate for (or not translate for). (In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > My dreams for cygwin support revolve around using the Eclipse File System to > > > provide the translation between the paths. It's a half baked idea, but I > hate > > > having cygpath handling sprinkled in somewhat random places in the CDT. > > I think we got infrastructure to do this. We have EFSExtensionProvider > > extension point which purpose is to translate EFS paths so we could inject > > cygwin translation logic to file:// schema or maybe introduce cygwin:/. This > > would require to sprinkle EFS in those places to replace cygwin hacks. > Having a cygwin:/ filesystem is an interesting idea... but I'd have to think > about it, and it's kind of too big a change for point releases. > Overriding behaviour for file:/ basically puts you back to where you started... > assuming that because you're running a Windows client, that you must be running > CygWin. > Since the translator is constructed with an IProject, can we not ask the build > system what toolchain is being used for the active configuration, and use that > to determine whether or not to do CygWin stuff? I realize that you might not be > building the active configuration, but right now the API doesn't have a way of > specifying a configuration to translate for (or not translate for). There could be a few permutations which I don't think you could determine automatically. For example remote build on EFS system and local cygwin "filesystem" (let me stick to the term). Or remote build on cygwin and local mingw. Or some other combination. I think it should be controllable by user in properties/preferences. I see here 2 translations involved, one is for remote EFS system to system-agnostic URL, and another one for local "cygwin filesystem". Using cygwin "filesystem" locally is not really a property of a toolchain but workspace property (or FS schema property). So for example for this bug's task a user would assign build FS to the EFS (by selecting schema) and local FS as cygwin (or rather unselect to avoid conversions). I do not really suggest to implement all that for this particular bug, just throwing in some ideas for future. Yeah, I'm definitely more focused on Juno for a real fix. I'm not sure I'd make it automatic unless there's something we can do specific for the cygwin toolchain. Frankly, I'd just like cygwin to disappear. MinGW/MSYS gives you pretty much everything you need to develop Windows apps. Created attachment 199205 [details]
Allow some items of PerProjectSICollector to be extended
Created attachment 199207 [details]
Extended PerProejctSICollector with a class that skips cygwin path translation
I have extended PerProejctSICollector with a class that skips cygwin path translation. To be used until a "cygwin:/" filesystem is invented.
org.eclipse.ptp.rdt.managedbuilder.gnu.ui will call the extended "PerProjectSICollectorSkipCygTrans" class instead of "PerProjectSICollector".
To be reviewed by Vivian.
(In reply to comment #13) > Created attachment 199207 [details] > Extended PerProejctSICollector with a class that skips cygwin path translation I'll take a look at your patches. > I have extended PerProejctSICollector with a class that skips cygwin path > translation. To be used until a "cygwin:/" filesystem is invented. > org.eclipse.ptp.rdt.managedbuilder.gnu.ui will call the extended > "PerProjectSICollectorSkipCygTrans" class instead of "PerProjectSICollector". > > To be reviewed by Vivian. You'll have to work with me on this as I am the lead for this component. (In reply to comment #14) > (In reply to comment #13) > > Created attachment 199207 [details] > > Extended PerProejctSICollector with a class that skips cygwin path translation > I'll take a look at your patches. > > > I have extended PerProejctSICollector with a class that skips cygwin path > > translation. To be used until a "cygwin:/" filesystem is invented. > > org.eclipse.ptp.rdt.managedbuilder.gnu.ui will call the extended > > "PerProjectSICollectorSkipCygTrans" class instead of "PerProjectSICollector". > > > > To be reviewed by Vivian. > You'll have to work with me on this as I am the lead for this component. Thanks Andrew. This solution should side step the issues that were brought up before. I tried to keep the code duplication to a minimum and only extended the methods that are needed. (In reply to comment #12) > Created attachment 199205 [details] > Allow some items of PerProjectSICollector to be extended I am OK with this patch. Far from considering it the best solution it will let you do what you need to do, i.e. extend the class, and pretty cheap. (In reply to comment #13) > Created attachment 199207 [details] > Extended PerProejctSICollector with a class that skips cygwin path translation > I have extended PerProejctSICollector with a class that skips cygwin path > translation. To be used until a "cygwin:/" filesystem is invented. > org.eclipse.ptp.rdt.managedbuilder.gnu.ui will call the extended > "PerProjectSICollectorSkipCygTrans" class instead of "PerProjectSICollector". But this one I don't think should go to CDT. I suggest to add it to your plugin where you need to use that functionality. Is there a reason not to do so? Comment on attachment 199207 [details]
Extended PerProejctSICollector with a class that skips cygwin path translation
Moved to RDT
(In reply to comment #16) > (In reply to comment #12) > > Created attachment 199205 [details] > > Allow some items of PerProjectSICollector to be extended > I am OK with this patch. Far from considering it the best solution it will let > you do what you need to do, i.e. extend the class, and pretty cheap. Backported this patch to cdt_7_0. Thanks Mike. Andrew, can you please check this into cdt_8_0 and master for Mike? Thanks! (In reply to comment #18) > (In reply to comment #16) > > (In reply to comment #12) > > > Created attachment 199205 [details] > > > Allow some items of PerProjectSICollector to be extended > > I am OK with this patch. Far from considering it the best solution it will let > > you do what you need to do, i.e. extend the class, and pretty cheap. > Backported this patch to cdt_7_0. Thanks Mike. > > Andrew, can you please check this into cdt_8_0 and master for Mike? Thanks! Sure, committed on master and 8.0.1 |