Re: Major CI changes - FreeBSD and Linux

[Gleb Popov]

On Mon, Jan 22, 2024 at 12:09 PM Ben Cooksley  wrote:
>
> Hi all,
>
> Over the past few weeks significant work has been undertaken to develop the 
> ability to make use of containerised builds for FreeBSD.

This is great news! Thank you for all the work done on that front.

Re: Gitlab CI Dashboards and retirement of build.kde.org

[Gleb Popov]

On Sat, Sep 3, 2022 at 7:46 AM Ben Cooksley  wrote:
>
> As previously indicated, I have now shutdown build.kde.org along with the 
> domain that supported it's version of the CI tooling.
> The repository containing that tooling has now also been archived, and the 
> former build.kde.org domain has been redirected to metrics.kde.org.
>
> The server which was acting as a builder for build.kde.org will be rebuilt in 
> the coming days and reallocated to support Gitlab CI workloads.
>
> Thanks,
> Ben

What should be used instead of binary-factory? How do I transform this link?

https://binary-factory.kde.org/view/Windows%2064-bit/job/Kate_Release_win64/1762/artifact/kate-22.08.0-1762-windows-msvc2019_64-cl.exe

Re: Rollout of Gitlab CI

[Gleb Popov]

On Sun, Oct 3, 2021 at 10:40 AM Ben Cooksley  wrote:

> As an update to this, work on FreeBSD and Android support is progressing
> well and should be made available within the next week all going well.
>

As a FreeBSD user I'm extremely excited about this. Thanks for all your
hard work!

Cheers,
> Ben
>

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd abandoned this revision.
arrowd added a comment.


  Mark the revision as closed to reflect reality.

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: kdudka, usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd added a comment.


  In D17816#677446 , @kdudka wrote:
  
  > I was wondering why copying files in Krusader or Dolphin from a 
vfat-formatted memory card stopped working for me after update.  After copying 
the first file, the transfer stopped progressing and the process ended up 
consuming 100% CPU forever.  Later I discovered that the cause of the breakage 
was this patch: If fgetxattr() fails with ENOTSUP, the code loops indefinitely 
calling fgetxattr().
  >
  > The following hotfix made copying of files in Krusader work again for me:
  >  ...
  >
  > However, the code might need bigger changes to cover all the cases.  I do 
not fully understand the (IMO over-complicated) loops around flistxattr() and 
fgetxattr().  Note that the fact that one of them uses `while (true) { ... }` 
whereas the other one uses `do { ... } while (true)` does not improve code 
readability either.
  
  
  I think, the correct fix is something like
  
diff --git a/src/ioslaves/file/file_unix.cpp 
b/src/ioslaves/file/file_unix.cpp
index 74767123..4bc87c57 100644
--- a/src/ioslaves/file/file_unix.cpp
+++ b/src/ioslaves/file/file_unix.cpp
@@ -614,8 +614,8 @@ bool FileProtocol::copyXattrs(const int src_fd, const 
int dest_fd)
 valuelen = 0;
 continue;
 }
-// happens when attr value is an empty string
-if (valuelen == 0) {
+// valuelen=0 happens when attr value is an empty string
+if (valuelen == 0 || valuelen == -1) {
 break;
 }
 } while (true);
  
  Can you check if this works for you too?

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: kdudka, usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

KACL from KIO isn't really POSIX-compliant

[Gleb Popov]

Hello everyone.

I tried compiling kio/src/core/kacl.cpp on FreeBSD, which does support
POSIX ACLs, and failed. This is because KACL's code uses non-standard
Linux-specific acl_* functions. I tried implementing them using standard
ones and it turned out to be impossible, mainly because types like acl_t
are opaque to the user of the library.

For instance, there is no standard acl_get_perm() function and it is
impossible to implement it without getting into acl_permset_t.

Now I'm a bit unsure how to solve this. I can implement non-standard
functions in FreeBSD's libc without touching KACL code, or I can rewrite
the KACL class to be truly POSIX-compliant. The latter seems to be a better
idea on the first look, but it'd require keeping track of all the
permission flags set (again, because there is no acl_get_perm()) inside.
This will turn KACL class from being a tiny acl_* wrapper into a beefy
chunk of code, but at least we won't lie that it is POSIX-compliant.

Any thoughts?

P.S. Please CC me, as I'm not subscribed.

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd added inline comments.

INLINE COMMENTS

> dfaure wrote in ConfigureChecks.cmake:12
> 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.

There are other parts of code that are guarded with `HAVE_POSIX_ACL`s, 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?

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd added inline comments.

INLINE COMMENTS

> dfaure wrote in ConfigureChecks.cmake:12
> 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.

