This is an archive of the discontinued LLVM Phabricator instance.

[llvm-ar] Unsupport test on FreeBSD
ClosedPublic

Authored by sameerarora101 on Jun 29 2020, 9:25 AM.

Details

Summary

The test error-opening-directory.test fails on FreeBSD as it allows
for reading in directories just fine. This patch adds `# UNSUPPORTED:
system-freebsd` to the test to prevent it from running on FreeBSD.

Diff Detail

Event Timeline

sameerarora101 created this revision.Jun 29 2020, 9:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2020, 9:25 AM

This depends on the underlying file system and FreeBSD version. However, I guess marking it as unsupported is the simplest solution to avoid test failures.

This depends on the underlying file system and FreeBSD version. However, I guess marking it as unsupported is the simplest solution to avoid test failures.

I concur... the change in FreeBSD that stops allowing it by default for all filesystems will get MFC'd and make it into 12.2 if there's not significant need to flip it back on, but stable/11 will remain supported for some time yet.

Hi everyone, sorry I am a little confused by the comments 😔 What exactly should I be doing? For reference, the issue was reported for the diff D80838. Thanks!

This depends on the underlying file system and FreeBSD version. However, I guess marking it as unsupported is the simplest solution to avoid test failures.

! In D82786#2120498, @kevans wrote:

! In D82786#2120469, @arichardson wrote:

This depends on the underlying file system and FreeBSD version. However, I guess marking it as unsupported is the simplest solution to avoid test failures.

I concur... the change in FreeBSD that stops allowing it by default for all filesystems will get MFC'd and make it into 12.2 if there's not significant need to flip it back on, but stable/11 will remain supported for some time yet.

Can you rewrite the comment to make it more accurate?

Hi everyone, sorry I am a little confused by the comments 😔 What exactly should I be doing? For reference, the issue was reported for the diff D80838. Thanks!

Sorry- the review is fine as-is. There is some nuance here, but it is better to just mark this unsupported for now (as you've done) and we (FreeBSD) can revisit later when all supported versions do not allow reading dirfds.

Sorry- the review is fine as-is. There is some nuance here, but it is better to just mark this unsupported for now (as you've done) and we (FreeBSD) can revisit later when all supported versions do not allow reading dirfds.

That said we should put a comment by the UNSUPPORTED so someone who comes across this in the future might remove it again when no longer needed. E.g. make the comment "Unsupported on FreeBSD as FreeBSD 12 and earlier allow reading directories by default."

ok, updated the comment. Thanks.

Updating comment

jhenderson accepted this revision.Jun 30 2020, 12:26 AM

Looks good to me, but it would be good though if somebody with a freebsd system can confirm this works - as far as I can see this is the first system-freebsd reference in the entirety of the LLVM, clang and lld lit test suites. @kevans, perhaps you can confirm?

This revision is now accepted and ready to land.Jun 30 2020, 12:26 AM

We're just waiting on someone with access with a FreeBSD system to confirm this works before pushing this.

We're just waiting on someone with access with a FreeBSD system to confirm this works before pushing this.

CC @emaste / @dim (maybe @adalava? =-)) -- I don't quite have the cycles to do this at this moment, unfortunately.

kevans removed a reviewer: kevans91.Jul 2 2020, 4:52 PM
arichardson accepted this revision.Jul 3 2020, 4:29 AM

I can confirm that without this patch tools/llvm-ar/error-opening-directory.test fails work again on FreeBSD12 and with it, the test is skipped as expected.
I checked that the system I was testing on has readable directories:

~/cheri/build/upstream-llvm-project-build> cat .
���P
~/cheri/build/upstream-llvm-project-build> echo $?
0
MaskRay accepted this revision.Jul 3 2020, 10:55 AM

LGTM.

This revision was automatically updated to reflect the committed changes.
adalava added a comment.EditedJul 3 2020, 1:51 PM

This was already commited, but for the record , I tested it on FreeBSD 13/powerpc64

[root@ ~/src/llvm-project]# build/bin/llvm-lit llvm/test/tools/llvm-ar/error-opening-directory.test
-- Testing: 1 tests, 1 workers --
UNSUPPORTED: LLVM :: tools/llvm-ar/error-opening-directory.test (1 of 1)

Testing Time: 0.18s
  Unsupported: 1

Thanks @arichardson and @adalava for testing, and thanks @MaskRay for committing! Sorry about the breakage.