[PATCH] gdb/gdbsupport: Use LOCALAPPDATA for index cache on windows

[PATCH] gdb/gdbsupport: Use LOCALAPPDATA for index cache on windows

[Alexander Fedotov via Gdb-patches]

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

Hi all

When running GDB on Windows using native command line (not Msys2 or
another) I see nasty warning Couldn't determine a path for index
cached directory"

So I have decided to use  LOCALAPPDATA as a last resort.

Comments are welcomed.

Alex

[-- Attachment #2: 0001-gdb-gdbsupport-pathstuff.cc-Use-LOCALAPPDATA-environ.patch --]
[-- Type: text/x-patch, Size: 1706 bytes --]

From 9c78154311c6315e8aae627fada5c339e932f887 Mon Sep 17 00:00:00 2001
From: Alexander Fedotov <alfedotov@gmail.com>
Date: Tue, 8 Dec 2020 13:07:23 +0300
Subject: [PATCH] gdb/gdbsupport/pathstuff.cc: Use LOCALAPPDATA environment
 variable when running on Windows with native command line, otherwise nasty
 warning "Couldn't determine a path for index cached directory" appears.

---
 gdbsupport/ChangeLog    |  6 ++++++
 gdbsupport/pathstuff.cc | 10 ++++++++++
 2 files changed, 16 insertions(+)

diff --git a/gdbsupport/ChangeLog b/gdbsupport/ChangeLog
index 88a0413e8b..cc1035d0ae 100644
--- a/gdbsupport/ChangeLog
+++ b/gdbsupport/ChangeLog
@@ -1,3 +1,9 @@
+2020-12-08  Alexander Fedotov  <alfedotov@gmail.com>
+
+	* pathstuff.cc (get_standard_cache_dir): Use LOCALAPPDATA environment
+	variable when running on Windows with native command line, otherwise nasty
+	warning "Couldn't determine a path for index cached directory" appears.
+
 2020-12-01  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* tdesc.cc (print_xml_feature::visit): Print enum fields using
diff --git a/gdbsupport/pathstuff.cc b/gdbsupport/pathstuff.cc
index 311456720e..994ddff300 100644
--- a/gdbsupport/pathstuff.cc
+++ b/gdbsupport/pathstuff.cc
@@ -238,6 +238,16 @@ get_standard_cache_dir ()
       return string_printf ("%s/" HOME_CACHE_DIR "/gdb", abs.get ());
     }
 
+#ifdef _WIN32
+  const char *win_home = getenv ("LOCALAPPDATA");
+  if (win_home != NULL)
+    {
+      /* Make sure the path is absolute and tilde-expanded.  */
+      gdb::unique_xmalloc_ptr<char> abs (gdb_abspath (win_home));
+      return string_printf ("%s/" HOME_CACHE_DIR "/gdb", abs.get ());
+    }
+#endif
+    
   return {};
 }
 
-- 
2.25.1

Re: [PATCH] gdb/gdbsupport: Use LOCALAPPDATA for index cache on windows

[Simon Marchi via Gdb-patches]

On 2020-12-08 5:16 a.m., Alexander Fedotov via Gdb-patches wrote:
> Hi all
> 
> When running GDB on Windows using native command line (not Msys2 or
> another) I see nasty warning Couldn't determine a path for index
> cached directory"
> 
> So I have decided to use  LOCALAPPDATA as a last resort.
> 
> Comments are welcomed.
> 
> Alex
> 

Hi Alex,

I made a few changes to your patch:

- I adjusted you comment message a little bit and added a ChangeLog entry
- I Changed "#ifdef _WIN32" to "#ifdef WIN32", because we use the latter 
a few lines below.

My only question is about:

   return string_printf ("%s/" HOME_CACHE_DIR "/gdb", abs.get ());

That would give a final path like:

   C:\Users\Simon\AppData\Local\.cache\gdb

The ".cache" part is not typical in this directory.  Should we instead 
just have:

   C:\Users\Simon\AppData\Local\gdb

?

Simon

Re: [PATCH] gdb/gdbsupport: Use LOCALAPPDATA for index cache on windows

[Simon Marchi via Gdb-patches]

On 2020-12-08 9:46 a.m., Simon Marchi wrote:
> - I adjusted you comment message a little bit and added a ChangeLog entry

Ah sorry, I then saw you already had a ChangeLog entry.  I just 
shortened it a bit to:

	* pathstuff.cc (get_standard_cache_dir): Use LOCALAPPDATA
	environment variable when running on Windows.

Re: [PATCH] gdb/gdbsupport: Use LOCALAPPDATA for index cache on windows

[Alexander Fedotov via Gdb-patches]

Hi Simon

>>The ".cache" part is not typical in this directory.  Should we instead
>>just have:
>>
>>   C:\Users\Simon\AppData\Local\gdb