> 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`?

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd added inline comments.

INLINE COMMENTS

> dfaure wrote in ConfigureChecks.cmake:12
> 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?

> 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?

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd updated this revision to Diff 83375.
arrowd marked an inline comment as done.
arrowd added a comment.


  - Put FileProtocol::copyXattrs inside a #if

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=83366=83375

BRANCH
  arcpatch-D17816

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd added a comment.


  Another bump.

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd updated this revision to Diff 83366.
arrowd marked 5 inline comments as done.
arrowd added a comment.


  - Address comments.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=83365=83366

BRANCH
  arcpatch-D17816

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd added inline comments.

INLINE COMMENTS

> usta wrote in file_unix.cpp:1517
> isnt this ignoring acl_from_mode part ? I mean not sure but i think we need 
> to check if it is nullptr or not before assigning it otherwise we will ignore 
> the acl_from_mode part.

You seem to be right. I blamed the code down to svn->git import and this bug 
was present all the time.

To properly fix this I'd start with a test for `KIO::chmod` job that changes 
ACL permissions, but I can't test this on FreeBSD as we don't have 
`acl_from_mode`.

Anyways, it is out of scope of this review.

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd updated this revision to Diff 83365.
arrowd marked an inline comment as done.
arrowd added a comment.


  - More const QString.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=83364=83365

BRANCH
  arcpatch-D17816

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd added inline comments.

INLINE COMMENTS

> bruns wrote in jobtest.cpp:780
> And now you **only** check the source FS, but no longer the destination FS.

To be honest, I was confused by your first comment. What was wrong with the 
first version of this code?

My understanding was that we want to skip xattr stuff if either source or 
destination doesn't support it

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd updated this revision to Diff 83364.
arrowd marked 2 inline comments as done.
arrowd added a comment.


  - Make checkXattrFsSupport accept directory as a parameter.
  - Consitify QStrings.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=83361=83364

BRANCH
  arcpatch-D17816

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd updated this revision to Diff 83361.
arrowd marked an inline comment as done.
arrowd added a comment.


  - Return "true" if the file doesn't have any xattrs.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=83334=83361

BRANCH
  arcpatch-D17816

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd added a comment.


  Bump.

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd updated this revision to Diff 83334.
arrowd added a comment.


  Rebase on master.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=83319=83334

BRANCH
  arcpatch-D17816

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd added inline comments.

INLINE COMMENTS

> bruns wrote in jobtest.cpp:487
> not done ...

There are no arrays of `512` size anymore. Or am I missing something?

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd updated this revision to Diff 83319.
arrowd marked an inline comment as done.
arrowd added a comment.


  - Remove extraneous debugging output.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=83318=83319

BRANCH
  arcpatch-D17816

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd updated this revision to Diff 83318.
arrowd marked 3 inline comments as done.
arrowd added a comment.


  - Handle attrs with empty values.
  - Add test for it.
  - Fix syscalls for FreeBSD case.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=83309=83318

BRANCH
  arcpatch-D17816

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd added inline comments.

INLINE COMMENTS

> bruns wrote in file_unix.cpp:625
> Infinite loop on valuelen == 0

Why? `ERANGE` means we need to come up with new value for `valuelen`, so we set 
it to zero and start over. On the new iteration it gets passed into 
`fgetxattr`/`extattr_get_fd` to find out sufficient value.

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd updated this revision to Diff 83309.
arrowd marked an inline comment as done.
arrowd added a comment.


  - Improve comment.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=83302=83309

BRANCH
  arcpatch-D17816

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd updated this revision to Diff 83302.
arrowd added a comment.


  - Rebase on master.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=83207=83302

BRANCH
  arcpatch-D17816

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd added a comment.


  I'd appreciate if someone test this on Linux and MacOS. I got FreeBSD covered.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd updated this revision to Diff 83207.
arrowd marked 8 inline comments as done.
arrowd added a comment.


  - Address comments.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=83203=83207

BRANCH
  arcpatch-D17816

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd added a comment.


  Almost every comment have been addressed. Please, give this another look.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd updated this revision to Diff 83203.
arrowd marked 17 inline comments as done.
arrowd added a comment.


  - Address comments.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=83192=83203

BRANCH
  arcpatch-D17816

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd marked an inline comment as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd updated this revision to Diff 83192.
arrowd marked 2 inline comments as done.
arrowd added a comment.


  - Change the algorithm in FileProtocol::copyXattrs()

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=83166=83192

BRANCH
  arcpatch-D17816

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd marked an inline comment as done.
arrowd added a comment.


  Mark stale command as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd updated this revision to Diff 83166.
arrowd marked an inline comment as done.
arrowd added a comment.


  - Use std::function to store a generator function for m_setXattrCmd's 
arguments.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=83143=83166

BRANCH
  arcpatch-D17816

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd updated this revision to Diff 83143.
arrowd marked 5 inline comments as done.
arrowd added a comment.


  - Refactor common code from compareXattr() into readXattr() as requested in 
comments.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=83068=83143

BRANCH
  arcpatch-D17816

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd updated this revision to Diff 83068.
arrowd marked 18 inline comments as done.
arrowd added a comment.


  - Use full paths to command line utilities and pass xattr args correctly.
  - Mark some stale comments as done.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=83016=83068

BRANCH
  arcpatch-D17816

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd updated this revision to Diff 83016.
arrowd added a comment.


  - Fix tests on FreeBSD.
  - Fix bug: Move `keylist.squeeze()` call before acquiring an interator.
  
  At this stage, all tests in `jobtest` pass on FreeBSD.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=82960=83016

BRANCH
  arcpatch-D17816

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd updated this revision to Diff 82960.
arrowd added a comment.


  I decided to help with this a bit.
  
  - Fix detection of  header. It requires  to be 
included too.
  - Implement test helpers for BSD (extattr).
  
  At the moment, some tests fail for me, because xattrs don't seem to be 
copied. I'll look into that later.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=80336=82960

BRANCH
  arcpatch-D17816

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

arrowd commandeered this revision.
arrowd added a reviewer: cochise.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

Re: Does FreeBSD have HAL?

[Gleb Popov]

On Sun, Jan 5, 2020 at 3:37 PM Tijl Coosemans  wrote:

> On Sun, 5 Jan 2020 10:06:08 +0100 "Tobias C. Berner"
>  wrote:
> > Moin moin
> >
> > We will replace (ancient) hal with Gleb's implementation of usdisks2
> > "bsdisks2" in the near future by default.
> >
> > Hald is likely not enabled on the CI hosts -- I could enable it :) --
> > or we could push the switch and make bsdisks2 the default in master,
> > which I would prefer.
> > What is your thought on this Gleb?
>
> Does it support cdrom/dvd?
>

Depends on what's meant by support. It should recognize it, probably
doesn't mount it.

Re: Does FreeBSD have HAL?

[Gleb Popov]

On Sun, Jan 5, 2020 at 1:06 PM Tobias C. Berner  wrote:

> Moin moin
>
> We will replace (ancient) hal with Gleb's implementation of usdisks2
> "bsdisks2" in the near future by default.
>
> Hald is likely not enabled on the CI hosts -- I could enable it :) --
> or we could push the switch and make bsdisks2 the default in master,
> which I would prefer.
> What is your thought on this Gleb?
>

I'm using bsdisks for a long time and have no problems with it. However,
tcberner was having busy-loop hangs, and I wasn't able to reproduce them
myself.

I'd say let's make a switch given that CI is green with bsdisks.


>
> mfg Tobias
>
> On Sat, 4 Jan 2020 at 18:16, David Faure  wrote:
> >
> > solid/src/CMakeLists.txt offers the option to use "UDisks2/bsdisks
> backend instead of HAL to manage disk devices" on FreeBSD, but OFF by
> default.
> >
> > So the default is the HAL backend, which however completely fails on CI:
> >
> https://build.kde.org/job/Frameworks/view/Platform%20-%20FreeBSDQt5.13/job/solid/job/kf5-qt5%20FreeBSDQt5.13/52/testReport/projectroot/autotests/halbasictest/
> > basically says that org.freedesktop.Hal is not running (on the system
> bus)
> >
> > FreeBSD users: does `qdbus --system org.freedesktop.HalManager` work for
> you?
> > If it does, any idea what should be done on the CI to make that work
> there?
> >
> > I'm really hoping for fully-green unittests one day, but FreeBSD isn't
> really helping with that :-)
>

I was working on improving tests on FreeBSD long ago, but got distracted by
$WORK. It is sad to hear that things aren't improving. I'll try my best
again in near future.

>
> > --
> > David Faure, fa...@kde.org, http://www.davidfaure.fr
> > Working on KDE Frameworks 5
> >
> >
> >
>

D22525: kioclient: Don't convert `:x:y` to `?line=x=y` for URLs starting with remote schemes.

[Gleb Popov]

arrowd added a comment.


  I'm not familiar with KDE release engineering, so I'm asking to do merging 
someone else.

REPOSITORY
  R126 KDE CLI Utilities

REVISION DETAIL
  https://phabricator.kde.org/D22525

To: arrowd, #frameworks, dfaure
Cc: wbauer, kwrite-devel, dfaure, cfeck, plasma-devel, #frameworks, LeGast00n, 
jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D22525: kioclient: Don't convert `:x:y` to `?line=x=y` for URLs starting with remote schemes.

[Gleb Popov]

This revision was automatically updated to reflect the committed changes.
Closed by commit R126:6d86fd453417: kioclient: Dont convert `:x:y` to 
`?line=xcolumn=y` for URLs starting with… (authored by arrowd).

REPOSITORY
  R126 KDE CLI Utilities

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22525?vs=61957=62126

REVISION DETAIL
  https://phabricator.kde.org/D22525

AFFECTED FILES
  kioclient/urlinfo.h

To: arrowd, #frameworks, dfaure
Cc: kwrite-devel, dfaure, cfeck, plasma-devel, #frameworks, LeGast00n, 
jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D22525: kioclient: Don't convert `:x:y` to `?line=x=y` for URLs starting with remote schemes.

[Gleb Popov]

arrowd added a comment.


  In D22525#498393 , @dfaure wrote:
  
  > I suppose the kate developers like the fact that this currently works over 
FTP, SFTP, FISH, SMB, etc.
  >  So maybe only HTTP[S]/WEBDAV should be blacklisted (because there queries 
have a different meaning, one that we can't know client-side).
  
  
  I'm confused. I just tried SFTP and SMB without the patch and it also doesn't 
work. As I said, it is invalid to rewrite URLs for remote schemas.

REPOSITORY
  R126 KDE CLI Utilities

REVISION DETAIL
  https://phabricator.kde.org/D22525

To: arrowd, #frameworks, dfaure
Cc: dfaure, cfeck, plasma-devel, #frameworks, LeGast00n, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D22525: kioclient: Don't convert `:x:y` to `?line=x=y` for URLs starting with remote schemes.

[Gleb Popov]

arrowd added a reviewer: Frameworks.

REPOSITORY
  R126 KDE CLI Utilities

REVISION DETAIL
  https://phabricator.kde.org/D22525

To: arrowd, #frameworks
Cc: cfeck, plasma-devel, #frameworks, LeGast00n, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D22525: kioclient: Don't convert `:x:y` to `?line=x=y` for URLs starting with remote schemes.

[Gleb Popov]

arrowd added a comment.


  In D22525#497556 , @cfeck wrote:
  
  > Oh, if the latter syntax also works, then you are right.
  
  
  Not sure what you mean by "also works". Current code checks if the URL ends 
with `:x:y` and turns it into `?line=x=y`. This is used to open local 
text files at the given cursor position. However, for URL's like 
`http://localhost:3000` it is invalid to perform such transformation. My patch 
checks URL scheme and applies the transformation only if it is `file://`.

