[PATCH] D113645: [clangd] Allow Unix config paths on Darwin

[Keith Smiley via Phabricator via cfe-commits]

keith added a comment.

In D113645#3126652 , @kadircet wrote:

> I got a couple of concerns about the general ideas in the change. Even though 
> the convenience of using ~/.config/clangd/config.yaml seems nice, I think 
> it'll just end up creating confusion in the long run.
>
> According to various macOS developer documentation & QAs, canonical user 
> config directory for applications are `~/Library/Preferences` (see 
> https://developer.apple.com/library/archive/qa/qa1170/_index.html#//apple_ref/doc/uid/DTS10001702-CH1-SECTION3
>  and various others).

While this makes sense, I think the practical realities today are that 
`~/.config` is always preferred by unix style tools, even when on macOS.

> Regarding XDG_CONFIG_HOME, AFAICT macOS doesn't really implement `XDG Base 
> Directory Specification`, so querying its existence in that platform seems 
> controversial.

Yea I considered still omitting that one, but figured it was fair to simplify 
the compiler conditionals and assume that if someone had that variable set on 
macOS it was intentional. But I think it would be fine to exclude.

> Another concern is around multiple user config files. Clangd's config 
> mechanism actually implements overriding of previous config options, and user 
> config file is the most authoritative one and in the case of multiple such 
> config files it's unclear which one should have precedence. So i don't think 
> we should ever use "multiple" user config files.

Yea this makes sense. Theoretically we could check these "search paths" to 
determine if the configs actually existed to make that more clear, but with 
`--log=verbose` the order of operations is clear where clangd is searching for 
these so I think that shouldn't be a common issue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113645/new/

https://reviews.llvm.org/D113645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113645: [clangd] Allow Unix config paths on Darwin

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a subscriber: sammccall.
kadircet added a comment.

I got a couple of concerns about the general ideas in the change. Even though 
the convenience of using ~/.config/clangd/config.yaml seems nice, I think it'll 
just end up creating confusion in the long run.

According to various macOS developer documentation & QAs, canonical user config 
directory for applications are `~/Library/Preferences` (see 
https://developer.apple.com/library/archive/qa/qa1170/_index.html#//apple_ref/doc/uid/DTS10001702-CH1-SECTION3
 and various others).
Regarding XDG_CONFIG_HOME, AFAICT macOS doesn't really implement `XDG Base 
Directory Specification`, so querying its existence in that platform seems 
controversial.

Another concern is around multiple user config files. Clangd's config mechanism 
actually implements overriding of previous config options, and user config file 
is the most authoritative one and in the case of multiple such config files 
it's unclear which one should have precedence. So i don't think we should ever 
use "multiple" user config files.

cc @sammccall as he was the one that introduced this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113645/new/

https://reviews.llvm.org/D113645

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113645: [clangd] Allow Unix config paths on Darwin

[Keith Smiley via Phabricator via cfe-commits]

keith created this revision.
Herald added subscribers: dexonsmith, usaxena95, kadircet, arphaman, hiraditya.
keith requested review of this revision.
Herald added subscribers: cfe-commits, llvm-commits, MaskRay, ilya-biryukov.
Herald added projects: LLVM, clang-tools-extra.

It's common for CLIs on macOS to read configuration files from more
unix-standard directories with either dotfiles directly in ~ or files in
~/.config. Previously macOS clangd configuration only read from
~/Library/Preferences on macOS, now it respects the other Unix
directories with XDG_CONFIG_HOME or just ~/.config as a fallback. This
is implemented by changing user_config_directory to
user_config_directories which sets a vector of paths to search,
maintaining the previous order of operations.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113645

Files:
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  llvm/include/llvm/Support/Path.h
  llvm/lib/Support/Unix/Path.inc
  llvm/lib/Support/Windows/Path.inc
  llvm/unittests/Support/Path.cpp

Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -509,9 +509,10 @@
 TEST(Support, ConfigDirectoryWithEnv) {
   WithEnv Env("XDG_CONFIG_HOME", "/xdg/config");
 
-  SmallString<128> ConfigDir;
-  EXPECT_TRUE(path::user_config_directory(ConfigDir));
-  EXPECT_EQ("/xdg/config", ConfigDir);
+  std::vector ConfigDirs;
+  EXPECT_TRUE(path::user_config_directories(ConfigDirs));
+  std::vector Expected = {"/xdg/config"};
+  EXPECT_EQ(Expected, ConfigDirs);
 }
 
 TEST(Support, ConfigDirectoryNoEnv) {
@@ -521,9 +522,10 @@
   ASSERT_TRUE(path::home_directory(Fallback));
   path::append(Fallback, ".config");
 
-  SmallString<128> CacheDir;
-  EXPECT_TRUE(path::user_config_directory(CacheDir));
-  EXPECT_EQ(Fallback, CacheDir);
+  std::vector ConfigDirs;
+  EXPECT_TRUE(path::user_config_directories(ConfigDirs));
+  std::vector Expected = {std::string(Fallback)};
+  EXPECT_EQ(Expected, ConfigDirs);
 }
 
 TEST(Support, CacheDirectoryWithEnv) {
@@ -549,13 +551,41 @@
 
 #ifdef __APPLE__
 TEST(Support, ConfigDirectory) {
-  SmallString<128> Fallback;
-  ASSERT_TRUE(path::home_directory(Fallback));
-  path::append(Fallback, "Library/Preferences");
+  WithEnv Env("XDG_CONFIG_HOME", nullptr);
 
-  SmallString<128> ConfigDir;
-  EXPECT_TRUE(path::user_config_directory(ConfigDir));
-  EXPECT_EQ(Fallback, ConfigDir);
+  SmallString<128> HomerDir;
+  ASSERT_TRUE(path::home_directory(HomerDir));
+
+  SmallString<128> Preferences = HomerDir;
+  path::append(Preferences, "Library/Preferences");
+
+  SmallString<128> ConfigDir = HomerDir;
+  path::append(ConfigDir, ".config");
+
+  std::vector ConfigDirs;
+  EXPECT_TRUE(path::user_config_directories(ConfigDirs));
+  std::vector Expected = {std::string(Preferences),
+   std::string(ConfigDir)};
+  EXPECT_TRUE(Expected == ConfigDirs);
+}
+
+TEST(Support, XDGConfigDirectory) {
+  WithEnv Env("XDG_CONFIG_HOME", "/xdg/config");
+
+  SmallString<128> HomerDir;
+  ASSERT_TRUE(path::home_directory(HomerDir));
+
+  SmallString<128> Preferences = HomerDir;
+  path::append(Preferences, "Library/Preferences");
+
+  SmallString<128> ConfigDir = HomerDir;
+  path::append(ConfigDir, ".config");
+
+  std::vector ConfigDirs;
+  EXPECT_TRUE(path::user_config_directories(ConfigDirs));
+  std::vector Expected = {std::string(Preferences), "/xdg/config",
+   std::string(ConfigDir)};
+  EXPECT_TRUE(Expected == ConfigDirs);
 }
 #endif
 
Index: llvm/lib/Support/Windows/Path.inc
===
--- llvm/lib/Support/Windows/Path.inc
+++ llvm/lib/Support/Windows/Path.inc
@@ -1434,10 +1434,13 @@
   return getKnownFolderPath(FOLDERID_Profile, result);
 }
 
-bool user_config_directory(SmallVectorImpl ) {
+bool user_config_directories(std::vector ) {
   // Either local or roaming appdata may be suitable in some cases, depending
   // on the data. Local is more conservative, Roaming may not always be correct.
-  return getKnownFolderPath(FOLDERID_LocalAppData, result);
+  SmallString result;
+  bool found = getKnownFolderPath(FOLDERID_LocalAppData, );
+  results.push_back(result.c_str());
+  return found;
 }
 
 bool cache_directory(SmallVectorImpl ) {
Index: llvm/lib/Support/Unix/Path.inc
===
--- llvm/lib/Support/Unix/Path.inc
+++ llvm/lib/Support/Unix/Path.inc
@@ -1363,27 +1363,34 @@
   return false;
 }
 
-bool user_config_directory(SmallVectorImpl ) {
+bool user_config_directories(std::vector ) {
+  bool found = false;
+  SmallString<128> result;
 #ifdef __APPLE__
   // Mac: ~/Library/Preferences/
   if (home_directory(result)) {
 append(result, "Library", "Preferences");
-return true;
+results.push_back(result.c_str());
+found = true;
   }
-#else
+#endif
+