Well, I don't know if other GDB parts depend on the final name. I just
followed common convention. If there are no such dependencies so I
don't mind using your suggestion :)

On Tue, Dec 8, 2020 at 5:48 PM Simon Marchi <simon.marchi@polymtl.ca> wrote:
>
> On 2020-12-08 9:46 a.m., Simon Marchi wrote:
> > - I adjusted you comment message a little bit and added a ChangeLog entry
>
> Ah sorry, I then saw you already had a ChangeLog entry.  I just
> shortened it a bit to:
>
>         * pathstuff.cc (get_standard_cache_dir): Use LOCALAPPDATA
>         environment variable when running on Windows.

Re: [PATCH] gdb/gdbsupport: Use LOCALAPPDATA for index cache on windows

[Simon Marchi via Gdb-patches]

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

On 2020-12-08 9:51 a.m., Alexander Fedotov wrote:
> Well, I don't know if other GDB parts depend on the final name. I just
> followed common convention. If there are no such dependencies so I
> don't mind using your suggestion:)

GDB doesn't depend on a particular directory, I think it's better to 
follow the standards of the platform as much as possible.  Does the 
attached patch look ok to you?

Simon

[-- Attachment #2: 0001-gdbsupport-Use-LOCALAPPDATA-to-determine-cache-dir.patch --]
[-- Type: text/x-patch, Size: 1684 bytes --]

From 60a7223fdd196d540f87504833166f558c95c035 Mon Sep 17 00:00:00 2001
From: Alexander Fedotov <alfedotov@gmail.com>
Date: Tue, 8 Dec 2020 13:07:23 +0300
Subject: [PATCH] gdbsupport: Use LOCALAPPDATA to determine cache dir

Use the LOCALAPPDATA environment variable to determine the cache dir
when running on Windows with native command line, otherwise nasty
warning "Couldn't determine a path for index cached directory" appears.

Change-Id: I77903f9f0cb4743555866b8aea892cef55132589
---
 gdbsupport/ChangeLog    |  5 +++++
 gdbsupport/pathstuff.cc | 10 ++++++++++
 2 files changed, 15 insertions(+)

diff --git a/gdbsupport/ChangeLog b/gdbsupport/ChangeLog
index 88a0413e8b9f..417e44aff31d 100644
--- a/gdbsupport/ChangeLog
+++ b/gdbsupport/ChangeLog
@@ -1,3 +1,8 @@
+2020-12-08  Alexander Fedotov  <alfedotov@gmail.com>
+
+	* pathstuff.cc (get_standard_cache_dir): Use LOCALAPPDATA environment
+	variable when running on Windows.
+
 2020-12-01  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* tdesc.cc (print_xml_feature::visit): Print enum fields using
diff --git a/gdbsupport/pathstuff.cc b/gdbsupport/pathstuff.cc
index 311456720e4a..b23e6ed064cf 100644
--- a/gdbsupport/pathstuff.cc
+++ b/gdbsupport/pathstuff.cc
@@ -238,6 +238,16 @@ get_standard_cache_dir ()
       return string_printf ("%s/" HOME_CACHE_DIR "/gdb", abs.get ());
     }
 
+#ifdef WIN32
+  const char *win_home = getenv ("LOCALAPPDATA");
+  if (win_home != NULL)
+    {
+      /* Make sure the path is absolute and tilde-expanded.  */
+      gdb::unique_xmalloc_ptr<char> abs (gdb_abspath (win_home));
+      return string_printf ("%s/gdb", abs.get ());
+    }
+#endif
+    
   return {};
 }
 
-- 
2.29.2

Re: [PATCH] gdb/gdbsupport: Use LOCALAPPDATA for index cache on windows

[Alexander Fedotov via Gdb-patches]

On Tue, Dec 8, 2020 at 6:10 PM Simon Marchi <simon.marchi@polymtl.ca> wrote:
>
> On 2020-12-08 9:51 a.m., Alexander Fedotov wrote:
> > Well, I don't know if other GDB parts depend on the final name. I just
> > followed common convention. If there are no such dependencies so I
> > don't mind using your suggestion:)
>
> GDB doesn't depend on a particular directory, I think it's better to
> follow the standards of the platform as much as possible.  Does the
> attached patch look ok to you?
Yes, looks good to me. Thank you.
>
> Simon

Re: [PATCH] gdb/gdbsupport: Use LOCALAPPDATA for index cache on windows

[Simon Marchi via Gdb-patches]

