[libcxx] [test] Fix filesystem permission tests for windows

[mstorsjo]

Summary

On Windows, the permission bits are mapped down to essentially only
two possible states; readonly or readwrite. Normalize the checked
permission bitmask to match what the implementation will return.

[mstorsjo]

libcxx/test/support/filesystem_test_helper.h

621

I guess it can be argued that we should normalize both operands, as it's not evident to the callers about which parameter is which. But by only normalizing the expectation, we keep the test a bit stronger. But I can be convinced otherwise too.

[• Quuxplusone]

LGTM % comment.

libcxx/test/support/filesystem_test_helper.h

621

If you're only normalizing one side, I'd name the function PermsNormalizeExpected (since you could perhaps have chosen to implement a PermsNormalizeActual instead). However, it's also a function with only one caller, so I'd actually inline it. I'd say

inline bool PermsEq(fs::perms Actual, fs::perms Expected) {
#ifdef _WIN32
  // On Windows, fs::perms maps down to one bit stored in the filesystem,
  // a boolean readonly flag.
  // Normalize the expected permissions to the format that is returned:
  // all files are read+exec for all users; writable files are writable for all users.
  Expected |= fs::perms::owner_read | fs::perms::group_read | fs::perms::others_read;
  Expected |= fs::perms::owner_exec | fs::perms::group_exec | fs::perms::others_exec;
  if (Expected & (fs::perms::owner_write | fs::perms::group_write | fs::perms::others_write))
    Expected |= fs::perms::owner_write | fs::perms::group_write | fs::perms::others_write;
#endif
  return Actual == Expected;
}

[mstorsjo]

libcxx/test/support/filesystem_test_helper.h

621

Sure, I can inline it.

If you're only normalizing one side, I'd name the function PermsNormalizeExpected (since you could perhaps have chosen to implement a PermsNormalizeActual instead).

Hmm, so no change wrt that as we'd inline it? Should we name the comparison function in a way to highlight this too, e.g. PermsEqNormalizeExpected (which is a mouthful...)? The tricky thing is that it's not clear to the callers which argument is which.

[• Quuxplusone]

libcxx/test/support/filesystem_test_helper.h

621

Meh. It has only a few callers, so IMHO the approach I presented for PermsEq is fine.
If you were going to rewrite it, though, the next thing I'd suggest would be

TEST_CHECK(pp == NormalizeExpectedPerms(TC.expected));

at the call-sites.

[mstorsjo]

libcxx/test/support/filesystem_test_helper.h

621

Ah, that's probably even clearer, I'll update to that form in a moment.

[mstorsjo]

Removed the unnecessary PermsEq, renamed the normalize function to NormalizeExpectedPerms.

[mstorsjo]

Ping for second approval

[mstorsjo]

@Mordante - does this look acceptable to you?

[Mordante]

LGTM, one small question.

libcxx/test/support/filesystem_test_helper.h

614

Is it specified which of the 3 write flags Windows sets when a file isn't read-only? Or should it set all 3 of them?

[mstorsjo]

libcxx/test/support/filesystem_test_helper.h

614

I'm not sure if there's a spec somewhere; MSVC's STL sets all three in sync, and libc++ does the same. As we check whether our normalized expected flags are equal to what the implementation sets, this test will show if an implementation returns something else.

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.copy_file/copy_file.pass.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
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03
 
-// XFAIL: LIBCXX-WINDOWS-FIXME
-
 // The string reported on errors changed, which makes those tests fail when run
 // against already-released libc++'s.
 // XFAIL: use_system_cxx_lib && x86_64-apple-macosx10.15
 
 // <filesystem>
 
 // bool copy_file(const path& from, const path& to);
 // bool copy_file(const path& from, const path& to, error_code& ec) noexcept;
