Review Request 126161: OS X housekeeping
René J.V. Bertin
rjvbertin at gmail.com
Wed Nov 25 16:57:59 UTC 2015
> On Nov. 25, 2015, 8:39 a.m., David Faure wrote:
> > src/kdeinit/kinit.cpp, line 1621
> > <https://git.reviewboard.kde.org/r/126161/diff/1/?file=418134#file418134line1621>
> >
> > Yes if you have to run a separate process which will then dlopen the kdeinit module, the whole purpose of kdeinit is moot. You might as well simplify your life by getting rid of most of the Q_OS_OSX code in this file and simply acting as if the kdeinit module doesn't exist, on Q_OS_OSX.
> >
> > The existing code to fallback to executing the binary directly will do exactly the same as your generic proxy, possibly even faster (no dlopen).
>
> René J.V. Bertin wrote:
> You're undoubtedly right, I considered doing this myself. It would amount to making the `--no-fork` option the default, no?
>
> I don't feel comfortable/ready for that right now, without having had a working equivalent to (the patched) kdeinit4 out there in the wild for observation. Can we leave your suggestion for a 2nd round of housekeeping and cleanup?
>
> David Faure wrote:
> Not at all, --no-fork is about kdeinit's own startup, not about the way it starts other applications.
>
> In general I don't see much purpose in pushing complex code if we confirm it to serve no purpose at all.
> But I looked a bit further into it, and in fact kinit's launch() does fork+dlopen or fork+exec, depending on whether the kdeinit module was found.
>
> So if fork + exec is a problem on OSX, then indeed that needs to be addressed
> But if your patch does fork + exec_helper + dlopen, then that is unnecessarily complex.
>
> The simple version of what I have in mind is http://www.davidfaure.fr/2015/kinit.osx.cpp.diff i.e. never using QLibrary on OSX. Would that work?
>
> René J.V. Bertin wrote:
> Well, all I can say is that with that patch nothing crashes, `kdeinit5 --kded` still launches kded5 but as a result I now no longer see something like (in `ps` output)
>
> ```
> 12980 1 400c 0 33 0 2510184 6716 - Ss 0 ?? 0:00.03 /opt/local/bin/kdeinit5 --suicide --nofork
> 12981 12980 4004 0 48 0 2641864 14856 - S 0 ?? 0:00.12 /opt/local/libexec/kde5/kf5/klauncher --fd=9 libkdeinit5_klauncher
> ```
>
> but
>
> ```
> 13211 1 400c 0 33 0 2527592 6724 - Ss 0 ?? 0:00.02 /opt/local/bin/kdeinit5 --suicide --nofork
> 13225 1076 4006 0 31 0 2432948 576 - S+ 0 ttys004 0:00.00 fgrep kdeinit5
> ```
>
> And `kwrapper5 /path/to/kwrite` now longer launches an kwrite process that can be killed via `killall kwrite` or equivalent. All that is probably to be expected, but that latter observation does feel like a regression of sorts to me.
>
>
> BTW, I noticed that kded5 will have to be turned into an agent too, it has no business showing up in the Dock.
>
> David Faure wrote:
> Yes, killall only works on linux because of the proc_settitle stuff.
>
> I think my approach would "fix" that "regression" for killall kwrite because it would be a real fork'ed+exec'ed process.
>
> You are seeing the drawbacks of the kdeinit mechanism (e.g. killall, and probably what the `ps` entry looks like for kwrite, too) without benefiting from its advantages (faster startup), if you have to go through a helper process.
>
> René J.V. Bertin wrote:
> Erm we have a communications problem here.
>
> No, with "my" fix, applications started through kwrapper appear as individual entries in `ps` listings, with your fix only the `kwrapper5 /path/to/command` entry shows up.
>
> Also, if your fix does a "real fork + exec", how come it doesn't run afoul of the limitations imposed on that on OS X? Only because it doesn't actually load `l` (the result of QLibrary(libpath)), meaning the crash will return the day kdeinit5 itself does something off-limits? The helper from my fix is probably much less likely to develop such symptoms. And even if it does (through Qt) it would be much easier to cure (make it not use Qt at all but only POSIX APIs).
>
> Looking at this more closely I do think that my fix could borrow from yours, and skip the whole `QLibrary l(libpath)` bit (including creating `l`) because that is being done for nothing. For the rest, using a helper does seem to give a better "emulation" of what `kdeinit5` does on Linux, no?
With "my" fix:
```
17906 1 400c 0 33 0 2527592 6744 - Ss 0 ?? 0:00.02 /opt/local/bin/kdeinit5 --suicide --nofork
17907 17906 4004 0 33 0 2641356 14844 - S 0 ?? 0:00.10 /opt/local/libexec/kde5/kf5/klauncher --fd=9
505 17925 1096 4006 0 33 0 2491072 5188 - S+ 0 ttys006 0:00.01 kwrapper5 /Applications/MacPorts/KDE4/kwrite.app/Contents/MacOS/kwrite
505 17926 17906 400c 0 48 0 4831548 47552 - S 0 ?? 0:01.00 /Applications/MacPorts/KDE4/kwrite.app/Contents/MacOS/kwrite
```
- René J.V.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126161/#review88784
-----------------------------------------------------------
On Nov. 25, 2015, 5:19 p.m., René J.V. Bertin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126161/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2015, 5:19 p.m.)
>
>
> Review request for KDE Software on Mac OS X and KDE Frameworks.
>
>
> Repository: kinit
>
>
> Description
> -------
>
> This patch addresses several issues with the OS X adaptations:
>
> -1 replaces the obsolete Q_OS_MAC with Q_OS_OSX
> -2 builds the relevant applications `nongui` instead of as app bundles
> -3 turns klauncher into an "agent" by setting `LSUIElement` to true programmatically
> -4 ports a patch that has been in MacPorts' `port:kdelibs4` since October 14th 2009, which prevents a kdeinit crash that is caused by calling exec after `fork()` in an application that has used non-POSIX APIs and/or calling those APIs in the exec'ed application. This patch (originally by MacPorts developers Jeremy Lainé and Jeremy Lavergne) rearranges call order and uses a proxy application to do the actual exec.
>
>
> Diffs
> -----
>
> src/klauncher/klauncher.h e155f72
> src/start_kdeinit/CMakeLists.txt 46d6cb3
> src/kdeinit/kinit.cpp a18008a
> src/kdeinit/CMakeLists.txt f94db71
> src/wrapper.cpp 95b7ec2
> src/klauncher/klauncher.cpp 8b3d343
> src/klauncher/klauncher_main.cpp f69aaa5
> src/klauncher/CMakeLists.txt 746edfa
> src/kdeinit/kdeinit5_proxy.cpp PRE-CREATION
>
> Diff: https://git.reviewboard.kde.org/r/126161/diff/
>
>
> Testing
> -------
>
> On OS X 10.9.5 with Qt 5.5.1 and KF5rameworks 5.16.0 . With this patch, starting `kded5` will launch kdeinit5 and klauncher5 as expected, but `kdeinit5 --kded` does not yet launch `kded5`. This is probably acceptable for typical KF5 use on OS X (kded5 can be launched as a login item or as a LaunchAgent) but I will have another look at why the kded isn't started.
>
> I am not yet able to perform further testing; practice will for instance have to show whether point 2 above needs revision (apps that need to be installed as app bundles).
>
> Similarly it will have to be seen whether point 3 above has any drawbacks. Applications running as agents do not show up in the Dock or App Switcher. Thus, klauncher will not be able to "turn itself into" an application that does have a full GUI presence with my current modification. I don't know if that's supposed to be possible though.
> NB: I have been building the KDE4 klauncher in a way that makes it impossible to construct a GUI at all, so I'm not expecting issues in KF5 as long as klauncher's role hasn't evolved too much.
>
>
> Thanks,
>
> René J.V. Bertin
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20151125/376e6ff0/attachment-0001.html>
More information about the Kde-frameworks-devel
mailing list