Differential D17816 Diff 83143 src/ioslaves/file/ConfigureChecks.cmake

Changeset View

Standalone View

View Options

src/ioslaves/file/ConfigureChecks.cmake

1include(CheckFunctionExists) 1​include(CheckFunctionExists)
2include(CheckLibraryExists) 2​include(CheckLibraryExists)
3include(CheckIncludeFile) 3​include(CheckIncludeFile)
4include(CheckIncludeFiles) 4​include(CheckIncludeFiles)
5include(CheckStructHasMember) 5​include(CheckStructHasMember)
6 6​
7check_include_files(sys/time.h HAVE_SYS_TIME_H) 7​check_include_files(sys/time.h HAVE_SYS_TIME_H)
8check_include_files(string.h HAVE_STRING_H) 8​check_include_files(string.h HAVE_STRING_H)
9check_include_files(limits.h HAVE_LIMITS_H) 9​check_include_files(limits.h HAVE_LIMITS_H)
10​check_include_files(sys/xattr.h HAVE_SYS_XATTR_H)
dfaureUnsubmitted
Not Done

This is already done by cmake/FindACL.cmake, why not add the other one (extattr.h) to that file as well?

The CMakeLists.txt in this directory calls find_package(ACL) so it'll be used.

Even though this isn't technically about ACLs, I'm guessing it all links *because* FindACL.cmake links to libattr, right?
And then this might also mean that the condition in FindACL.cmake needs to be updated, it currently *requires* HAVE_SYS_XATTR_H instead of allowing both variants.

But then I guess this means kpropertiesdialog.cpp needs to be ported to the sys/extattr.h API.

Has this been tested on a system with sys/extattr.h? I'm wondering if what will happen is: FindACL didn't find sys/attr.h so it doesn't link to libattr, and then the new code here fails to link. Or am I missing something?

dfaure: This is already done by `cmake/FindACL.cmake`, why not add the other one (extattr.h) to that…
arrowdAuthorUnsubmitted
Not Done

Has this been tested on a system with sys/extattr.h?

I was working on this revision on such system all the time. FreeBSD contains extattr bits in its libc, so no extra libraries are required.

So, what should be done here, then? Just move HAVE_SYS_EXTATTR_H check to FindACL.cmake module?

arrowd: > Has this been tested on a system with sys/extattr.h? I was working on this revision on such…
dfaureUnsubmitted
Not Done

Ah, I see, OK. No extra lib needed makes it simple.

The FindACL.cmake stuff is a bit of a mess now, with the need for extended attributes outside the ACL related code.
Maybe this could be split up into "find extended attribute stuff" and "find ACL stuff", the latter relying on the former.
But this merge request has been pending for long enough, let's do buildsystem refactoring as part of it.

Let's leave this part as is for now.

If you feel like it, I suggest followup commits to 1) enable the ACL stuff on systems with extattr, see the little bit of code in kpropertiesdialog.cpp, and 2) separate the cmake stuff for ACLs from the cmake stuff for extended attributes.

dfaure: Ah, I see, OK. No extra lib needed makes it simple. The FindACL.cmake stuff is a bit of a mess…
arrowdAuthorUnsubmitted
Not Done
  1. enable the ACL stuff on systems with extattr, see the little bit of code in kpropertiesdialog.cpp

By that you mean that I should edit the CMake module to define HAVE_POSIX_ACL when extattr headers are found?

Or should I change checks in kpropertiesdialog.cpp from HAVE_POSIX_ACL to HAVE_*ATTR_H?

arrowd: > 1) enable the ACL stuff on systems with extattr, see the little bit of code in…
dfaureUnsubmitted
Not Done

I'm talking about the implementation of fileSystemSupportsACL in kpropertiesdialog.cpp, which uses getxattr.
But now that I take another look at it, I see that it's already implemented on FreeBSD, without the use of extattr, it uses statfs instead.
So I think I was wrong, this part is fine.

I guess the only thing that's a bit weird is that kio_file's attr code (unrelated to ACLs) relies on FindACL linking to the right lib (on Linux). It would be cleaner if the attr stuff was separate. But no big deal I guess.

dfaure: I'm talking about the implementation of fileSystemSupportsACL in kpropertiesdialog.cpp, which…
arrowdAuthorUnsubmitted
Not Done

There are other parts of code that are guarded with HAVE_POSIX_ACLs, and these aren't enabled ATM for FreeBSD. So, the change is needed and I'm willing to implement it.

I plan to move set(HAVE_POSIX_ACL ...) part into FindACL.cmake and use this module everywhere. Does that sound OK?

arrowd: There are other parts of code that are guarded with `HAVE_POSIX_ACL`s, and these aren't enabled…
dfaureUnsubmitted
Not Done

Yep.

dfaure: Yep.
11​check_include_files("sys/types.h;sys/extattr.h" HAVE_SYS_EXTATTR_H)
12​
10check_function_exists(sendfile HAVE_SENDFILE) 13​check_function_exists(sendfile HAVE_SENDFILE)
11 14​
12check_function_exists(posix_fadvise HAVE_FADVISE) # kioslave 15​check_function_exists(posix_fadvise HAVE_FADVISE) # kioslave
13 16​
14check_library_exists(volmgt volmgt_running "" HAVE_VOLMGT) 17​check_library_exists(volmgt volmgt_running "" HAVE_VOLMGT)
15 18​
16check_struct_has_member("struct dirent" d_type dirent.h HAVE_DIRENT_D_TYPE LANGUAGE CXX) 19​check_struct_has_member("struct dirent" d_type dirent.h HAVE_DIRENT_D_TYPE LANGUAGE CXX)