[libc++] Make sure that temp_directory_path() doesn't return a path with a trailing slash

[ldionne]

Summary

This fixes https://llvm.org/PR45307.

[ldionne]

I'm eager to see what @EricWF thinks about this.

[EricWF]

Lets not start writing tests like this.

It breaks with the fundamental design goal of the test suite:

• c++ test.pass.cpp should be enough to compile most tests.
• ./a.out should be enough to run it.

Please add these tests to std/input.output/filesystems//fs.op.funcs/fs.op.temp_dir_path/temp_directory_path.pass.cpp.

[EricWF]

I don't necessarily agree.

First, no implementation currently has the behavior you describe.
Both libstdc++ and Boost filesystem return the environment variable as specified.

Also, on OS X, the TEMPDIR variable includes the trailing slash. If that's what the operating system does, why should we differ?

I also think there are some subtle differences in pathname resolution. Specifically around symlinks and when the basename doesn't exist.

For example, let's assume TEMPDIR is /this-directory-does-not-exist/. touch foo; cp foo $TEMPDIR fails because there is no such directory. Without the trailing slash it succeeds and copies foo to a new file called this-directory-does-not-exist.

In general trailing slashes are used to denote that the entity is a directory. I don't see a strong reason to differ.

But if you disagree, I'm willing to proceed with your fix, on the condition that you file a LWG issue about this proposing this behavior as standard.

[ldionne]

In D76798#1943906, @EricWF wrote:

Lets not start writing tests like this.

It breaks with the fundamental design goal of the test suite:

• c++ test.pass.cpp should be enough to compile most tests.
• ./a.out should be enough to run it.

Please add these tests to std/input.output/filesystems//fs.op.funcs/fs.op.temp_dir_path/temp_directory_path.pass.cpp.

First, this test would be a non-standard extension, so it needs to stay in libcxx/test/libcxx, not libcxx/test/std. And regarding using a .sh.cpp instead of a .pass.cpp, the only reason is that I wanted to avoid using ::setenv, which is not standard. But if we're fine with using ::setenv, then I agree a .pass.cpp is a lot simpler.

In D76798#1944023, @EricWF wrote:

I don't necessarily agree.

[...]

But if you disagree, I'm willing to proceed with your fix, on the condition that you file a LWG issue about this proposing this behavior as standard.

I replied on the bug report. I think I agree with you but I'd like to clarify a few things. Let's have the discussion on Bugzilla and I'll update this thread with the result of the discussion.

[ldionne]

Abandoning this -- see bug report for details.

libcxx/src/filesystem/operations.cpp

path __temp_directory_path(error_code* ec) {
   file_status st = detail::posix_stat(p, &m_ec);
   if (!status_known(st))
     return err.report(m_ec, "cannot access path \"%s\"", p);
 
   if (!exists(st) || !is_directory(st))
     return err.report(errc::not_a_directory, "path \"%s\" is not a directory",
                       p);
 
+  // https://llvm.org/PR45307:
+  // Avoid returning a temporary path with a trailing slash.
+  auto last_component = --p.end();
+  if (last_component->empty())
+    return p.parent_path();
   return p;
 }
 
 path __weakly_canonical(const path& p, error_code* ec) {
   ErrorHandler<path> err("weakly_canonical", ec, &p);
 
   if (p.empty())
     return __canonical("", ec);

libcxx/test/libcxx/input.output/filesystems/fs.op.funcs/fs.op.temp_dir_path/trailing-slash.sh.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// https://llvm.org/PR45307
+//
+// Make sure that temp_directory_path() doesn't return a path with a trailing
+// slash, even when one of the relevant environment variables contains a path
+// with a trailing slash.
+
+// UNSUPPORTED: c++98, c++03
+
+// The dylibs shipped until and including macosx10.15 did not contain the fix
+// for this issue, so they fail at runtime.
+// XFAIL: with_system_cxx_lib=macosx10.15
+// XFAIL: with_system_cxx_lib=macosx10.14
+// XFAIL: with_system_cxx_lib=macosx10.13
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
+
+// RUN: %build
+// RUN: mkdir -p "%t.dir"
+// RUN: %exec env TMPDIR="%t.dir/" %t.exe "%t.dir"
+// RUN: %exec env TMP="%t.dir/" %t.exe "%t.dir"
+// RUN: %exec env TEMP="%t.dir/" %t.exe "%t.dir"
+// RUN: %exec env TEMPDIR="%t.dir/" %t.exe "%t.dir"
+
+#include <cassert>
+#include <filesystem>
+
+int main(int, char** argv) {
+  std::filesystem::path expected_tmp = argv[1];
+  std::filesystem::path tmp = std::filesystem::temp_directory_path();
+  assert(tmp == expected_tmp);
+  return 0;
+}