RE: [patch] Shorten Windows path

RE: [patch] Shorten Windows path

[Joey Ye]

Ping

> -----Original Message-----
> From: Joey Ye [mailto:joey.ye@arm.com]
> Sent: 19 February 2014 15:45
> To: gcc-patches@gcc.gnu.org; Ian Lance Taylor (iant@google.com)
> Subject: [patch] Shorten Windows path
> 
> Max length of path on Windows is 255, which is easy to exceed in a
> complicated project. Ultimate solution may be complex but canonizing the
> path and skipping the ".."s in path is helpful.
> 
> Relative discussion in gcc-patches:
> http://gcc.gnu.org/ml/gcc-patches/2013-11/msg00582.html
> 
> OK to trunk stage 1?
> 
> ChangeLog.libcpp:
>     * files.c (find_file_in_dir): Always try to shorten for DOS.
> 
> diff --git a/libcpp/files.c b/libcpp/files.c index 7e88778..9dcc71f 100644
> --- a/libcpp/files.c
> +++ b/libcpp/files.c
> @@ -386,9 +386,18 @@ find_file_in_dir (cpp_reader *pfile, _cpp_file *file,
> bool *invalid_pch)
>        hashval_t hv;
>        char *copy;
>        void **pp;
> +      bool do_canonical;
> 
> +#ifdef HAVE_DOS_BASED_FILE_SYSTEM
> +      /* For DOS based file system, we always try to shorten file path
> +       * to as it has a shorter constraint on max path length.  */
> +      do_canonical = true;
> +#else
>        /* We try to canonicalize system headers.  */
> -      if (CPP_OPTION (pfile, canonical_system_headers) &&
file->dir->sysp)
> +      do_canonical = (CPP_OPTION (pfile, canonical_system_headers)
> +                      && file->dir->sysp); #endif
> +      if ( do_canonical )
>  	{
>  	  char * canonical_path = maybe_shorter_path (path);
>  	  if (canonical_path)

Re: [patch] Shorten Windows path

[Ian Lance Taylor]

On Tue, Mar 25, 2014 at 1:58 AM, Joey Ye <joey.ye@arm.com> wrote:
> Ping

This code looks different on mainline.

Writing "if ( do_canonical )" is not GCC style.

This patch does not respect the configure option
--disable-canonical-system-headers.

Also I personally don't actually know what the consequences would be.
Are there any downsides to canonicalizing header names?

Ian


>> -----Original Message-----
>> From: Joey Ye [mailto:joey.ye@arm.com]
>> Sent: 19 February 2014 15:45
>> To: gcc-patches@gcc.gnu.org; Ian Lance Taylor (iant@google.com)
>> Subject: [patch] Shorten Windows path
>>
>> Max length of path on Windows is 255, which is easy to exceed in a
>> complicated project. Ultimate solution may be complex but canonizing the
>> path and skipping the ".."s in path is helpful.
>>
>> Relative discussion in gcc-patches:
>> http://gcc.gnu.org/ml/gcc-patches/2013-11/msg00582.html
>>
>> OK to trunk stage 1?
>>
>> ChangeLog.libcpp:
>>     * files.c (find_file_in_dir): Always try to shorten for DOS.
>>
>> diff --git a/libcpp/files.c b/libcpp/files.c index 7e88778..9dcc71f 100644
>> --- a/libcpp/files.c
>> +++ b/libcpp/files.c
>> @@ -386,9 +386,18 @@ find_file_in_dir (cpp_reader *pfile, _cpp_file *file,
>> bool *invalid_pch)
>>        hashval_t hv;
>>        char *copy;
>>        void **pp;
>> +      bool do_canonical;
>>
>> +#ifdef HAVE_DOS_BASED_FILE_SYSTEM
>> +      /* For DOS based file system, we always try to shorten file path
>> +       * to as it has a shorter constraint on max path length.  */
>> +      do_canonical = true;
>> +#else
>>        /* We try to canonicalize system headers.  */
>> -      if (CPP_OPTION (pfile, canonical_system_headers) &&
> file->dir->sysp)
>> +      do_canonical = (CPP_OPTION (pfile, canonical_system_headers)
>> +                      && file->dir->sysp); #endif
>> +      if ( do_canonical )
>>       {
>>         char * canonical_path = maybe_shorter_path (path);
>>         if (canonical_path)
>
>

RE: [patch] Shorten Windows path

[Joey Ye]

Ian, thanks for your comments. Please find answers and new version below:

> -----Original Message-----
> From: Ian Lance Taylor [mailto:iant@google.com]
> Sent: 25 March 2014 21:09
> To: Joey Ye
> Cc: gcc-patches
> Subject: Re: [patch] Shorten Windows path
> 
> On Tue, Mar 25, 2014 at 1:58 AM, Joey Ye <joey.ye@arm.com> wrote:
> > Ping
> 
> This code looks different on mainline.
> 
> Writing "if ( do_canonical )" is not GCC style.
Fixed
> 
> This patch does not respect the configure option --disable-canonical-system-
> headers.
Solved by put is under the control of default ENABLE_CANONICAL_SYSTEM_HEADERS
> 
> Also I personally don't actually know what the consequences would be.
> Are there any downsides to canonicalizing header names?
Since 4.8 system headers are by default canonicalized. This version only additionally canonical non-system headers. I can't think of any downsides.

> 
> Ian

ChangeLog.libcpp:

    * files.c (find_file_in_dir): Always try to shorten for DOS non-system headers.
    * init.c (ENABLE_CANONICAL_SYSTEM_HEADERS): Default enabled for DOS.

diff --git a/libcpp/files.c b/libcpp/files.c
index 7e88778..ad68682 100644
--- a/libcpp/files.c
+++ b/libcpp/files.c
@@ -387,8 +387,14 @@ find_file_in_dir (cpp_reader *pfile, _cpp_file *file, bool *invalid_pch)
       char *copy;
       void **pp;
 
-      /* We try to canonicalize system headers.  */
-      if (CPP_OPTION (pfile, canonical_system_headers) && file->dir->sysp)
+      /* We try to canonicalize system headers.  For DOS based file
+       * system, we always try to shorten non-system headers, as DOS
+       * has a tighter constraint on max path length.  */
+      if (CPP_OPTION (pfile, canonical_system_headers) && file->dir->sysp
+#ifdef HAVE_DOS_BASED_FILE_SYSTEM
+	  || !file->dir->sysp
+#endif
+	 )
 	{
 	  char * canonical_path = maybe_shorter_path (path);
 	  if (canonical_path)
diff --git a/libcpp/init.c b/libcpp/init.c
index f10413a..b809515 100644
--- a/libcpp/init.c
+++ b/libcpp/init.c
@@ -27,8 +27,12 @@ along with this program; see the file COPYING3.  If not see
 #include "filenames.h"
 
 #ifndef ENABLE_CANONICAL_SYSTEM_HEADERS
+#ifdef HAVE_DOS_BASED_FILE_SYSTEM
+#define ENABLE_CANONICAL_SYSTEM_HEADERS 1
+#else
 #define ENABLE_CANONICAL_SYSTEM_HEADERS 0
 #endif
+#endif
 
 static void init_library (void);
 static void mark_named_operators (cpp_reader *, int);

RE: [patch] Shorten Windows path

[Joey Ye]

Ping

> -----Original Message-----
> From: Joey Ye [mailto:joey.ye@arm.com]
> Sent: Tuesday, April 01, 2014 6:18 PM
> To: 'Ian Lance Taylor'
> Cc: gcc-patches
> Subject: RE: [patch] Shorten Windows path
> 
> Ian, thanks for your comments. Please find answers and new version below:
> 
> > -----Original Message-----
> > From: Ian Lance Taylor [mailto:iant@google.com]
> > Sent: 25 March 2014 21:09
> > To: Joey Ye
> > Cc: gcc-patches
> > Subject: Re: [patch] Shorten Windows path
> >
> > On Tue, Mar 25, 2014 at 1:58 AM, Joey Ye <joey.ye@arm.com> wrote:
> > > Ping
> >
> > This code looks different on mainline.
> >
> > Writing "if ( do_canonical )" is not GCC style.
> Fixed
> >
> > This patch does not respect the configure option --disable-canonical-
> system-
> > headers.
> Solved by put is under the control of default
> ENABLE_CANONICAL_SYSTEM_HEADERS
> >
> > Also I personally don't actually know what the consequences would be.
> > Are there any downsides to canonicalizing header names?
> Since 4.8 system headers are by default canonicalized. This version only
> additionally canonical non-system headers. I can't think of any downsides.
> 
> >
> > Ian
> 
> ChangeLog.libcpp:
> 
>     * files.c (find_file_in_dir): Always try to shorten for DOS non-system
> headers.
>     * init.c (ENABLE_CANONICAL_SYSTEM_HEADERS): Default enabled for DOS.
> 
> diff --git a/libcpp/files.c b/libcpp/files.c
> index 7e88778..ad68682 100644
> --- a/libcpp/files.c
> +++ b/libcpp/files.c
> @@ -387,8 +387,14 @@ find_file_in_dir (cpp_reader *pfile, _cpp_file *file,
> bool *invalid_pch)
>        char *copy;
>        void **pp;
> 
> -      /* We try to canonicalize system headers.  */
> -      if (CPP_OPTION (pfile, canonical_system_headers) && file->dir->sysp)
> +      /* We try to canonicalize system headers.  For DOS based file
> +       * system, we always try to shorten non-system headers, as DOS
> +       * has a tighter constraint on max path length.  */
> +      if (CPP_OPTION (pfile, canonical_system_headers) && file->dir->sysp
> +#ifdef HAVE_DOS_BASED_FILE_SYSTEM
> +	  || !file->dir->sysp
> +#endif
> +	 )
>  	{
>  	  char * canonical_path = maybe_shorter_path (path);
>  	  if (canonical_path)
> diff --git a/libcpp/init.c b/libcpp/init.c
> index f10413a..b809515 100644
> --- a/libcpp/init.c
> +++ b/libcpp/init.c
> @@ -27,8 +27,12 @@ along with this program; see the file COPYING3.  If not
> see
>  #include "filenames.h"
> 
>  #ifndef ENABLE_CANONICAL_SYSTEM_HEADERS
> +#ifdef HAVE_DOS_BASED_FILE_SYSTEM
> +#define ENABLE_CANONICAL_SYSTEM_HEADERS 1
> +#else
>  #define ENABLE_CANONICAL_SYSTEM_HEADERS 0
>  #endif
> +#endif
> 
>  static void init_library (void);
>  static void mark_named_operators (cpp_reader *, int);

Re: [patch] Shorten Windows path

[Ian Lance Taylor]

On Fri, Apr 25, 2014 at 3:16 AM, Joey Ye <joey.ye@arm.com> wrote:
> Ping

To be clear, I am not a libcpp maintainer and I don't plan to approve
this patch.  This should be reviewed by a libcpp maintainer or a C or
C++ frontend maintainer.

Ian


>> -----Original Message-----
>> From: Joey Ye [mailto:joey.ye@arm.com]
>> Sent: Tuesday, April 01, 2014 6:18 PM
>> To: 'Ian Lance Taylor'
>> Cc: gcc-patches
>> Subject: RE: [patch] Shorten Windows path
>>
>> Ian, thanks for your comments. Please find answers and new version below:
>>
>> > -----Original Message-----
>> > From: Ian Lance Taylor [mailto:iant@google.com]
>> > Sent: 25 March 2014 21:09
>> > To: Joey Ye
>> > Cc: gcc-patches
>> > Subject: Re: [patch] Shorten Windows path
>> >
>> > On Tue, Mar 25, 2014 at 1:58 AM, Joey Ye <joey.ye@arm.com> wrote:
>> > > Ping
>> >
>> > This code looks different on mainline.
>> >
>> > Writing "if ( do_canonical )" is not GCC style.
>> Fixed
>> >
>> > This patch does not respect the configure option --disable-canonical-
>> system-
>> > headers.
>> Solved by put is under the control of default
>> ENABLE_CANONICAL_SYSTEM_HEADERS
>> >
>> > Also I personally don't actually know what the consequences would be.
>> > Are there any downsides to canonicalizing header names?
>> Since 4.8 system headers are by default canonicalized. This version only
>> additionally canonical non-system headers. I can't think of any downsides.
>>
>> >
>> > Ian
>>
>> ChangeLog.libcpp:
>>
>>     * files.c (find_file_in_dir): Always try to shorten for DOS non-system
>> headers.
>>     * init.c (ENABLE_CANONICAL_SYSTEM_HEADERS): Default enabled for DOS.
>>
>> diff --git a/libcpp/files.c b/libcpp/files.c
>> index 7e88778..ad68682 100644
>> --- a/libcpp/files.c
>> +++ b/libcpp/files.c
>> @@ -387,8 +387,14 @@ find_file_in_dir (cpp_reader *pfile, _cpp_file *file,
>> bool *invalid_pch)
>>        char *copy;
>>        void **pp;
>>
>> -      /* We try to canonicalize system headers.  */
>> -      if (CPP_OPTION (pfile, canonical_system_headers) && file->dir->sysp)
>> +      /* We try to canonicalize system headers.  For DOS based file
>> +       * system, we always try to shorten non-system headers, as DOS
>> +       * has a tighter constraint on max path length.  */
>> +      if (CPP_OPTION (pfile, canonical_system_headers) && file->dir->sysp
>> +#ifdef HAVE_DOS_BASED_FILE_SYSTEM
>> +       || !file->dir->sysp
>> +#endif
>> +      )
>>       {
>>         char * canonical_path = maybe_shorter_path (path);
>>         if (canonical_path)
>> diff --git a/libcpp/init.c b/libcpp/init.c
>> index f10413a..b809515 100644
>> --- a/libcpp/init.c
>> +++ b/libcpp/init.c
>> @@ -27,8 +27,12 @@ along with this program; see the file COPYING3.  If not
>> see
>>  #include "filenames.h"
>>
>>  #ifndef ENABLE_CANONICAL_SYSTEM_HEADERS
>> +#ifdef HAVE_DOS_BASED_FILE_SYSTEM
>> +#define ENABLE_CANONICAL_SYSTEM_HEADERS 1
>> +#else
>>  #define ENABLE_CANONICAL_SYSTEM_HEADERS 0
>>  #endif
>> +#endif
>>
>>  static void init_library (void);
>>  static void mark_named_operators (cpp_reader *, int);
>
>

Re: [patch] Shorten Windows path

[Jeff Law]

On 04/25/14 04:16, Joey Ye wrote:
> Ping
I've spoke with Kai a bit about this and he thinks it's appropriate and 
desirable to shorten paths on these kinds of filesystems.

Ok for the trunk.

Thanks and sorry for the wait,
Jeff

Re: [patch] Shorten Windows path

[Meador Inge]

Hi,

On 04/01/2014 05:17 AM, Joey Ye wrote:

> diff --git a/libcpp/files.c b/libcpp/files.c
> index 7e88778..ad68682 100644
> --- a/libcpp/files.c
> +++ b/libcpp/files.c
> @@ -387,8 +387,14 @@ find_file_in_dir (cpp_reader *pfile, _cpp_file *file, bool *invalid_pch)
>        char *copy;
>        void **pp;
>  
> -      /* We try to canonicalize system headers.  */
> -      if (CPP_OPTION (pfile, canonical_system_headers) && file->dir->sysp)
> +      /* We try to canonicalize system headers.  For DOS based file
> +       * system, we always try to shorten non-system headers, as DOS
> +       * has a tighter constraint on max path length.  */
> +      if (CPP_OPTION (pfile, canonical_system_headers) && file->dir->sysp
> +#ifdef HAVE_DOS_BASED_FILE_SYSTEM
> +	  || !file->dir->sysp
> +#endif
> +	 )
>  	{
>  	  char * canonical_path = maybe_shorter_path (path);

Recently I have been working with a Windows GCC user that is running into the
minuscule Windows path limits discussed here.  I backported this patch to see
if it helped their case, but it didn't.

The problem we ran into is that 'lrealpath' is used in 'maybe_shorter_path' to
compute the canonical file path that has the relative portions removed.  On
Windows the implementation uses 'GetFullPathName'.  Unfortunately,
'GetFullPathName' suffers from the same MAX_PATH limit thus the
canonicalization fails (and from what I can tell the relative path that is
passed to 'GetFullPathName' is below the limit, but the joining of the current
working directory with the relative path name passed is not).

Did y'all run into anything like this?  Were other options to produce a
canonical path name discussed?  Nothing obvious jumped out at me.  Despite the
limits, using 'GetFullPathName' seems like the natural way to handle it because
it knows about all the various Windows file system quirks.

-- 
Meador Inge
CodeSourcery / Mentor Embedded

[patch] Shorten Windows path

[Joey Ye]

[-- Attachment #1: Type: text/plain, Size: 1289 bytes --]

Max length of path on Windows is 255, which is easy to exceed in a
complicated project. Ultimate solution may be complex but canonizing the
path and skipping the ".."s in path is helpful.

Relative discussion in gcc-patches:
http://gcc.gnu.org/ml/gcc-patches/2013-11/msg00582.html

OK to trunk stage 1?

ChangeLog.libcpp:
    * files.c (find_file_in_dir): Always try to shorten for DOS.

diff --git a/libcpp/files.c b/libcpp/files.c
index 7e88778..9dcc71f 100644
--- a/libcpp/files.c
+++ b/libcpp/files.c
@@ -386,9 +386,18 @@ find_file_in_dir (cpp_reader *pfile, _cpp_file *file,
bool *invalid_pch)
       hashval_t hv;
       char *copy;
       void **pp;
+      bool do_canonical;
 
+#ifdef HAVE_DOS_BASED_FILE_SYSTEM
+      /* For DOS based file system, we always try to shorten file path
+       * to as it has a shorter constraint on max path length.  */
+      do_canonical = true;
+#else
       /* We try to canonicalize system headers.  */
-      if (CPP_OPTION (pfile, canonical_system_headers) && file->dir->sysp)
+      do_canonical = (CPP_OPTION (pfile, canonical_system_headers)
+                      && file->dir->sysp);
+#endif
+      if ( do_canonical )
 	{
 	  char * canonical_path = maybe_shorter_path (path);
 	  if (canonical_path)

[-- Attachment #2: max_path_joey-140109-1.patch --]
[-- Type: application/octet-stream, Size: 866 bytes --]

diff --git a/libcpp/files.c b/libcpp/files.c
index 7e88778..9dcc71f 100644
--- a/libcpp/files.c
+++ b/libcpp/files.c
@@ -386,9 +386,18 @@ find_file_in_dir (cpp_reader *pfile, _cpp_file *file, bool *invalid_pch)
       hashval_t hv;
       char *copy;
       void **pp;
+      bool do_canonical;
 
+#ifdef HAVE_DOS_BASED_FILE_SYSTEM
+      /* For DOS based file system, we always try to shorten file path
+       * to as it has a shorter constraint on max path length.  */
+      do_canonical = true;
+#else
       /* We try to canonicalize system headers.  */
-      if (CPP_OPTION (pfile, canonical_system_headers) && file->dir->sysp)
+      do_canonical = (CPP_OPTION (pfile, canonical_system_headers)
+                      && file->dir->sysp);
+#endif
+      if ( do_canonical )
 	{
 	  char * canonical_path = maybe_shorter_path (path);
 	  if (canonical_path)