Fix build on FreeBSD broken by d7cce9937d5e9af2753fadb82d11f308b58bb8fa
ClosedPublic

Authored by rominf on Mar 26 2018, 1:20 PM.

Details

Summary

Support hidden NTFS files only on OS that support extended file attributes. For this purpose check if sys/xattr.h exist via CMake.

Diff Detail

Repository
R241 KIO
Branch
fix-ntfs-hidden-files-on-freebsd
Lint
No Linters Available
Unit
No Unit Test Coverage
rominf created this revision.Mar 26 2018, 1:20 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 26 2018, 1:20 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
rominf requested review of this revision.Mar 26 2018, 1:20 PM
kossebau added a comment.EditedMar 26 2018, 1:33 PM

Thanks for coming up so quickly with a fix :)

You surely wanted to also wrap the include of the "sys/xattr.h" header in the new condition? Missing from the patch.

src/ioslaves/file/config-kioslave-file.h.cmake
16

Please add a trailing newline to the end of the file.

rominf updated this revision to Diff 30626.Mar 26 2018, 1:39 PM
  • Actualy fixing build
rominf updated this revision to Diff 30627.Mar 26 2018, 1:40 PM
  • Add a trailing newline to the end of the file
kossebau added inline comments.Mar 26 2018, 1:41 PM
src/ioslaves/file/config-kioslave-file.h.cmake
14

While FindACL.cmake sets internally the cmake variable HAVE_SYS_XATTR_H, it is not part of the officially set variables (cmp. notes in header of that file)

So more future-proof will be to do an explicit

check_include_files(sys/xattr.h HAVE_SYS_XATTR_H)

in src/ioslaves/file/CMakeLists.txt instead of reyling on an undocumented side-effect of find_package(ACL).

rominf updated this revision to Diff 30629.Mar 26 2018, 1:44 PM
  • Consider criticism

From quick pure code reading this looks fine to me. +1

Have not tested though right now, so have to leave the ship-it to actual Dolphin/KIO maintainers.

dfaure requested changes to this revision.Mar 26 2018, 7:08 PM
dfaure added inline comments.
src/ioslaves/file/file_unix.cpp
416

should be #if, not #ifdef.

It's always defined, to either 0 or 1.

550

same

This revision now requires changes to proceed.Mar 26 2018, 7:08 PM
rominf updated this revision to Diff 30696.Mar 27 2018, 7:19 AM
  • Replace #ifdef with #if

This change or an equivalent fix needs to be merged in relatively urgently as it is now creating a maintainability issue for the CI system.
As a consequence of the breakage the Dependency Builds are failing and they need to complete a successful run in order to restore the ability of the system to successfully build several projects.

Among those affected are KMyMoney and plasma-workspace.

As I have mentioned previously, Frameworks must always be kept in a buildable condition on all relevant platforms.

@dfaure, is this good now?

dfaure accepted this revision.Mar 29 2018, 7:00 AM
This revision is now accepted and ready to land.Mar 29 2018, 7:00 AM
This revision was automatically updated to reflect the committed changes.