On 2020-12-08 10:12 a.m., Alexander Fedotov wrote:
> On Tue, Dec 8, 2020 at 6:10 PM Simon Marchi <simon.marchi@polymtl.ca> wrote:
>>
>> On 2020-12-08 9:51 a.m., Alexander Fedotov wrote:
>>> Well, I don't know if other GDB parts depend on the final name. I just
>>> followed common convention. If there are no such dependencies so I
>>> don't mind using your suggestion:)
>>
>> GDB doesn't depend on a particular directory, I think it's better to
>> follow the standards of the platform as much as possible.  Does the
>> attached patch look ok to you?
> Yes, looks good to me. Thank you.

Thanks, I pushed it.

Note that this is a small enough patch that it doesn't require a 
copyright assignment.  But if you ever plan on submitting larger 
patches, you will need one:

https://sourceware.org/gdb/wiki/ContributionChecklist#FSF_copyright_Assignment

Simon

Re: [PATCH] gdb/gdbsupport: Use LOCALAPPDATA for index cache on windows

[Eli Zaretskii via Gdb-patches]

> Date: Tue, 8 Dec 2020 09:46:47 -0500
> From: Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>
> 
> 
> My only question is about:
> 
>    return string_printf ("%s/" HOME_CACHE_DIR "/gdb", abs.get ());
> 
> That would give a final path like:
> 
>    C:\Users\Simon\AppData\Local\.cache\gdb
> 
> The ".cache" part is not typical in this directory.  Should we instead 
> just have:
> 
>    C:\Users\Simon\AppData\Local\gdb
> 
> ?

IMO, it indeed should be in C:\Users\Simon\AppData\Local\gdb, as other
applications do.  Moreover, I think we should use APPDATA, not
LOCALAPPDATA, because it makes sense to be able to synchronize the
cache over the network if the computer joins some domain.  IOW, it
isn't necessarily local data, is it?

Thanks.

Re: [PATCH] gdb/gdbsupport: Use LOCALAPPDATA for index cache on windows

[Alexander Fedotov via Gdb-patches]

On Tue, Dec 8, 2020 at 6:31 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > Date: Tue, 8 Dec 2020 09:46:47 -0500
> > From: Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>
> >
> >
> > My only question is about:
> >
> >    return string_printf ("%s/" HOME_CACHE_DIR "/gdb", abs.get ());
> >
> > That would give a final path like:
> >
> >    C:\Users\Simon\AppData\Local\.cache\gdb
> >
> > The ".cache" part is not typical in this directory.  Should we instead
> > just have:
> >
> >    C:\Users\Simon\AppData\Local\gdb
> >
> > ?
>
> IMO, it indeed should be in C:\Users\Simon\AppData\Local\gdb, as other
> applications do.  Moreover, I think we should use APPDATA, not
> LOCALAPPDATA, because it makes sense to be able to synchronize the
> cache over the network if the computer joins some domain.  IOW, it
> isn't necessarily local data, is it?
IMO default behaviour should be restrictive and store user data
locally. If necessary user
can specify folder using .gdbinit or by commands explicitly.
I guess such a roaming user keeping his project under version control
along with makefiles
and .gdbinit.
>
> Thanks.

Re: [PATCH] gdb/gdbsupport: Use LOCALAPPDATA for index cache on windows

[Christian Biesinger via Gdb-patches]

On Wed, Dec 9, 2020 at 10:15 AM Alexander Fedotov via Gdb-patches
<gdb-patches@sourceware.org> wrote:
>
> On Tue, Dec 8, 2020 at 6:31 PM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > > Date: Tue, 8 Dec 2020 09:46:47 -0500
> > > From: Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>
> > >
> > >
> > > My only question is about:
> > >
> > >    return string_printf ("%s/" HOME_CACHE_DIR "/gdb", abs.get ());
> > >
> > > That would give a final path like:
> > >
> > >    C:\Users\Simon\AppData\Local\.cache\gdb
> > >
> > > The ".cache" part is not typical in this directory.  Should we instead
> > > just have:
> > >
> > >    C:\Users\Simon\AppData\Local\gdb
> > >
> > > ?
> >
> > IMO, it indeed should be in C:\Users\Simon\AppData\Local\gdb, as other
> > applications do.  Moreover, I think we should use APPDATA, not
> > LOCALAPPDATA, because it makes sense to be able to synchronize the
> > cache over the network if the computer joins some domain.  IOW, it
> > isn't necessarily local data, is it?
> IMO default behaviour should be restrictive and store user data
> locally. If necessary user
> can specify folder using .gdbinit or by commands explicitly.
> I guess such a roaming user keeping his project under version control
> along with makefiles
> and .gdbinit.

Is this only used for the index cache or also for other things? The
index cache probably makes sense to be in Local, since it can be
relatively big and caches should generally not roam. Other things may
make more sense in the roaming part (e.g. settings).

https://docs.microsoft.com/en-us/previous-versions/windows/apps/hh464917(v=win.10)?redirectedfrom=MSDN
does say "The OS limits the size of the app data that each app may
roam".

Christian