REPOSITORY
  R126 KDE CLI Utilities

REVISION DETAIL
  https://phabricator.kde.org/D22525

To: arrowd
Cc: cfeck, plasma-devel, #frameworks, LeGast00n, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D22525: kioclient: Don't convert `:x:y` to `?line=x=y` for URLs starting with remote schemes.

[Gleb Popov]

arrowd added a comment.


  In D22525#497332 , @cfeck wrote:
  
  > Could we only apply the `:xx` check on the filename part, not on the server 
part? Someone might expect that `http://path.to/file.txt:99` also works for 
remote files.
  
  
  I don't think that rewriting `http://path.to/file.txt:99` to 
`http://path.to/file.txt?line=99` is a good idea. I'd better not touch 
non`file://` URLs at all.

REPOSITORY
  R126 KDE CLI Utilities

REVISION DETAIL
  https://phabricator.kde.org/D22525

To: arrowd
Cc: cfeck, plasma-devel, #frameworks, LeGast00n, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D22525: kioclient: Don't convert `:x:y` to `?line=x=y` for URLs starting with remote schemes.

[Gleb Popov]

arrowd created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
arrowd requested review of this revision.

REVISION SUMMARY
  BUG: 408632

TEST PLAN
  `kioclient5 exec http://localhost:9000` now works correctly.

REPOSITORY
  R126 KDE CLI Utilities

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D22525

AFFECTED FILES
  kioclient/urlinfo.h

To: arrowd
Cc: plasma-devel, #frameworks, LeGast00n, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D20967: [UserMetaData] Add method to query which attributes are set

[Gleb Popov]

arrowd added a comment.


  Now the test passes!

INLINE COMMENTS

