[Support] Extend sys::path with user_cache_directory function.

[chfast]

Summary

The new function sys::path::user_cache_directory tries to discover
a directory suitable for cache storage for current system user.

On Windows and Darwin it returns a path to system-specific user cache directory.

On Linux it follows XDG Base Directory Specification, what is:

• use non-empty $XDG_CACHE_HOME env var,
• use $HOME/.cache.

[aaron.ballman]

include/llvm/Support/Path.h

328 ↗

(On Diff #37571)

Should we be using @brief here like the rest of the file?

lib/Support/Unix/Path.inc

592 ↗

(On Diff #37571)

Please use / only for doxygen comments, otherwise.

lib/Support/Windows/Path.inc

773 ↗

(On Diff #37571)

How is this intended to be used? Is the caller expected to place a directory under the user_cache_directory() and use that, or are they expecting this to be a folder where they can dump cached data? I ask because putting files directly into the local AppData directory is not a particularly good thing to do on Windows.

[chfast]

Thanks for the review.

include/llvm/Support/Path.h

328 ↗

(On Diff #37571)

@brief has become redundant some time ago as Doxygen's option JAVADOC_AUTOBRIEF has been turned on. The brief is now first line until dot.

lib/Support/Unix/Path.inc

592 ↗

(On Diff #37571)

I actually wanted that to be exported as a Doxygen note. But that might not be the best idea as it is Unix specific.

lib/Support/Windows/Path.inc

773 ↗

(On Diff #37571)

That's right. An application should append its name to the returned path in general case. But that issues is already present on Darwin with system_temp_directory(false) that returns a path of "~/Library/Caches".

That should be definitely clarified in the docs.

I'm thinking about adding optional param to the function with a path that will be appended to the returned path. Then usage could be `user_cache_directory(result, "vendor/appname"). What do you think?

[chfast]

Aaron, could you comment my comments?

[aaron.ballman]

In D13801#271166, @chfast wrote:

Aaron, could you comment my comments?

Sorry for the delayed response; I'm in standards meetings this week, and vacation next, so I'm a bit hard to pin down.

include/llvm/Support/Path.h

328 ↗

(On Diff #37571)

I was speaking more for conformity within the same file. We usually try to match the style of the file, even if it's not strictly the correct thing to do style-wise.

I don't feel strongly. I just prefer consistency. Easier on the eyes. ;-)

lib/Support/Unix/Path.inc

592 ↗

(On Diff #37571)

Good to know. I would still recommend only using //.

lib/Support/Windows/Path.inc

773 ↗

(On Diff #37571)

I am not certain that's a good idea because then it embeds path separators in the string literal. If there's a way to pass that information that's abstracted (I'm not certain if something in fs would suffice or not), then I think it's a good idea.

[chfast]

• Update documenting comments.

[chfast]

lib/Support/Windows/Path.inc

775 ↗

(On Diff #37977)

It looks the best we can do is to repeat path::append interface that has optional path elements of type llvm::Twine. I don't think it is a good idea to do so. I've updated the doc comment of path::user_cache_directory and in my opinions it is enough.

[chfast]

Ping.

[• rafael]

btw, what will be using this?

[chfast]

In D13801#276308, @rafael wrote:

btw, what will be using this?

Personally I'm using it for keeping JIT cached objects. I believe it is more suitable for that case than system_temp_directory (and in case of MacOS is just more explicit).

[• rafael]

include/llvm/Support/Path.h

332 ↗

(On Diff #37977)

Maybe take a twine argument and do the appending in here? It is easy to forget to read documentation :-).

lib/Support/Unix/Path.inc

636 ↗

(On Diff #37977)

Should we drop the EraseOnReboot argument from this function? We would now have

user_cache_directory
system_temp_directory(true
system_temp_directory(false

When should each one be used?

[chfast]

include/llvm/Support/Path.h

332 ↗

(On Diff #37977)

That can be done. I would go for 2 Twine optional args to support <vendor>/<application> convention.

But that duplicates features of path::append and you still need to append a file name. The common pattern is:

if (sys::path::user_cache_directory(Dir)) {
  sys::path::append(Dir, "LLVM", "Kaleidoscope", CacheFileName);
}

and the above would be replaced with:

if (sys::path::user_cache_directory(Dir, "LLVM", "Kaleidoscope")) {
  sys::path::append(Dir, CacheFileName);
}

Which one is better?

lib/Support/Unix/Path.inc

636 ↗

(On Diff #37977)

user_cache_directory is similar to system_temp_directory(false) (level of similarity is OS-specific). We can drop system_temp_directory(false) then, but because it is the API change it's probably bad idea to do it at once.

In the end I would like to have:
user_cache_directory
system_temp_directory
and maybe user_temp_directory

I think that needs to be discussed. And we can agree on the final solution I'm happy to implement it.

[• rafael]

and the above would be replaced with:

if (sys::path::user_cache_directory(Dir, "LLVM", "Kaleidoscope")) {
  sys::path::append(Dir, CacheFileName);
}

Which one is better?

The second one if at least the first argument is mandatory. That way
we make sure we never write directly to the top level folder.

Comment at: lib/Support/Unix/Path.inc:636
@@ -585,3 +635,3 @@

void system_temp_directory(bool ErasedOnReboot, SmallVectorImpl<char> &Result) {

Result.clear();

---

rafael wrote:

Should we drop the EraseOnReboot argument from this function? We would now have

user_cache_directory
system_temp_directory(true
system_temp_directory(false

When should each one be used?

user_cache_directory is similar to system_temp_directory(false) (level of similarity is OS-specific). We can drop system_temp_directory(false) then, but because it is the API change it's probably bad idea to do it at once.

In the end I would like to have:
user_cache_directory
system_temp_directory
and maybe user_temp_directory

I think that needs to be discussed. And we can agree on the final solution I'm happy to implement it.

Awesome. Yes, multiple patches are better.

Cheers,
Rafael

[chfast]

Function path::user_cache_directory has been extended with params of additional paths to be appended to the resulting path. First of the params is mandatory.

[• rafael]

LGTM with the variable names changed to the llvm style (Path1, Path2, etc).

[aaron.ballman]

LGTM with a testing request.

unittests/Support/Path.cpp

340 ↗

(On Diff #38733)

It would be nice to have a test that ensures non-ASCII characters in the path do not get corrupted. I don't think there's a reasonable way to test that for the user_cache_directory itself, but the paths we append would at least give some peace of mind.

[chfast]

• Params and variables names capitalized.
• A unittest with Unicode names added.

llvm/trunk/include/llvm/Support/Path.h

 void system_temp_directory(bool erasedOnReboot, SmallVectorImpl<char> &result);
 
 /// @brief Get the user's home directory.
 ///
 /// @param result Holds the resulting path name.
 /// @result True if a home directory is set, false otherwise.
 bool home_directory(SmallVectorImpl<char> &result);
 
+/// @brief Get the user's cache directory.
+///
+/// Expect the resulting path to be a directory shared with other
+/// applications/services used by the user. Params \p Path1 to \p Path3 can be
+/// used to append additional directory names to the resulting path. Recommended
+/// pattern is <user_cache_directory>/<vendor>/<application>.
+///
+/// @param Result Holds the resulting path.
+/// @param Path1 Additional path to be appended to the user's cache directory
+/// path. "" can be used to append nothing.
+/// @param Path2 Second additional path to be appended.
+/// @param Path3 Third additional path to be appended.
+/// @result True if a cache directory path is set, false otherwise.
+bool user_cache_directory(SmallVectorImpl<char> &Result, const Twine &Path1,
+                          const Twine &Path2 = "", const Twine &Path3 = "");
+
 /// @brief Has root name?
 ///
 /// root_name != ""
 ///
 /// @param path Input path.
 /// @result True if the path has a root name, false otherwise.
 bool has_root_name(const Twine &path);
 

llvm/trunk/lib/Support/Path.cpp

 
 // Include the truly platform-specific parts.
 #if defined(LLVM_ON_UNIX)
 #include "Unix/Path.inc"
 #endif
 #if defined(LLVM_ON_WIN32)
 #include "Windows/Path.inc"
 #endif
+
+namespace llvm {
+namespace sys {
+namespace path {
+
+bool user_cache_directory(SmallVectorImpl<char> &Result, const Twine &Path1,
+                          const Twine &Path2, const Twine &Path3) {
+  if (getUserCacheDir(Result)) {
+    append(Result, Path1, Path2, Path3);
+    return true;
+  }
+  return false;
+}
+
+} // end namespace path
+} // end namsspace sys
+} // end namespace llvm

llvm/trunk/lib/Support/Unix/Path.inc

if (char *RequestedDir = getenv("HOME")) {
     result.clear();
     result.append(RequestedDir, RequestedDir + strlen(RequestedDir));
     return true;
   }
 
   return false;
 }
 
+namespace {
+bool getDarwinConfDir(bool TempDir, SmallVectorImpl<char> &Result) {
+  #if defined(_CS_DARWIN_USER_TEMP_DIR) && defined(_CS_DARWIN_USER_CACHE_DIR)
+  // On Darwin, use DARWIN_USER_TEMP_DIR or DARWIN_USER_CACHE_DIR.
+  // macros defined in <unistd.h> on darwin >= 9
+  int ConfName = TempDir ? _CS_DARWIN_USER_TEMP_DIR
+                         : _CS_DARWIN_USER_CACHE_DIR;
+  size_t ConfLen = confstr(ConfName, nullptr, 0);
+  if (ConfLen > 0) {
+    do {
+      Result.resize(ConfLen);
+      ConfLen = confstr(ConfName, Result.data(), Result.size());
+    } while (ConfLen > 0 && ConfLen != Result.size());
+
+    if (ConfLen > 0) {
+      assert(Result.back() == 0);
+      Result.pop_back();
+      return true;
+    }
+
+    Result.clear();
+  }
+  #endif
+  return false;
+}
+
+bool getUserCacheDir(SmallVectorImpl<char> &Result) {
+  // First try using XDS_CACHE_HOME env variable,
+  // as specified in XDG Base Directory Specification at
+  // http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html
+  if (const char *XdsCacheDir = std::getenv("XDS_CACHE_HOME")) {
+    Result.clear();
+    Result.append(XdsCacheDir, XdsCacheDir + strlen(XdsCacheDir));
+    return true;
+  }
+
+  // Try Darwin configuration query
+  if (getDarwinConfDir(false, Result))
+    return true;
+
+  // Use "$HOME/.cache" if $HOME is available
+  if (home_directory(Result)) {
+    append(Result, ".cache");
+    return true;
+  }
+
+  return false;
+}
+}
+
+
 static const char *getEnvTempDir() {
   // Check whether the temporary directory is specified by an environment
   // variable.
   const char *EnvironmentVariables[] = {"TMPDIR", "TMP", "TEMP", "TEMPDIR"};
   for (const char *Env : EnvironmentVariables) {
     if (const char *Dir = std::getenv(Env))
       return Dir;
   }
void system_temp_directory(bool ErasedOnReboot, SmallVectorImpl<char> &Result) {
   if (ErasedOnReboot) {
     // There is no env variable for the cache directory.
     if (const char *RequestedDir = getEnvTempDir()) {
       Result.append(RequestedDir, RequestedDir + strlen(RequestedDir));
       return;
     }
   }
 
-#if defined(_CS_DARWIN_USER_TEMP_DIR) && defined(_CS_DARWIN_USER_CACHE_DIR)
-  // On Darwin, use DARWIN_USER_TEMP_DIR or DARWIN_USER_CACHE_DIR.
-  // macros defined in <unistd.h> on darwin >= 9
-  int ConfName = ErasedOnReboot? _CS_DARWIN_USER_TEMP_DIR
-                               : _CS_DARWIN_USER_CACHE_DIR;
-  size_t ConfLen = confstr(ConfName, nullptr, 0);
-  if (ConfLen > 0) {
-    do {
-      Result.resize(ConfLen);
-      ConfLen = confstr(ConfName, Result.data(), Result.size());
-    } while (ConfLen > 0 && ConfLen != Result.size());
-
-    if (ConfLen > 0) {
-      assert(Result.back() == 0);
-      Result.pop_back();
-      return;
-    }
-
-    Result.clear();
-  }
-#endif
+  if (getDarwinConfDir(ErasedOnReboot, Result))
 
   const char *RequestedDir = getDefaultTempDir(ErasedOnReboot);
   Result.append(RequestedDir, RequestedDir + strlen(RequestedDir));
 }
 
 } // end namespace path
 
 } // end namespace sys
 } // end namespace llvm

llvm/trunk/lib/Support/Windows/Path.inc

bool getKnownFolderPath(KNOWNFOLDERID folderId, SmallVectorImpl<char> &result) {
   wchar_t *path = nullptr;
   if (::SHGetKnownFolderPath(folderId, KF_FLAG_CREATE, nullptr, &path) != S_OK)
     return false;
 
   bool ok = !UTF16ToUTF8(path, ::wcslen(path), result);
   ::CoTaskMemFree(path);
   return ok;
 }
+
+bool getUserCacheDir(SmallVectorImpl<char> &Result) {
+  return getKnownFolderPath(FOLDERID_LocalAppData, Result);
+}
 }
 
 bool home_directory(SmallVectorImpl<char> &result) {
   return getKnownFolderPath(FOLDERID_Profile, result);
 }
 
 static bool getTempDirEnvVar(const char *Var, SmallVectorImpl<char> &Res) {
   SmallVector<wchar_t, 128> NameUTF16;

llvm/trunk/unittests/Support/Path.cpp

#endif
   if (!expected.empty()) {
     SmallString<128> HomeDir;
     auto status = path::home_directory(HomeDir);
     EXPECT_TRUE(status);
     EXPECT_EQ(expected, HomeDir);
   }
 }
 
+TEST(Support, UserCacheDirectory) {
+  SmallString<13> CacheDir;
+  SmallString<20> CacheDir2;
+  auto Status = path::user_cache_directory(CacheDir, "");
+  EXPECT_TRUE(Status ^ CacheDir.empty());
+
+  if (Status) {
+    EXPECT_TRUE(path::user_cache_directory(CacheDir2, "")); // should succeed
+    EXPECT_EQ(CacheDir, CacheDir2); // and return same paths
+
+    EXPECT_TRUE(path::user_cache_directory(CacheDir, "A", "B", "file.c"));
+    auto It = path::rbegin(CacheDir);
+    EXPECT_EQ("file.c", *It);
+    EXPECT_EQ("B", *++It);
+    EXPECT_EQ("A", *++It);
+    auto ParentDir = *++It;
+
+    // Test Unicode: "<user_cache_dir>/(pi)r^2/aleth.0"
+    EXPECT_TRUE(path::user_cache_directory(CacheDir2, "\xCF\x80r\xC2\xB2",
+                                           "\xE2\x84\xB5.0"));
+    auto It2 = path::rbegin(CacheDir2);
+    EXPECT_EQ("\xE2\x84\xB5.0", *It2);
+    EXPECT_EQ("\xCF\x80r\xC2\xB2", *++It2);
+    auto ParentDir2 = *++It2;
+
+    EXPECT_EQ(ParentDir, ParentDir2);
+  }
+}
+
 class FileSystemTest : public testing::Test {
 protected:
   /// Unique temporary directory in which all created filesystem entities must
   /// be placed. It is removed at the end of each test (must be empty).
   SmallString<128> TestDirectory;
 
   void SetUp() override {
     ASSERT_NO_ERROR(