TEST_CASE(test_attributes_get_copied) {
   const path dest = env.make_env_path("file2");
   (void)status(file);
   perms new_perms = perms::owner_read;
   permissions(file, new_perms);
   std::error_code ec = GetTestEC();
   TEST_REQUIRE(fs::copy_file(file, dest, ec) == true);
   TEST_CHECK(!ec);
   auto new_st = status(dest);
-  TEST_CHECK(new_st.permissions() == new_perms);
+  TEST_CHECK(new_st.permissions() == NormalizeExpectedPerms(new_perms));
 }
 
 TEST_CASE(copy_dir_test) {
   scoped_test_env env;
   const path file = env.create_file("file1", 42);
   const path dest = env.create_dir("dir1");
   std::error_code ec = GetTestEC();
   TEST_CHECK(fs::copy_file(file, dest, ec) == false);

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.permissions/permissions.pass.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
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03
 
-// XFAIL: LIBCXX-WINDOWS-FIXME
-
 // <filesystem>
 
 // void permissions(const path& p, perms prms,
 //                  perm_options opts = perm_options::replace);
 // void permissions(const path& p, perms prms, std::error_code& ec) noexcept;
 // void permissions(const path& p, perms prms, perm_options opts, std::error_code);
 
 
TEST_CASE(basic_permissions_test)
     };
     for (auto const& TC : cases) {
         TEST_CHECK(status(TC.p).permissions() != TC.expected);
         {
           std::error_code ec = GetTestEC();
           permissions(TC.p, TC.set_perms, TC.opts, ec);
           TEST_CHECK(!ec);
           auto pp = status(TC.p).permissions();
-          TEST_CHECK(pp == TC.expected);
+          TEST_CHECK(pp == NormalizeExpectedPerms(TC.expected));
         }
         if (TC.opts == perm_options::replace) {
           std::error_code ec = GetTestEC();
           permissions(TC.p, TC.set_perms, ec);
           TEST_CHECK(!ec);
           auto pp = status(TC.p).permissions();
-          TEST_CHECK(pp == TC.expected);
+          TEST_CHECK(pp == NormalizeExpectedPerms(TC.expected));
         }
     }
 }
 
+#ifndef _WIN32
+// This test isn't currently meaningful on Windows; the Windows file
+// permissions visible via std::filesystem doesn't show any difference
+// between owner/group/others.
 TEST_CASE(test_no_resolve_symlink_on_symlink)
 {
     scoped_test_env env;
     const path file = env.create_file("file", 42);
     const path sym = env.create_symlink(file, "sym");
     const auto file_perms = status(file).permissions();
 
     struct TestCase {
#endif
         if (expected_ec)
             TEST_CHECK(ErrorIs(ec, static_cast<std::errc>(expected_ec.value())));
         else
             TEST_CHECK(!ec);
         TEST_CHECK(status(file).permissions() == file_perms);
         TEST_CHECK(symlink_status(sym).permissions() == expected_link_perms);
     }
 }
+#endif
 
 TEST_SUITE_END()

libcxx/test/support/filesystem_test_helper.h

 }
 
 inline bool PathEqIgnoreSep(fs::path LHS, fs::path RHS) {
   LHS.make_preferred();
   RHS.make_preferred();
   return LHS.native() == RHS.native();
 }
 
+inline fs::perms NormalizeExpectedPerms(fs::perms P) {
+#ifdef _WIN32
+  // On Windows, fs::perms only maps down to one bit stored in the filesystem,
+  // a boolean readonly flag.
+  // Normalize permissions to the format it gets returned; all fs entries are
+  // read+exec for all users; writable ones also have the write bit set for
+  // all users.
+  P |= fs::perms::owner_read | fs::perms::group_read | fs::perms::others_read;
+  P |= fs::perms::owner_exec | fs::perms::group_exec | fs::perms::others_exec;
+  fs::perms Write =
+      fs::perms::owner_write | fs::perms::group_write | fs::perms::others_write;
+  if ((P & Write) != fs::perms::none)
+    P |= Write;
+#endif
+  return P;
+}
+
 struct ExceptionChecker {
   std::errc expected_err;
   fs::path expected_path1;
   fs::path expected_path2;
   unsigned num_paths;
   const char* func_name;
   std::string opt_message;
 
   explicit ExceptionChecker(std::errc first_err, const char* fun_name,
                             std::string opt_msg = {})