Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 350587 - [Scanner Discovery] The CygpathTranslator should check if the path that it is translating is in fact a Cygwin path
Summary: [Scanner Discovery] The CygpathTranslator should check if the path that it is...
Status: RESOLVED FIXED
Alias: None
Product: CDT
Classification: Tools
Component: cdt-build (show other bugs)
Version: 8.0   Edit
Hardware: PC All
: P3 normal (vote)
Target Milestone: 7.0.3   Edit
Assignee: Andrew Gvozdev CLA
QA Contact: Andrew Gvozdev CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-28 10:18 EDT by Michael Lindo CLA
Modified: 2011-07-11 17:13 EDT (History)
3 users (show)

See Also:


Attachments
Only translate paths that contain cygwin, mingw and cygdrive (1.60 KB, patch)
2011-06-28 10:20 EDT, Michael Lindo CLA
angvoz.dev: iplog-
Details | Diff
Allow some items of PerProjectSICollector to be extended (2.78 KB, patch)
2011-07-06 15:50 EDT, Michael Lindo CLA
vivkong: iplog+
Details | Diff
Extended PerProejctSICollector with a class that skips cygwin path translation (6.09 KB, patch)
2011-07-06 15:54 EDT, Michael Lindo CLA
angvoz.dev: iplog-
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Lindo CLA 2011-06-28 10:18:47 EDT
Build Identifier: 

Remote paths for remote managed build GCC projects won't show up in the project. The simple local OS check that is currently there is insufficient when dealing with remote projects.

Reproducible: Always
Comment 1 Michael Lindo CLA 2011-06-28 10:20:47 EDT
Created attachment 198735 [details]
Only translate paths that contain cygwin, mingw and cygdrive
Comment 2 Andrew Gvozdev CLA 2011-06-28 12:12:06 EDT
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?
Comment 3 Michael Lindo CLA 2011-06-28 15:31:24 EDT
(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.
Comment 4 Andrew Gvozdev CLA 2011-06-28 17:03:02 EDT
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?
Comment 5 Michael Lindo CLA 2011-06-28 20:57:57 EDT
(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.
Comment 6 Andrew Gvozdev CLA 2011-06-29 00:43:59 EDT
(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?
Comment 7 Doug Schaefer CLA 2011-06-29 11:00:46 EDT
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.
Comment 8 Andrew Gvozdev CLA 2011-06-29 11:21:43 EDT
(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.
Comment 9 Chris Recoskie CLA 2011-06-29 11:53:53 EDT
(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).
Comment 10 Andrew Gvozdev CLA 2011-06-29 12:34:40 EDT
(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.
Comment 11 Doug Schaefer CLA 2011-06-29 14:22:08 EDT
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.
Comment 12 Michael Lindo CLA 2011-07-06 15:50:57 EDT
Created attachment 199205 [details]
Allow some items of PerProjectSICollector to be extended
Comment 13 Michael Lindo CLA 2011-07-06 15:54:30 EDT
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.
Comment 14 Andrew Gvozdev CLA 2011-07-06 16:13:29 EDT
(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.
Comment 15 Michael Lindo CLA 2011-07-06 18:39:26 EDT
(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.
Comment 16 Andrew Gvozdev CLA 2011-07-07 16:45:54 EDT
(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 17 Michael Lindo CLA 2011-07-11 15:14:36 EDT
Comment on attachment 199207 [details]
Extended PerProejctSICollector with a class that skips cygwin path translation

Moved to RDT
Comment 18 Vivian Kong CLA 2011-07-11 16:05:58 EDT
(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!
Comment 19 Andrew Gvozdev CLA 2011-07-11 17:13:47 EDT
(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