> xattr_p.h:194
> +{
> +int pos;
> +QList entries;

variable 'pos' is uninitialized

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D20967

To: bruns, #baloo, #frameworks, astippich, ngraham
Cc: arrowd, #freebsd, #windows, kde-frameworks-devel, LeGast00n, domson, 
ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D21720: Enable usermetadatawritertest for all UNIXes, not only Linux.

[Gleb Popov]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:346918916474: Enable usermetadatawritertest for all 
UNIXes, not only Linux. (authored by arrowd).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21720?vs=59503=59504

REVISION DETAIL
  https://phabricator.kde.org/D21720

AFFECTED FILES
  autotests/CMakeLists.txt

To: arrowd, #frameworks, bruns
Cc: kde-frameworks-devel, #baloo, LeGast00n, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D21720: Enable usermetadatawritertest for all UNIXes, not only Linux.

[Gleb Popov]

arrowd created this revision.
arrowd added reviewers: Frameworks, bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
arrowd requested review of this revision.

TEST PLAN
  The test passes on FreeBSD.

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D21720

AFFECTED FILES
  autotests/CMakeLists.txt

To: arrowd, #frameworks, bruns
Cc: kde-frameworks-devel, #baloo, LeGast00n, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D20967: [UserMetaData] Add method to query which attributes are set

[Gleb Popov]

arrowd added a comment.


  In D20967#477314 , @bruns wrote:
  
  > Apparently, the usermetadatawritertest was not executed on *BSD before, can 
you check if the test succeeds on the old code?
  
  
  Yep, it works on master.
  
  The output with `qDebug()`s:
  
PASS   : UserMetaDataWriterTest::initTestCase()
QDEBUG : UserMetaDataWriterTest::test() extattr_list size: 14
QDEBUG : UserMetaDataWriterTest::test() extattr_list size: 14
QDEBUG : UserMetaDataWriterTest::test() extattr_list r: 14 "\ruser.xdg.tags"
FAIL!  : UserMetaDataWriterTest::test() 
'md.queryAttributes(UserMetaData::Attribute::All) & 
UserMetaData::Attribute::Tags' returned FALSE. ()
   Loc: 
[/home/arr/projects/kfilemetadata/autotests/usermetadatawritertest.cpp(55)]

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D20967

To: bruns, #baloo, #frameworks, astippich, ngraham
Cc: arrowd, #freebsd, #windows, kde-frameworks-devel, LeGast00n, domson, 
ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D20967: [UserMetaData] Add method to query which attributes are set

[Gleb Popov]

arrowd added a comment.


  It compiles now, but the test fails:
  
* Start testing of UserMetaDataWriterTest *
Config: Using QtTest library 5.12.2, Qt 5.12.2 (x86_64-little_endian-lp64 
shared (dynamic) release build; by Clang 8.0.0 (tags/RELEASE_800/final 356365))
PASS   : UserMetaDataWriterTest::initTestCase()
FAIL!  : UserMetaDataWriterTest::test() 
'md.queryAttributes(UserMetaData::Attribute::All) & 
UserMetaData::Attribute::Tags' returned FALSE. ()
   Loc: 
[/home/arr/projects/kfilemetadata/autotests/usermetadatawritertest.cpp(55)]
PASS   : UserMetaDataWriterTest::cleanupTestCase()
Totals: 2 passed, 1 failed, 0 skipped, 0 blacklisted, 1ms
* Finished testing of UserMetaDataWriterTest *
  
  I also had to do this to enable the test:
  
diff --git a/autotests/CMakeLists.txt b/autotests/CMakeLists.txt
index 49824ba..994c7bc 100644
--- a/autotests/CMakeLists.txt
+++ b/autotests/CMakeLists.txt
@@ -220,7 +220,7 @@ endif()
 #
 # UserMetaData
 #
-if(CMAKE_SYSTEM_NAME MATCHES "Linux")
+if(UNIX)
 kde_enable_exceptions()
 ecm_add_test(usermetadatawritertest.cpp ../src/usermetadata.cpp
 TEST_NAME "usermetadatawritertest"

INLINE COMMENTS

> bruns wrote in xattr_p.h:231
> Why ditto?

Right, no need to change there.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D20967

To: bruns, #baloo, #frameworks, astippich, ngraham
Cc: arrowd, #freebsd, #windows, kde-frameworks-devel, LeGast00n, domson, 
ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D20967: [UserMetaData] Add method to query which attributes are set

[Gleb Popov]

arrowd added inline comments.

INLINE COMMENTS

> xattr_p.h:204
> +#elif defined(Q_OS_FREEBSD) || defined(Q_OS_NETBSD)
> +const ssize_t size = extattr_list_file(encodedPath, 
> EXTATTR_NAMESPACE_USER, attributeName, nullptr, 0);
> +#endif

error: use of undeclared identifier 'attributeName'

> xattr_p.h:231
> +#elif defined(Q_OS_FREEBSD) || defined(Q_OS_NETBSD)
> +const ssize_t size = extattr_list_file(encodedPath, 
> EXTATTR_NAMESPACE_USER, data.data(), data.size());
> +#endif

Ditto.

Also, `size` should be `r`, probably, see below.

> xattr_p.h:234
> +
> +if (r == 0) {
> +return UserMetaData::Attribute::None;

error: use of undeclared identifier 'r'

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D20967

To: bruns, #baloo, #frameworks, astippich, ngraham
Cc: arrowd, #freebsd, #windows, kde-frameworks-devel, LeGast00n, domson, 
ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D21380: show static word wrap marker always if requested

[Gleb Popov]

arrowd accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R39 KTextEditor

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D21380

To: cullmann, #ktexteditor, #frameworks, arrowd
Cc: kwrite-devel, kde-frameworks-devel, domson, michaelh, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D18296: Add support for passing cursor information via URL parameters when running kioclient exec.

[Gleb Popov]

arrowd marked an inline comment as done.

REPOSITORY
  R126 KDE CLI Utilities

REVISION DETAIL
  https://phabricator.kde.org/D18296

To: arrowd, #plasma, #ktexteditor, broulik, #frameworks, pino, cfeck, dfaure, 
elvisangelaccio
Cc: apol, cullmann, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D18296: Add support for passing cursor information via URL parameters when running kioclient exec.

[Gleb Popov]

This revision was automatically updated to reflect the committed changes.
Closed by commit R126:8072a6acf221: Add support for passing cursor information 
via URL parameters when running… (authored by arrowd).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D18296?vs=55879=55973#toc

REPOSITORY
  R126 KDE CLI Utilities

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18296?vs=55879=55973

REVISION DETAIL
  https://phabricator.kde.org/D18296

AFFECTED FILES
  kioclient/kioclient.cpp
  kioclient/kioclient.h
  kioclient/urlinfo.h

To: arrowd, #plasma, #ktexteditor, broulik, #frameworks, pino, cfeck, dfaure, 
elvisangelaccio
Cc: apol, cullmann, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D18296: Add support for passing cursor information via URL parameters when running kioclient exec.

[Gleb Popov]

arrowd added inline comments.

INLINE COMMENTS

> dfaure wrote in urlinfo.h:77
> But then you can't do `kde-open5 www.google.fr` anymore, right?
> 
> I see what you mean with typo handling, but there is no perfect solution. 
> Either we treat typos as URLs (but it means we also treat actual URLs as 
> such), or we treat everything non-existing as a local file (breaking any use 
> of short URLs). The latter is OK for kwrite, but not for the more general 
> purpose kioclient / kde-open5.

> But then you can't do kde-open5 www.google.fr anymore, right?

Ah, right. Ok, then.

REPOSITORY
  R126 KDE CLI Utilities

REVISION DETAIL
  https://phabricator.kde.org/D18296

To: arrowd, #plasma, #ktexteditor, broulik, #frameworks, pino, cfeck, dfaure, 
elvisangelaccio
Cc: apol, cullmann, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D18296: Add support for passing cursor information via URL parameters when running kioclient exec.

[Gleb Popov]

arrowd updated this revision to Diff 55879.
arrowd marked 5 inline comments as done.
arrowd added a comment.


  Address comments.

REPOSITORY
  R126 KDE CLI Utilities

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18296?vs=54774=55879

BRANCH
  cursor

REVISION DETAIL
  https://phabricator.kde.org/D18296

AFFECTED FILES
  kioclient/kioclient.cpp
  kioclient/kioclient.h
  kioclient/urlinfo.h

To: arrowd, #plasma, #ktexteditor, broulik, #frameworks, pino, cfeck, dfaure, 
elvisangelaccio
Cc: apol, cullmann, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D18296: Add support for passing cursor information via URL parameters when running kioclient exec.

[Gleb Popov]

arrowd added a comment.


  In D18296#445829 , @dfaure wrote:
  
  > Is the makeURL function still used, or should it be removed now?
  
  
  Yep, it is still used in a bunch of other places.

INLINE COMMENTS

> dfaure wrote in urlinfo.h:39
> const QString &
> 
> And what if it's a URL? At this point this string is pathOrUrl.

> const QString &

There is `path.chop(match.capturedLength());`, which requires non-const 
`QString`.

> And what if it's a URL? At this point this string is pathOrUrl.

Well, `if (QFile::exists(path))` will return false in this case, and `url` 
would get populated by `url = QUrl::fromUserInput()`. What's wrong with that?

> dfaure wrote in urlinfo.h:77
> I'm not sure about AssumeLocalFile, in the context of kde-open.
> This is about opening existing files, not creating new files.
> So it should be removed.

It is not about creating missing files, but reaction to user typos. If I try to 
open `fiel.txt` instead of a `file.txt`, I want to get a "no such file or 
directory error message" instead of popping browser trying to open 
"http://fiel.txt;.

REPOSITORY
  R126 KDE CLI Utilities

REVISION DETAIL
  https://phabricator.kde.org/D18296

To: arrowd, #plasma, #ktexteditor, broulik, #frameworks, pino, cfeck, dfaure, 
elvisangelaccio
Cc: apol, cullmann, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D18296: Add support for passing cursor information via URL parameters when running kioclient exec.

[Gleb Popov]

arrowd added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in urlinfo.h:45-52
> Why doesn't it parse the URL if the file already exists?

If the file is named `foo:123`, it refuses to search for cursor infomation and 
just open it.

REPOSITORY
  R126 KDE CLI Utilities

REVISION DETAIL
  https://phabricator.kde.org/D18296

To: arrowd, #plasma, #ktexteditor, broulik, #frameworks, pino, cfeck, dfaure, 
elvisangelaccio
Cc: apol, cullmann, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D18296: Add support for passing cursor information via URL parameters when running kioclient exec.

[Gleb Popov]

arrowd added a comment.


  In D18296#444642 , 
@elvisangelaccio wrote:
  
  > Is it really necessary to copy `urlinfo.h` here? Wouldn't be enough to just 
create a static function and put the URL parsing logic there?
  
  
  I just copied this from Kate project. KDevelop also done this.

REPOSITORY
  R126 KDE CLI Utilities

REVISION DETAIL
  https://phabricator.kde.org/D18296

To: arrowd, #plasma, #ktexteditor, broulik, #frameworks, pino, cfeck, dfaure, 
elvisangelaccio
Cc: apol, cullmann, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D17938: Add support for passing cursor information to KRun.

[Gleb Popov]

arrowd abandoned this revision.
arrowd added a comment.


  Right.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17938

To: arrowd, #frameworks
Cc: aacid, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, bruns

D17938: Add support for passing cursor information to KRun.

[Gleb Popov]

arrowd added a comment.


  In D17938#386985 , 
@elvisangelaccio wrote:
  
  > I don't know, "cursorRow" and "cursorColumn" are pretty editor-specific. 
I'm not sure they make sense in the KRun API.
  
  
  I'd say they are both filetype-specific and service-specific. But I haven't 
found any other approach.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17938

To: arrowd, #frameworks
Cc: elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, bruns

D17938: Add support for passing cursor information to KRun.

[Gleb Popov]

arrowd created this revision.
arrowd added a reviewer: Frameworks.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
arrowd requested review of this revision.

REVISION SUMMARY
  This is the KIO part of https://bugs.kde.org/show_bug.cgi?id=398998

TEST PLAN
  With another change to `kioclient5` (not submitted yet) running `kioclient5 
exec textfile:12:34` works fine.
  Corner cases also tested.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D17938

AFFECTED FILES
  src/widgets/krun.cpp
  src/widgets/krun.h
  src/widgets/krun_p.h

To: arrowd, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D6430: Add syntax definition for julius language.

[Gleb Popov]

arrowdodger abandoned this revision.

REPOSITORY
  R216 Syntax Highlighting

REVISION DETAIL
  https://phabricator.kde.org/D6430

To: arrowdodger
Cc: #frameworks

D6430: Add syntax definition for julius language.

[Gleb Popov]

arrowdodger created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D6430

AFFECTED FILES
  data/syntax/julius.xml

To: arrowdodger
Cc: #frameworks

D6087: Haskell: Add all language pragmas as keywords.

[Gleb Popov]

This revision was automatically updated to reflect the committed changes.
Closed by commit R216:d2a38d790914: Haskell: Add all language pragmas as 
keywords. (authored by arrowdodger).

REPOSITORY
  R216 Syntax Highlighting

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6087?vs=15982=15984

REVISION DETAIL
  https://phabricator.kde.org/D6087

AFFECTED FILES
  autotests/input/highlight.hs
  autotests/reference/highlight.hs.ref
  data/syntax/haskell.xml

To: arrowdodger, #framework_syntax_hightlighting, dhaumann, cullmann
Cc: cullmann, dhaumann, #frameworks

D6087: Haskell: Add all language pragmas as keywords.

[Gleb Popov]

arrowdodger updated this revision to Diff 15982.
arrowdodger added a comment.


  Updated reference file for autotest as well.

REPOSITORY
  R216 Syntax Highlighting

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6087?vs=15981=15982

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D6087

AFFECTED FILES
  autotests/input/highlight.hs
  autotests/reference/highlight.hs.ref
  data/syntax/haskell.xml

To: arrowdodger, #framework_syntax_hightlighting, dhaumann
Cc: cullmann, dhaumann, #frameworks

D6087: Haskell: Add all language pragmas as keywords.

[Gleb Popov]

arrowdodger updated this revision to Diff 15981.
arrowdodger added a comment.


  Added autotests. Not sure if .ref file should be updated, as the tests pass 
as they are now.

REPOSITORY
  R216 Syntax Highlighting

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6087?vs=15772=15981

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D6087

AFFECTED FILES
  autotests/input/highlight.hs
  data/syntax/haskell.xml

To: arrowdodger, #framework_syntax_hightlighting, dhaumann
Cc: cullmann, dhaumann, #frameworks

D6087: Haskell: Add all language pragmas as keywords.

[Gleb Popov]

arrowdodger added a comment.


  So, can I push this?

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D6087

To: arrowdodger, #framework_syntax_hightlighting, dhaumann
Cc: dhaumann, #frameworks

D6087: Haskell: Add all language pragmas as keywords.

[Gleb Popov]

arrowdodger added a comment.


  In https://phabricator.kde.org/D6087#118963, @dhaumann wrote:
  
  > Demo for the highlighting rule - I don't think I can say anything to the 
auto completion issue. I am pretty sure it is unrelated to your xml file.
  
  
  Sure. Pragmas have following form:
  
{-# blabla #-}
  
  Specifically, pragmas that enable language extensions looks like
  
{-# LANGUAGE OverlappingInstances, BangPatterns #-}
  
  As I've found today, pragmas can span onto multiple lines:
  
{-# LANGUAGE OverlappingInstances, 
 BangPatterns
#-}

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D6087

To: arrowdodger, #framework_syntax_hightlighting, dhaumann
Cc: dhaumann, #frameworks

D6087: Haskell: Add all language pragmas as keywords.

[Gleb Popov]

arrowdodger added a comment.


  In https://phabricator.kde.org/D6087#117019, @dhaumann wrote:
  
  > Also: could you provide us with a small demo code so that we can extend our 
unit test?
  
  
  The demo for added highlighting rule, or the problem with autocompletion?

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D6087

To: arrowdodger, #framework_syntax_hightlighting, dhaumann
Cc: dhaumann, #frameworks

D6087: Haskell: Add all language pragmas as keywords.

[Gleb Popov]

arrowdodger updated this revision to Diff 15772.
arrowdodger added a comment.


  - Use WordDetect instead of StringDetect for `omport` keyword.
  - Pop `Pragma` context on `#-}` string, not newline.

REPOSITORY
  R216 Syntax Highlighting

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6087?vs=15129=15772

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D6087

AFFECTED FILES
  data/syntax/haskell.xml

To: arrowdodger, #framework_syntax_hightlighting, dhaumann
Cc: dhaumann, #frameworks

D6102: KUrlRequester: Set NOTIFY signal to textChanged() for text property.

[Gleb Popov]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:f7dfb713a852: KUrlRequester: Set NOTIFY signal to 
textChanged() for text property. (authored by arrowdodger).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6102?vs=15177=15371

REVISION DETAIL
  https://phabricator.kde.org/D6102

AFFECTED FILES
  src/widgets/kurlrequester.h

To: arrowdodger, #frameworks, dfaure
Cc: apol, elvisangelaccio, kfunk

D6087: Haskell: Add all language pragmas as keywords.

[Gleb Popov]

arrowdodger added a comment.


  I just found that the same problem exists for `import_keywords` too - when I 
type `import qual`, the completion doesn't show up, and deleteting `qual` and 
typing it again causes it to show.
  
  So, it seems to be a coding bug.

REPOSITORY
  R216 Syntax Highlighting

REVISION DETAIL
  https://phabricator.kde.org/D6087

To: arrowdodger, #framework_syntax_hightlighting
Cc: #frameworks

D6101: Work-around MSVC2017 compiler bug

[Gleb Popov]

arrowdodger accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R279 ThreadWeaver

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D6101

To: kfunk, arrowdodger, bcooksley
Cc: #frameworks

D6102: KUrlRequester: Set NOTIFY signal to textChanged() for text property.

[Gleb Popov]

arrowdodger created this revision.
Restricted Application added a project: Frameworks.

REVISION SUMMARY
  Having `NOTIFY textChanged` on `text` property may come useful when using 
KUrlRequester with KConfig. KUrlRequester field may hold paths for executables 
and sometimes it is more convenient to specify only executable name, so that it 
would be looked in PATH. But KConfig uses `setUrl()` method for saving 
KUrlRequester values, which turns `foo` into 
`/absolute/path/to/current/working/dir/foo`, which is obviouslt wrong.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D6102

AFFECTED FILES
  src/widgets/kurlrequester.h

To: arrowdodger, #frameworks

D6087: Haskell: Add all language pragmas as keywords.

[Gleb Popov]

arrowdodger edited the summary of this revision.
arrowdodger edited the test plan for this revision.
arrowdodger added a reviewer: Framework: Syntax Hightlighting.

REPOSITORY
  R216 Syntax Highlighting

REVISION DETAIL
  https://phabricator.kde.org/D6087

To: arrowdodger, #framework_syntax_hightlighting
Cc: #frameworks

D6087: Haskell: Add all language pragmas as keywords.

[Gleb Popov]

arrowdodger created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D6087

AFFECTED FILES
  data/syntax/haskell.xml

To: arrowdodger
Cc: #frameworks

D5890: Pass `--key-positions=*` to gperf instead of `-k *` because on Windows this symbol seems to have a special meaning.

[Gleb Popov]

This revision was automatically updated to reflect the committed changes.
Closed by commit R313:07a8a4e6985c: Pass `--key-positions=*` to gperf instead 
of `-k *` because on Windows this… (authored by arrowdodger).

REPOSITORY
  R313 KHtml

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5890?vs=14606=14663

REVISION DETAIL
  https://phabricator.kde.org/D5890

AFFECTED FILES
  src/CMakeLists.txt

To: arrowdodger, #frameworks, bcooksley
Cc: bcooksley

D5890: Pass `--key-positions=*` to gperf instead of `-k *` because on Windows this symbol seems to have a special meaning.

[Gleb Popov]

arrowdodger added a comment.


  In https://phabricator.kde.org/D5890#110586, @bcooksley wrote:
  
  > This looks good to me. Would this also resolve 
https://build-sandbox.kde.org/view/Frameworks/job/Frameworks%20kcodecs%20kf5-qt5%20WindowsQt5.7/6/console
 ?
  
  
  
  
  > Invalid position value or range, use 1,2,3-255,'$' or '*'.
  
  Yep, looks like it.

REPOSITORY
  R313 KHtml

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D5890

To: arrowdodger, #frameworks, bcooksley
Cc: bcooksley

D5890: Pass `--key-positions=*` to gperf instead of `-k *` because on Windows this symbol seems to have a special meaning.

[Gleb Popov]

arrowdodger created this revision.
Restricted Application added a project: Frameworks.

REVISION SUMMARY
  Pass `--key-positions=*` to gperf instead of `-k *` because on Windows this 
symbol seems to have a special meaning.

TEST PLAN
  Successfully compiled with MSVC.

REPOSITORY
  R313 KHtml

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D5890

AFFECTED FILES
  src/CMakeLists.txt

To: arrowdodger, #frameworks

D5138: Fill UDS_CREATION_TIME with the value of st_birthtime on FreeBSD

[Gleb Popov]

arrowdodger accepted this revision.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D5138

To: tcberner, adridg, arrowdodger, rakuco, dfaure
Cc: #frameworks

Re: breeze-icons and the symlink business

[Gleb Popov]

On Thu, Feb 2, 2017 at 2:49 AM, Harald Sitter  wrote:

> hola
>
> breeze-icons uses lots of symlinks. Unfortunately, ever so often our
> icon designers make a mistake and create a bad symlink. To mitigate
> this I added a bunch of tests making sure everything is nice and
> dandy.
>
> In the mean parts of the build were changed to not tolerate broken
> symlinks. While I don't really have a problem with that in of itself,
> the code largely simply ignores the possibility of broken symlinks and
> fails with the most shitty error messages you could think of. Given
> our artists are not software engineers they are having a hard time
> figuring out what is going on. And TBH, I too had to stat files to get
> to the bottom of the errors. This is a fairly shit situation as on the
> one hand we want lovely icons and on the other hand the people working
> on the icons can't understand what needs fixing without having to find
> a developer they aren't too afraid of to talk to.
>
> This really needs fixing.
>
> Notably offenders I had a fight with today:
>
>
> # breeze-validate-svg (introduced by Jos)
>
> This is a bash script running xmllint.
>
> ## Problem 1: Sources
>
> The custom target sets `SOURCES ${SVGS}` this has no notable advantage
> other than making the svgs show up in an IDE, it does however mean
> that cmake will try to inspect them (as a build tool does when they
> get told something is a source) and then falls flat on the face when
> it encounters a broken symlink as it now can't determine the source
> type resulting in this lovely error
>
> ```
> 21:39:07 CMake Error at CMakeLists.txt:70 (add_custom_target):
> 21:39:07   Cannot find source file:
> 21:39:07
> 21:39:07 /home/jenkins/sources/breeze-icons/kf5-qt5/icons-dark/
> categories/32/applications-other.svg
> ```
>
> ## Problem 2: The code
>
> The bash code in of itself runs find with -exec on xmllint. Problem
> being that if the symlink is broken xmllint will (rightfully) complain
>
> ```
> warning: failed to load external entity
> "./icons-dark/categories/32/applications-other.svg"
> ```
>
> While that is much better than the earlier problem it's still plenty
> unobvious what the underlying cause for this is. Supposedly the script
> should skip bogus symlinks.
>
> ## Problem 3: Oh but really
>
> This isn't really related to the issue of bad symlink handling:
>
> - apparently this didn't get a review. why ever would this not get a
> review?
> - this should be a test and only run when testing is enabled
> - when xmllint is not present it should report this in some form or
> fashion during the test run so one knows linting is not done
> - it should record its complaints via ctest so jenkins can display them
> properly
> - I fail to appreciate the reason this needs to depend on bash (versus
> sh, or well, neither)
>
> ## Fix Suggestion
>
> - don't needlesly set SOURCE
> - don't pass bad paths to xmllint
> - deal with stuff in problem 3
>
>
> # RCC generation (introduced by Gleb, enabled by Jaroslaw)
>
> This is a bunch of cmake rigging and helper binaries to assemble the
> icons into an rcc.
>
> ## Problem
>
> `cmake -E copy_directory` is used to copy the src tree of the themes
> into the build dir from which the resource file gets assembled. I am
> guessing copy_directory does not preserver, but resolve symlinks
> because it greets us with the ever so opaque error:
>
> ```
> Error copying directory from
> "/home/me/src/git/breeze-icons/icons-dark" to
> "/home/me/src/git/breeze-icons/build/icon
> s-dark/res".
> ```
>
> ## Fix Suggestion
>
> This is slightly less trivial since the rcc/qrc helpers seem to depend
> on resolved symlinks, so I am guessing a way to deal with this would
> be to use cmake's `file(COPY...)` instead of copy_directory and then
> have another helper run through the directories to flatten out the
> symlinks (dropping broken symlinks).
>
>
> Kindly fix this to empower the artists to know what's going on again :(
> HS
>

This PR might be relevant: https://git.reviewboard.kde.org/r/128112/
I intended to use it on Windows only, but it well may be used on *nix too.

Review Request 129590: KAuth: Make D-Bus dependency optional.

[Gleb Popov]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129590/
---

Review request for KDE Frameworks and kdewin.


Repository: kauth


Description
---

This adds an `ENABLE_DBUS` CMake option defaulting to `ON`, which can be used 
to make KAuth not depend on DBus. This is useful for some apps that are running 
on Windows.

https://www.mail-archive.com/kde-frameworks-devel@kde.org/msg34246.html


Diffs
-

  CMakeLists.txt b8b7ba7 
  autotests/CMakeLists.txt b53d760 
  src/CMakeLists.txt 1b6930d 

Diff: https://git.reviewboard.kde.org/r/129590/diff/


Testing
---


Thanks,

Gleb Popov

Re: Review Request 128112: New module: ecm_win_resolve_symlinks

[Gleb Popov]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128112/
---

(Updated Nov. 5, 2016, 2:40 p.m.)


Status
--

This change has been marked as submitted.


Review request for Extra Cmake Modules and KDE Frameworks.


Changes
---

Submitted with commit 632b8868de38e2f85fb4969bcae3dd89a2a0695c by Gleb Popov to 
branch master.


Repository: extra-cmake-modules


Description
---

When git is checking out repositories with UNIX symbolic links inside on 
Windows machine, it writes them as plain text files, containing relative path 
to the real file. This is the case for breeze-icons framework, for instance, 
and this breaks some icons that are symlinked.

This macro is intended to fix that. There is some room for performance 
improvement, but i wanted to get the feedback early.


Diffs
-

  modules/ECMWinResolveSymlinks.cmake PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/128112/diff/


Testing
---


Thanks,

Gleb Popov

Re: Review Request 129244: Make UDisks2 backend compile on FreeBSD (and, possibly, other UNIXes).

[Gleb Popov]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129244/
---

(Updated Oct. 31, 2016, 11:49 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Solid.


Changes
---

Submitted with commit 9bca1fd1993665a2233e0155a1baf9973c7a4b6f by Gleb Popov to 
branch master.


Repository: solid


Description
---

This patch makes Solid UDisks2 backend compile on FreeBSD.


Diffs
-

  src/solid/devices/CMakeLists.txt e1f3f6e 
  src/solid/devices/backends/udisks2/udisksblock.h 123e204 
  src/solid/devices/backends/udisks2/udisksblock.cpp 76cfc21 
  src/solid/devices/backends/udisks2/udisksopticaldisc.h f8dd59a 
  src/solid/devices/backends/udisks2/udisksopticaldisc.cpp a9d9c6b 
  src/solid/devices/backends/udisks2/udisksstoragedrive.h be24c7f 
  src/solid/devices/backends/udisks2/udisksstoragedrive.cpp af49063 

Diff: https://git.reviewboard.kde.org/r/129244/diff/


Testing
---


Thanks,

Gleb Popov

Re: Review Request 129197: Fix tests on FreeBSD

[Gleb Popov]

> On Oct. 16, 2016, 4:12 p.m., Tobias Berner wrote:
> > I'm kind of unsure if this is right. Yes, the tests run now, but isn't the 
> > issue rather in the way kpty works (or fails to work on FreeBSD)?
> 
> Gleb Popov wrote:
> From what i've understood, this boils down to 
> `KPtyDevicePrivate::_k_canRead()` method in kptydevice.cpp. The line 284
> 
> if (!::ioctl(q->masterFd(), PTY_BYTES_AVAILABLE, (char *) ))
> 
> returns 0 in `available` and this makes method return `false`. This, in 
> turn, make `waitFor*` methods return false too.
> 
> Now you mention it, i'm also unsure if this `ioctl` behaves different on 
> Linux.
> 
> Oswald Buddenhagen wrote:
> i've been trying to make sense of the freebsd pts layer, and utterly 
> failed. the documentation is anything between abysmal and non-existing. i 
> suggest you find and invite an actual expert.
> 
> kptyprocess keeps both ends of the pty open, so whether the child process 
> is running or not should be irrelevant. apparently, it's not. maybe this is 
> somehow related to whether the tty is the process' controlling terminal.
> 
> you can try the following:
> - remove the freebsd case from the PTY_BYTES_AVAILABLE definition near 
> the top. this was introduced 2008, before freebsd 8's release, which got an 
> entirely new pts layer. it may just work with the generic code now.
> - try replacing the masterFd() with slaveFd() in the above ioctl. that 
> would be kinda broken, but at least it would be a data point.

This turned out to be a bug in Qt itself. `QProcess` unit tests are also 
failing with the same `.waitForFinished()` error. So, we'll first deal with it 
and then i'll get back on this.


- Gleb


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129197/#review100039
---


On Oct. 16, 2016, 2:44 p.m., Gleb Popov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129197/
> ---
> 
> (Updated Oct. 16, 2016, 2:44 p.m.)
> 
> 
> Review request for KDE Frameworks, Adriaan de Groot, Tobias Berner, Oswald 
> Buddenhagen, and Martin Tobias Holmedahl Sandsmark.
> 
> 
> Repository: kpty
> 
> 
> Description
> ---
> 
> Apparently, KPtyDevice can't be operated on after KPtyProcess finishes. Tweak 
> tests accordingly, so they actually test things while the process is still 
> running.
> 
> 
> Diffs
> -----
> 
>   autotests/kptyprocesstest.cpp 8b0b5b0 
> 
> Diff: https://git.reviewboard.kde.org/r/129197/diff/
> 
> 
> Testing
> ---
> 
> make test on FreeBSD
> 
> 
> Thanks,
> 
> Gleb Popov
> 
>

Re: Review Request 129244: Make UDisks2 backend compile on FreeBSD (and, possibly, other UNIXes).

[Gleb Popov]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129244/
---

(Updated Oct. 31, 2016, 10:03 a.m.)


Review request for KDE Frameworks and Solid.


Repository: solid


Description
---

This patch makes Solid UDisks2 backend compile on FreeBSD.


Diffs (updated)
-

  src/solid/devices/CMakeLists.txt e1f3f6e 
  src/solid/devices/backends/udisks2/udisksblock.h 123e204 
  src/solid/devices/backends/udisks2/udisksblock.cpp 76cfc21 
  src/solid/devices/backends/udisks2/udisksopticaldisc.h f8dd59a 
  src/solid/devices/backends/udisks2/udisksopticaldisc.cpp a9d9c6b 
  src/solid/devices/backends/udisks2/udisksstoragedrive.h be24c7f 
  src/solid/devices/backends/udisks2/udisksstoragedrive.cpp af49063 

Diff: https://git.reviewboard.kde.org/r/129244/diff/


Testing
---


Thanks,

Gleb Popov

Re: Review Request 129122: Try to use ulog-helper if utempter does not exist

[Gleb Popov]

> On Oct. 19, 2016, 11:07 p.m., Oswald Buddenhagen wrote:
> > an update about the runtime testing would be nice ;)

Here it is: https://git.reviewboard.kde.org/r/129197/

With this and that patch, the test passes on FreeBSD.


- Gleb


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129122/#review100144
---


On Oct. 19, 2016, 10:51 p.m., Tobias Berner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129122/
> ---
> 
> (Updated Oct. 19, 2016, 10:51 p.m.)
> 
> 
> Review request for KDE Frameworks, Adriaan de Groot, Gleb Popov, Oswald 
> Buddenhagen, and Martin Tobias Holmedahl Sandsmark.
> 
> 
> Repository: kpty
> 
> 
> Description
> ---
> 
> FreeBSD does not have `/usr/libexec/*/utempter`. It does however have 
> `/usr/libexec/ulog-helper` [1].
> 
> It uses `login` instead of `add` and `logout` instead of `del`.
> 
> 
> [1] 
> https://svnweb.freebsd.org/base/head/libexec/ulog-helper/ulog-helper.c?revision=234469=markup
> 
> 
> Diffs
> -
> 
>   cmake/FindUTEMPTER.cmake 4921e58 
>   src/kpty.cpp 7bf31c3 
> 
> Diff: https://git.reviewboard.kde.org/r/129122/diff/
> 
> 
> Testing
> ---
> 
> Builds fine. Still need to test if it is actually working.
> 
> 
> Thanks,
> 
> Tobias Berner
> 
>

Re: Review Request 129197: Fix tests on FreeBSD

[Gleb Popov]

> On Oct. 16, 2016, 4:12 p.m., Tobias Berner wrote:
> > I'm kind of unsure if this is right. Yes, the tests run now, but isn't the 
> > issue rather in the way kpty works (or fails to work on FreeBSD)?

>From what i've understood, this boils down to 
>`KPtyDevicePrivate::_k_canRead()` method in kptydevice.cpp. The line 284

if (!::ioctl(q->masterFd(), PTY_BYTES_AVAILABLE, (char *) ))

returns 0 in `available` and this makes method return `false`. This, in turn, 
make `waitFor*` methods return false too.

Now you mention it, i'm also unsure if this `ioctl` behaves different on Linux.


- Gleb


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129197/#review100039
---


On Oct. 16, 2016, 2:44 p.m., Gleb Popov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129197/
> ---
> 
> (Updated Oct. 16, 2016, 2:44 p.m.)
> 
> 
> Review request for KDE Frameworks, Adriaan de Groot, Tobias Berner, Oswald 
> Buddenhagen, and Martin Tobias Holmedahl Sandsmark.
> 
> 
> Repository: kpty
> 
> 
> Description
> ---
> 
> Apparently, KPtyDevice can't be operated on after KPtyProcess finishes. Tweak 
> tests accordingly, so they actually test things while the process is still 
> running.
> 
> 
> Diffs
> -
> 
>   autotests/kptyprocesstest.cpp 8b0b5b0 
> 
> Diff: https://git.reviewboard.kde.org/r/129197/diff/
> 
> 
> Testing
> ---
> 
> make test on FreeBSD
> 
> 
> Thanks,
> 
> Gleb Popov
> 
>

Review Request 129197: Fix tests on FreeBSD

[Gleb Popov]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129197/
---

Review request for KDE Frameworks, Adriaan de Groot, Tobias Berner, Oswald 
Buddenhagen, and Martin Tobias Holmedahl Sandsmark.


Repository: kpty


Description
---

Apparently, KPtyDevice can't be operated on after KPtyProcess finishes. Tweak 
tests accordingly, so they actually test things while the process is still 
running.


Diffs
-

  autotests/kptyprocesstest.cpp 8b0b5b0 

Diff: https://git.reviewboard.kde.org/r/129197/diff/


Testing
---

make test on FreeBSD


Thanks,

Gleb Popov

Re: Review Request 129122: Try to use ulog-helper if utempter does not exist

[Gleb Popov]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129122/#review99871
---




cmake/FindUTEMPTER.cmake (line 45)
<https://git.reviewboard.kde.org/r/129122/#comment67101>

Why search in `KDE_INSTALL` folders? `ulog-helper` comes with base and 
should always be in `/usr/libexec`, no?


- Gleb Popov


On Oct. 8, 2016, 9:37 p.m., Tobias Berner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129122/
> ---
> 
> (Updated Oct. 8, 2016, 9:37 p.m.)
> 
> 
> Review request for KDE Frameworks, Adriaan de Groot, Gleb Popov, and Martin 
> Tobias Holmedahl Sandsmark.
> 
> 
> Repository: kpty
> 
> 
> Description
> ---
> 
> FreeBSD does not have `/usr/libexec/*/utempter`. It does however have 
> `/usr/libexec/ulog-helper` [1].
> 
> It uses `login` instead of `add` and `logout` instead of `del`.
> 
> 
> [1] 
> https://svnweb.freebsd.org/base/head/libexec/ulog-helper/ulog-helper.c?revision=234469=markup
> 
> 
> Diffs
> -
> 
>   cmake/FindUTEMPTER.cmake 9773963 
>   src/kpty.cpp 7bf31c3 
> 
> Diff: https://git.reviewboard.kde.org/r/129122/diff/
> 
> 
> Testing
> ---
> 
> Builds fine. Still need to test if it is actually working.
> 
> 
> Thanks,
> 
> Tobias Berner
> 
>

Re: Review Request 129020: Fix find invocation in validate_svg.sh on FreeBSD.

[Gleb Popov]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129020/
---

(Updated Sept. 26, 2016, 11:49 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Changes
---

Submitted with commit 9e0466a1996717e44ca8c3842abd304d69af7ea7 by Gleb Popov to 
branch master.


Repository: breeze-icons


Description
---

Without this patch the build was failing with

```
find: illegal option -- n
usage: find [-H | -L | -P] [-EXdsx] [-f path] path ... [expression]
   find [-H | -L | -P] [-EXdsx] -f path [path ...] [expression]
```


Diffs
-

  validate_svg.sh bbc3076 

Diff: https://git.reviewboard.kde.org/r/129020/diff/


Testing
---


Thanks,

Gleb Popov

Review Request 129020: Fix find invocation in validate_svg.sh on FreeBSD.

[Gleb Popov]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129020/
---

Review request for KDE Frameworks.


Repository: breeze-icons


Description
---

Without this patch the build was failing with

```
find: illegal option -- n
usage: find [-H | -L | -P] [-EXdsx] [-f path] path ... [expression]
   find [-H | -L | -P] [-EXdsx] -f path [path ...] [expression]
```


Diffs
-

  validate_svg.sh bbc3076 

Diff: https://git.reviewboard.kde.org/r/129020/diff/


Testing
---


Thanks,

Gleb Popov

Re: Review Request 128916: kconfig: Make test XFAIL when running as root

[Gleb Popov]

> On Sept. 16, 2016, 7:57 p.m., Sune Vuorela wrote:
> > autotests/kconfigtest.cpp, line 1376
> > <https://git.reviewboard.kde.org/r/128916/diff/4/?file=476949#file476949line1376>
> >
> > Isn't a QSKIP if user is root in the beginning of the test a better 
> > approach ?

I think, XFAIL is better idea, because if anyone ever would want to figure out 
why it is expected, it would instantly found out the cause.


- Gleb


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128916/#review99231
---


On Sept. 16, 2016, 10:17 a.m., Evgeniy Sadovnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128916/
> ---
> 
> (Updated Sept. 16, 2016, 10:17 a.m.)
> 
> 
> Review request for KDE Frameworks and Gleb Popov.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> The test checks that saving a read-only config file fails. But because root 
> can write into read-only files, the test is failing when running by root.
> Check for uid when running the test and make it XFAIL if we are running as 
> root.
> 
> 
> Diffs
> -
> 
>   autotests/kconfigtest.cpp 2b905b5 
> 
> Diff: https://git.reviewboard.kde.org/r/128916/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Evgeniy Sadovnik
> 
>

Re: Review Request 128916: kconfig: Make test XFAIL when running as root

[Gleb Popov]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128916/#review99227
---



I'd say ship it, but let's wait for someone else.

- Gleb Popov


On Sept. 16, 2016, 10:17 a.m., Evgeniy Sadovnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128916/
> ---
> 
> (Updated Sept. 16, 2016, 10:17 a.m.)
> 
> 
> Review request for KDE Frameworks and Gleb Popov.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> The test checks that saving a read-only config file fails. But because root 
> can write into read-only files, the test is failing when running by root.
> Check for uid when running the test and make it XFAIL if we are running as 
> root.
> 
> 
> Diffs
> -
> 
>   autotests/kconfigtest.cpp 2b905b5 
> 
> Diff: https://git.reviewboard.kde.org/r/128916/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Evgeniy Sadovnik
> 
>

Re: Review Request 128916: kconfig: Make test XFAIL when running as root

[Gleb Popov]

> On Sept. 15, 2016, 3:01 p.m., Gleb Popov wrote:
> > autotests/kconfigtest.cpp, line 1378
> > <https://git.reviewboard.kde.org/r/128916/diff/1/?file=476914#file476914line1378>
> >
> > I'd put both `cgLocal.writeEntry("someLocalString", "whatever2");` and 
> > `QVERIFY(!cgLocal.sync());` inside else clause.

After looking at `QEXPECT_FAIL()` documentation, i've figured you are right.


- Gleb


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128916/#review99210
---


On Sept. 15, 2016, 4:20 p.m., Evgeniy Sadovnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128916/
> ---
> 
> (Updated Sept. 15, 2016, 4:20 p.m.)
> 
> 
> Review request for KDE Frameworks and Gleb Popov.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> The test checks that saving a read-only config file fails. But because root 
> can write into read-only files, the test is failing when running by root.
> Check for uid when running the test and make it XFAIL if we are running as 
> root.
> 
> 
> Diffs
> -
> 
>   autotests/kconfigtest.cpp 2b905b5 
> 
> Diff: https://git.reviewboard.kde.org/r/128916/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Evgeniy Sadovnik
> 
>

Re: Review Request 128916: kconfig: Make test XFAIL when running as root

[Gleb Popov]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128916/#review99215
---




autotests/kconfigtest.cpp (line 1373)
<https://git.reviewboard.kde.org/r/128916/#comment66803>

getuid() isn't present on Windows. Guard this check with `#ifndef Q_OS_WIN`.


- Gleb Popov


On Sept. 15, 2016, 4:20 p.m., Evgeniy Sadovnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128916/
> ---
> 
> (Updated Sept. 15, 2016, 4:20 p.m.)
> 
> 
> Review request for KDE Frameworks and Gleb Popov.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> The test checks that saving a read-only config file fails. But because root 
> can write into read-only files, the test is failing when running by root.
> Check for uid when running the test and make it XFAIL if we are running as 
> root.
> 
> 
> Diffs
> -
> 
>   autotests/kconfigtest.cpp 2b905b5 
> 
> Diff: https://git.reviewboard.kde.org/r/128916/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Evgeniy Sadovnik
> 
>

Re: Review Request 128916: kconfig: Make test XFAIL when running as root

[Gleb Popov]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128916/#review99210
---




autotests/kconfigtest.cpp 
<https://git.reviewboard.kde.org/r/128916/#comment66800>

I'd put both `cgLocal.writeEntry("someLocalString", "whatever2");` and 
`QVERIFY(!cgLocal.sync());` inside else clause.


- Gleb Popov


On Sept. 15, 2016, 2:59 p.m., Evgeniy Sadovnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128916/
> ---
> 
> (Updated Sept. 15, 2016, 2:59 p.m.)
> 
> 
> Review request for KDE Frameworks and Gleb Popov.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> The test checks that saving a read-only config file fails. But because root 
> can write into read-only files, the test is failing when running by root.
> Check for uid when running the test and make it XFAIL if we are running as 
> root.
> 
> 
> Diffs
> -
> 
>   autotests/kconfigtest.cpp 2b905b5 
> 
> Diff: https://git.reviewboard.kde.org/r/128916/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Evgeniy Sadovnik
> 
>