[KDE/Mac] Review Request 119844: [OS X] backend improvements for kwallet/Keychain integration

[RJVB Bertin]

> On Aug. 26, 2014, 10:03 p.m., Valentin Rusu wrote:
> > Ok to ship it. However, see the note on your other review request about KF5 and the deprecated kdelibs.
> 
> RJVB Bertin wrote:
>     Isn't this the KF5-Kwallet code you mentioned in that note?!
>     
>     Also, I don't have commit powers here either, so can someone else please do the shipping?
> 
> Ian Wadham wrote:
>     Amazing work, René. Congratulations!
>     
>     I would be happy to commit both sets of code for you, except that I do not feel I have the right. We are in a Catch 22 situation. The changes for KDE 4 would count as a new "feature", rather than a bug fix, so I do not think they would be accepted. KDE 4.14.x is the last KDE 4 release and kdelibs 4 no longer has a "master" branch.
>     
>     OTOH Frameworks/KF 5 implementation on OS X is still in progress, so it is not possible to test your work there. I see that you did a "placebo" test on KDE 4 in Linux, i.e. that your changes leave KWallet running as normal on Linux/KDE desktops. Maybe a core KDE developer could do a similiar test on KF 5. If successful, he/she could commit your work there.
>     
>     Valentin?

Ian, I'm not sure why this would qualify as a new feature. Both things I have worked on existed, which is why this RR is presented as an improvement.

I have a feeling that my modifications to AuthServices undo a regression that slipt in since whomever it was implemented the backend for OS X relying on DBus to handle the actual authentication.

As to the kwallet modifications: support for using the OS X keychain was added and implemented as a build-time backend, linking in kwallet_mac.cpp instead of kwallet.cpp . I did add qosxkeychain.{cpp,h} to the Mac "backend", but I could just as well have implemented the corresponding class in kwallet_mac.cpp. For the rest, all I did was fleshing out the work Franck had started, to bring the Mac backend on par with the regular one as much as possible.

As to the placebo test on Linux: close, but not a perfectly accurate way to call it. It's true that the Mac backends I worked on aren't used on Linux, but the cmake files and kwallettest.cpp are. So it was a real test, ensuring I didn't introduce regressions to the build system, and also a check if the (new) kwallet tests run on Linux.

In short: no need to wake sleeping dogs or dragons, AFAIC :)


- RJVB


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


On Aug. 19, 2014, 7:59 p.m., RJVB Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119844/
> -----------------------------------------------------------
> 
> (Updated Aug. 19, 2014, 7:59 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and Valentin Rusu.
> 
> 
> Repository: kwallet
> 
> 
> Description
> -------
> 
> The submitted improvements to kwallet consist of a number of changes to existing files, as well as 2 new files that contain the actual interface to OS X's SecKeychain API.
> 
> With these modifications, KDE wallets are stored in the same location as native OS X keychains, and both can be managed (up to a certain extent) in the OS X Keychain utility as well as the kwalletmanager. In addition, password prompts no longer get posted somewhere in the background.
> 
> 
> Diffs
> -----
> 
>   src/api/KWallet/CMakeLists.txt 9709559 
>   src/api/KWallet/kwallet_mac.cpp d93e5ae 
>   src/api/KWallet/qosxkeychain.h PRE-CREATION 
>   src/api/KWallet/qosxkeychain.cpp PRE-CREATION 
>   tests/KWallet/CMakeLists.txt b155f64 
>   tests/KWallet/kwallettest.cpp 3351a6b 
> 
> Diff: https://git.reviewboard.kde.org/r/119844/diff/
> 
> 
> Testing
> -------
> 
> Testing and development of these was done on OS X 10.6.8 running KDE 4.12.5, which is part of my production environment (https://trac.macports.org/ticket/44473). Since I am not currently able to build (parts of) KF5, porting of my modifications to KF5/KWallet has been done in source only. However, I have good hope that there will be little bugs to review in this request, given the lack of non-esthetic (formatting) modifications to the current kwallet_mac.cpp and kwallettest.cpp.
> 
> 
> Thanks,
> 
> RJVB Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20140829/4b3fe474/attachment-0001.html>