Don't test setting sticky bits on files for (Free|Open)BSD

[dim]

Summary

In rL297945, jhenderson added methods for setting permissions to
sys::fs, but some of the unittests that attempt to set sticky bits
(01000) on files fail on FreeBSD and OpenBSD. This is because those
systems do not allow regular users to set sticky bits on files, only on
directories. Fix it by disabling these particular tests on FreeBSD and
OpenBSD.

[jhenderson]

Thanks for looking at this!

unittests/Support/Path.cpp

1539 ↗

(On Diff #95406)

Could you add an additional test that uses fs::all_perms, but without the sticky bit, please, so that we don't lose coverage of that enum value on FreeBSD? It wouldn't need to be inside the #if block. Something like the below:

EXPECT_EQ(fs::setPermissions(TempPath, fs::all_perms & ~fs::sticky_bit), NoError);
EXPECT_TRUE(CheckPermissions(fs::all_perms & ~fs::sticky_bit));

[dim]

Add additional test for fs::all_perms without the sticky bit.

[krytarowski]

This is valid for NetBSD as well.

[dim]

Also add NetBSD. At some point, we might want to have a global
LLVM_ON_BSD define to make these easier. :)

[krytarowski]

In D32120#734601, @dim wrote:

Also add NetBSD. At some point, we might want to have a global
LLVM_ON_BSD define to make these easier. :)

I don't agree. It would mean that it's supposed to work on BSD4.4 and derivate systems like BSD/OS.

[emaste]

In D32120#734603, @krytarowski wrote:

I don't agree. It would mean that it's supposed to work on BSD4.4 and derivate systems like BSD/OS.

I think it would be useful to have a #define for modern BSDs though, as presumably DragonFly, bitrig, EdgeBSD etc. will need it set as well (although some/all may still include the #define inherited from their parent).

unittests/Support/Path.cpp

1518 ↗

(On Diff #96284)

Maybe just "Modern BSDs require..." so that the comment doesn't repeat the test and need to be updated if/when another one is added?

[krytarowski]

In D32120#734634, @emaste wrote:

In D32120#734603, @krytarowski wrote:

I don't agree. It would mean that it's supposed to work on BSD4.4 and derivate systems like BSD/OS.

I think it would be useful to have a #define for modern BSDs though, as presumably DragonFly, bitrig, EdgeBSD etc. will need it set as well (although some/all may still include the #define inherited from their parent).

I support automatic detection of features in configure time rather than ifdefing. Haiku, Minix, Darwin are close to BSD and they perhaps shouldn't be in the category of BSD, while they share similar interfaces.

[joerg]

I think a better idea would be to just allow this tests to fail with EPERM.

[dim]

In D32120#734641, @joerg wrote:

I think a better idea would be to just allow this tests to fail with EPERM.

They resulted in EFTYPE though, not EPERM. I'm fine with either approach, though if you want to allow them to fail, the CheckPermissions calls also need to be conditional.

[jhenderson]

LGTM, with the suggested comment change by @emaste.

I'm afraid I don't know enough about the BSD world to really be able to contribute to the rest of the discussion, but it feels like it's somewhat tangential to this patch, so maybe worth bringing up on the mailing lists? FWIW, if there are more BSD cases that this applies to, I'd be in favour of some sort of combined macro as suggested.

llvm/trunk/unittests/Support/Path.cpp

#else
   EXPECT_TRUE(CheckPermissions(fs::all_exe));
 
   EXPECT_EQ(fs::setPermissions(TempPath, fs::set_uid_on_exe), NoError);
   EXPECT_TRUE(CheckPermissions(fs::set_uid_on_exe));
 
   EXPECT_EQ(fs::setPermissions(TempPath, fs::set_gid_on_exe), NoError);
   EXPECT_TRUE(CheckPermissions(fs::set_gid_on_exe));
 
+  // Modern BSDs require root to set the sticky bit on files.
+#if !defined(__FreeBSD__) && !defined(__NetBSD__) && !defined(__OpenBSD__)
   EXPECT_EQ(fs::setPermissions(TempPath, fs::sticky_bit), NoError);
   EXPECT_TRUE(CheckPermissions(fs::sticky_bit));
 
   EXPECT_EQ(fs::setPermissions(TempPath, fs::set_uid_on_exe |
                                              fs::set_gid_on_exe |
                                              fs::sticky_bit),
             NoError);
   EXPECT_TRUE(CheckPermissions(fs::set_uid_on_exe | fs::set_gid_on_exe |
                                fs::sticky_bit));
 
   EXPECT_EQ(fs::setPermissions(TempPath, fs::all_read | fs::set_uid_on_exe |
                                              fs::set_gid_on_exe |
                                              fs::sticky_bit),
             NoError);
   EXPECT_TRUE(CheckPermissions(fs::all_read | fs::set_uid_on_exe |
                                fs::set_gid_on_exe | fs::sticky_bit));
 
   EXPECT_EQ(fs::setPermissions(TempPath, fs::all_perms), NoError);
   EXPECT_TRUE(CheckPermissions(fs::all_perms));
+#endif // !FreeBSD && !NetBSD && !OpenBSD
+
+  EXPECT_EQ(fs::setPermissions(TempPath, fs::all_perms & ~fs::sticky_bit),
+                               NoError);
+  EXPECT_TRUE(CheckPermissions(fs::all_perms & ~fs::sticky_bit));
 #endif
 }
 
 } // anonymous namespace